fix(db): index the two hot unindexed FK referencing columns - #934
Merged
Conversation
Postgres auto-indexes the REFERENCED side of a foreign key but never the REFERENCING side. Both of these are ON DELETE SET NULL, which Postgres implements as an internal per-row trigger that fires once for EVERY deleted parent row and runs `UPDATE ONLY child SET fk = NULL WHERE fk = $1`. Unindexed, that is a sequential scan of the whole child table — per deleted row, not per statement. Measured on staging against the structurally identical sibling webhook_events, which HAS such an index (idx_webhook_events_message_created, 026): webhook_subscriber_deliveries Seq Scan cost 93443.49 421,319 rows webhook_events Index Scan cost 8.44 454,487 rows ~11,000x, and webhook_events is the larger table. The consequences were not subtle, and none of them looked like a missing index: a 1000-message delete batch was ~88M page visits and aborted on a 60s statement_timeout; a plain agent delete blew the 60s proxy budget and returned HAProxy's HTML 504, which failed the conformance suite's response-schema gate because the spec documents application/json; and the same slow deletes pinned both slots of the per-account concurrent-destructive cap, so unrelated deletes 429'd. Four unrelated-looking symptoms, one cause. This is latent in production, not staging-specific: 025's own header says message_id is ON DELETE SET NULL so the 30-day retention purge can proceed — i.e. the design depends on exactly the path that was unindexed, and the cost grows with table size. 106 (webhook_subscriber_deliveries.message_id) is a plain index: 98.9% of rows carry a message_id, so a partial one would be within ~1% of the same size. 107 (messages.reviewed_by_user_id) is partial: only 0.79% of rows are non-null, making the partial index ~127x smaller. The FK check can still use it, because `col = $1` implies `col IS NOT NULL` — the same implication the planner already exploits for the partial idx_webhook_events_message_created, confirmed by EXPLAIN. Its path is rarer but worse per occurrence, and deleteAccount is itself in destructiveOps, so it runs under the same cap and timeout budget. Both use CREATE INDEX CONCURRENTLY with e2a:no-transaction, following 059 — which added an index to this same table for the same class of problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Postgres auto-indexes the referenced side of a foreign key (the PK) but never the referencing side. Both columns below are
ON DELETE SET NULL, which Postgres implements as an internal per-row trigger firing once for every deleted parent row:Unindexed, that's a sequential scan of the entire child table — per deleted row, not per statement.
The measurement
Same database, same moment, same predicate shape, same FK action. The only difference is the index:
webhook_subscriber_deliveriesSeq Scanwebhook_eventsidx_webhook_events_message_created(026)Index Scan~11,000× — and
webhook_eventsis the larger table.Why it mattered
A 1000-message delete batch was ~88M page visits. Every batch aborted on a 60s
statement_timeout. Downstream, one missing index produced four symptoms that looked unrelated:deleteAgentexceeded the 60s proxy budget → HTML 504application/json)maxConcurrentDestructive, so unrelated deletes 429'dLatent in production, not staging-specific.
025's own header saysmessage_idisON DELETE SET NULLso the 30-day retention purge can proceed — the design depends on exactly the path that was unindexed, and the cost scales with table size.The two migrations
106—webhook_subscriber_deliveries.message_id, plain index. 419,593 of 424,276 rows (98.9%) are non-null, so a partial index would be within ~1% of the same size.107—messages.reviewed_by_user_id, partial index. Only 4,333 of 549,579 rows (0.79%) are non-null, making partial ~127× smaller. The FK check still uses it becausecol = $1impliescol IS NOT NULL— the same implication the planner already exploits for the partialidx_webhook_events_message_created, confirmed byEXPLAIN. Rarer path, but worse per occurrence, anddeleteAccountis itself indestructiveOps, so it runs under the same cap and timeout budget — and "the account deletion timed out" is a poor answer on a data-rights path.Both use
CREATE INDEX CONCURRENTLYwithe2a:no-transaction, following 059, which added an index to this same table for this same class of problem. Both carry the invalid-index recovery note 059 established.Deliberately not in scope
The audit found 9 more single-column FKs with no leading index, all on tables that are currently empty or never-analyzed on staging:
I did not blanket-index these: indexes cost write throughput, and a seq scan of an empty table is free. But several are per-user tables that grow (
user_sessions,oauth_refresh_tokens), and I can't size them on prod from staging. Worth a follow-up audit against prod row counts, plus a schema test with an explicit allowlist so a new unindexed FK becomes a deliberate decision rather than an accident.Tests
go test ./internal/identity/ -run Migrat -shortgreen;go build ./...clean. The migration runner'se2a:no-transactionsingle-statement requirement is satisfied by both files.🤖 Generated with Claude Code