Skip to content

Add V137 email event dedup indexes and aws_message_id backfill - #604

Merged
ddon merged 8 commits into
BeamLabEU:mainfrom
timujinne:emails-v137-dedup-indexes
Jun 24, 2026
Merged

Add V137 email event dedup indexes and aws_message_id backfill#604
ddon merged 8 commits into
BeamLabEU:mainfrom
timujinne:emails-v137-dedup-indexes

Conversation

@timujinne

Copy link
Copy Markdown
Contributor

Summary

Adds migration V137 for the Emails module: database-level event deduplication, an aws_message_id backfill for the hot SQS lookup, and a set of performance indexes for the admin email list and analytics.

Changes

  • Event dedup: backs the Emails.Event schema's declared unique constraints with real partial unique indexes — one per (email_log_uuid, event_type) for single-occurrence types (delivery/bounce/complaint/…), and one per (email_log_uuid, event_type, occurred_at) for multi-occurrence types (open/click). Pre-existing duplicates are removed first (keeping the earliest uuid, which is UUIDv7 time-ordered).
  • aws_message_id backfill: populates the dedicated indexed column from the legacy headers JSONB for old rows, conflict-safe via DISTINCT ON + NOT EXISTS.
  • Performance indexes: pg_trgm substring search (to/subject/campaign_id) for the admin list, per-template open/click analytics composites, and a partial index for the archiver's body-compression scan.
  • Bumps the migration registry @current_version to 137.

Test Plan

  • Migration applied on a live parent app (COMMENT ON TABLE phoenix_kit IS '137'), all 8 indexes present
  • 0 remaining duplicate events after dedup; admin email-list queries (trgm) run cleanly
  • down/0 drops the indexes (the data-only backfill is intentionally not reversed)

…on_spaces
DBs that ran V91 before the locations feature was added to it never got the
phoenix_kit_locations table. V122 adds phoenix_kit_location_spaces with a FK to
it, so the migration failed with undefined_table on those installs. Create the
parent table idempotently (create_if_not_exists, mirroring V91) before the
child; a no-op where V91 already created it.
Backs the Emails module's declared unique constraints with real partial unique indexes (single- vs multi-occurrence event types), removing pre-existing duplicates first. Backfills aws_message_id from legacy headers JSONB for the hot SQS lookup, and adds pg_trgm / per-template analytics / archiver performance indexes for the admin email list.
Bumps the migration registry @current_version to 137.

@timujinnetimujinne left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR adds migration V137, bumping the registry to 137 and introducing a single new migration module. It (a) deletes pre-existing duplicate phoenix_kit_email_events rows and creates two partial unique indexes (single- vs multi-occurrence event types) to enforce constraints the schema already declared but the DB never had; (b) backfills aws_message_id on phoenix_kit_email_logs from legacy headers JSONB; and (c) adds pg_trgm search indexes, per-template analytics composites, and an archiver scan index. The migration is well-documented, follows the repo's prefix/COMMENT ON TABLE versioning conventions exactly, is idempotent throughout (IF NOT EXISTS / IF EXISTS), and down/0 cleanly reverses every index up/0 creates. The dedup DELETE (keeping MIN(uuid) as earliest UUIDv7) and the conflict-safe backfill (DISTINCT ON + NOT EXISTS) are both correct. Overall this is solid, low-risk work; my findings are operational/clarity notes rather than blockers.

Findings

IMPROVEMENT - MEDIUM

  • v137.ex:51-67 — GIN trigram + partial index builds run non-CONCURRENTLY inside the migration transaction, taking an ACCESS EXCLUSIVE/SHARE lock and blocking writes to phoenix_kit_email_logs for the duration. This is consistent with the framework (no migration here uses disable_ddl_transaction, and V111 builds its trgm index the same way), so it isn't a regression — but phoenix_kit_email_logs is a high-volume hot table and three GIN trgm indexes are the most expensive operation in the file. Worth a one-line note in the moduledoc that on large installs this migration should be run in a maintenance window, since the framework's transactional model precludes CREATE INDEX CONCURRENTLY here.

  • v137.ex:20-26 / multi-occurrence index — the "exact SQS redelivery (identical timestamp) is collapsed" guarantee hinges on occurred_at being byte-identical across a redelivery.occurred_at is utc_datetime_usec; if any code path stamps it with insert-time NOW() rather than the AWS event timestamp on each delivery, two redeliveries get distinct microsecond values and the unique index will not collapse them — defeating the documented dedup for open/click. The migration itself is fine; please confirm (in the emails module's event-ingest path, outside this diff) that occurred_at is sourced from the immutable AWS event timestamp, not generated per-insert. If it can be regenerated, soften the moduledoc claim.

NITPICK

  • v137.ex:69-91 — backfill COALESCE(headers->>'aws_message_id', 'X-AWS-Message-Id', 'MessageId'). The first key (aws_message_id) is the app's own snake_case header and the latter two are AWS/SES casings. Worth a one-line comment noting the precedence order is intentional (prefer the app's normalized key) so a future reader doesn't "fix" it.
  • v137.ex — the dedup DELETE ... USING self-joins (lines for single- and multi-occurrence) do a full self-join on a table with no bigint id. Correct as written; just flagging that on a very large phoenix_kit_email_events this is the other potentially heavy step. No change required — the existing (event_type, occurred_at) index from V07 helps the multi-occurrence variant.
  • v137.ex:155down/0 intentionally does not reverse the dedup DELETEs or the aws_message_id backfill. This is correct and documented; no action — noting it only so a reviewer doesn't expect symmetry.

Verdict

Looks good to merge. Consider addressing the two MEDIUM notes (maintenance-window caveat in the moduledoc, and confirming occurred_at provenance for the open/click dedup claim) but neither blocks merge.

@ddon
ddon merged commit 0f62811 into BeamLabEU:mainJun 24, 2026
ddon pushed a commit that referenced this pull request Jun 24, 2026
… mode
PR #603's `all_sitemap_sources/0` iterated `all_modules/0`, relying on each
source's own `enabled?/0` to gate emission at generation time. That holds in
index mode, but flat mode (`do_generate_flat`) passes `force: true`, which
`Source.safe_collect/2` honors by bypassing `enabled?/0` — so a disabled module
that registered a `sitemap_sources/0` entry would still publish its URLs,
contradicting the PR's documented "disabled module emits nothing" guarantee.
Gate at the module level instead: iterate `enabled_modules/0` so a disabled
module contributes no source at all, in both modes. A source's own `enabled?/0`
remains a secondary gate in index mode. The intentional flat-mode force-collect
(commit c853379) is left intact for the sources that are in the list.
Also add post-merge review docs for #603 and #604.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request Jun 24, 2026
Releases the unpublished work on main since 1.7.164:
- #603 sitemap_sources/0 auto-registration (+ enabled-module gating fix)
- #604 V137 email event dedup indexes + aws_message_id backfill
- #605 notifications graceful handling + user/file comment-resource links
(+ cached default-link lookup)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@timujinne
timujinne deleted the emails-v137-dedup-indexes branch June 25, 2026 08:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@timujinne@ddon