Conversation
… job RevokeExpiredDispatchesPlanScaleTest commits a production-shaped dispatches table (20k revoked history, 50 live), ANALYZEs it, captures the query RevokeExpiredDispatchesWorker's perform/1 issues through repo telemetry, and requires the default planner to read dispatches exactly once, by an index range scan on dispatches_expires_at_active_index with an Index Cond on expires_at. It runs in CI's scale job, not the default suite, because an ANALYZE writes pg_class reltuples in place and would skew other tests' plans (#932). The rows are deleted and the table re-ANALYZEd on exit. The migration comment, the worker test, the story and the epic 35 README point at it. Mutations (bin/mutate.sh, all exit 0): worker predicate made non-implying (coalesce); live fraction raised to 10 percent; the ANALYZE removed; the asserted index name swapped.
… it off the shared DB - The moduledoc says what the guard covers and where it stops, with a measurement: at this seed the default planner picks the partial index at 500 live of 20,500 (2.4 percent) and seq-scans at 1,000 of 21,000 (4.8 percent), because it multiplies the selectivities of revoked_at IS NULL and expires_at < $1 as if independent. - The seed adds a 20-row expired, unswept backlog, the steady-state shape. - PlanAssertions.assert_index_range_scan/4: exactly one scan of the relation, an Index or Index Only Scan on the named index, with a matching Index Cond; the plan is printed on every failure. The test's own EXPLAIN walker is gone. - on_exit is registered before seeding, so a failed seed is still removed. - The test refuses the shared loopctl_test database outside CI; the moduledoc gives the MIX_TEST_PARTITION commands. - setup :verify_on_exit!. - The migration comment says both captures are by hand and describes what CI asserts. Mutations (bin/mutate.sh, exit 0 each): worker predicate made non-implying; live fraction 10 percent; ANALYZE removed; index name swapped; Index Cond regex swapped.
…op the flip claim - The test's own cleanup never finished: four foreign keys reference dispatches, the self-reference among them unindexed, so deleting 20k rows scanned the table per row and on_exit timed out. Every local run leaked its seed, about 200k rows by the time it was found. The delete now runs with session_replication_role = replica inside its own transaction (the rows are inserted raw and nothing references them), and the slug is a UUID so a leaked tenant can never collide with the next run's. - The live-fraction flip recorded in round 1 was measured on that polluted table. On a clean table the index is sought at every live fraction tried, 50 up to 20,000 live rows (half the table), so the claim and the migration comment's pointer to it are gone. - The module is skipped unless CI=true, replacing the database-name guard that refused the main tree but passed a worktree's own test database, and the local recipe with it. - assert_index_range_scan builds on relation_scan_nodes (which now carries the raw node), also accepts a Bitmap Heap Scan driven by a single Bitmap Index Scan on the index, and refuses a relation pg_stat_user_tables shows was never ANALYZEd. - The migration comment no longer restates seed sizes and names the accepted scan types. Mutations (bin/mutate.sh, CI=true): worker predicate made non-implying, exit 0; index name swapped, exit 0; Index Cond regex swapped, exit 0; helper's scan-type check replaced, exit 0. Raising the live set to 2,000 and removing the ANALYZE both pass now, consistent with the measurement above: neither is load-bearing on a clean table.
Owner
Author
mkreyman
added a commit
that referenced
this pull request
Sep 28, 2026
* US-32.1 AC-32.1.2: guard the revoke sweep's plan in the scale job (replaces #934) RevokeExpiredDispatchesPlanScaleTest commits a dispatches table in the shape a running one settles into (a large revoked history, a small live set, a small expired backlog), rebuilds the partial index and VACUUM ANALYZEs, captures the query RevokeExpiredDispatchesWorker.perform/1 issues through repo telemetry, and requires the default planner to use dispatches_expires_at_active_index, through the existing PlanAssertions.assert_index_used/2. A new step in CI's scale job runs it. The seed is removed with foreign-key triggers off for the dispatches only (the self-reference parent_dispatch_id is unindexed, so a plain delete of 20k rows scanned the table per row and never finished), and the tenant is deleted after, with its cascades on. The migration comment, the worker test, AC-32.1.2 and the epic 35 README point at it and name the scans it accepts: an Index Scan, or a Bitmap Index Scan on the index. Mutations (bin/mutate.sh, all exit 0): worker predicate made non-implying (coalesce); range on expires_at replaced by a date cast; asserted index name swapped. * Review round 1 on #935: index the foreign keys into dispatches, roll the sweep back - Migration 20260928140000 adds concurrent partial indexes on the two unindexed foreign keys into dispatches, dispatches.parent_dispatch_id and story_acceptance_criteria.verified_by_dispatch_id. Without them every bulk delete of dispatches checked each row against a table scan. The test's cleanup is now a plain delete with every trigger on, and ForeignKeyIndexesTest fails the default suite when a foreign key into dispatches has no index. - The sweep runs inside a transaction the test rolls back, so a cross-tenant perform/1 cannot revoke another test's dispatches; inside it the test checks the write too: exactly the backlog is revoked. - The plan must use the partial index and must not use the tenant-leading composite (new PlanAssertions.refute_index_used/2), so a bitmap that reads the composite whole fails. - The captured query is the SELECT from dispatches, not one spelling of its predicate. - REINDEX CONCURRENTLY, the tenant from fixture(:tenant), the async exception named in the moduledoc, and both independent scale-job steps run unless the job was cancelled. - AC-32.1.2 and the epic 35 README name the scans accepted and the index ruled out. Mutations (bin/mutate.sh, exit 0 each): index dropped under the FK test; worker predicate made non-implying; asserted index swapped; composite refute pointed at the partial index; sweep's write removed. * Review round 2 on #935: exact index checks, reconciled migration, hour-long live rows - ForeignKeyIndexesTest requires a valid index whose leading columns are exactly the foreign key's, unconditional or conditioned only on NOT NULL; a tenant-led index or an unusable partial one no longer counts. - PlanAssertions.assert_only_index_used/3 replaces refute_index_used/2: the named index must exist and be the only index in the plan, and the relation's statistics must be current (assert_stats_current!/1, pg_class.reltuples within 10 percent of the rows). - The migration drops an index only when it exists invalid or misshapen (stale?/2, as in 20260919100000), so a re-run never drops a valid index on the hot table. - Live rows expire hours out, so a slow REINDEX cannot expire one mid-test; invalid _ccnew leftovers of an interrupted concurrent rebuild are dropped first. - The scale-job gates run after one another fails, but not after the database failed to migrate. - The seed is fixture(:dispatch_sweep_history); US-32.1's criteria are marked complete. Mutations (bin/mutate.sh, exit 0 each): index dropped, replaced by a wrong-predicate one, and by a tenant-led one under ForeignKeyIndexesTest; worker predicate made non-implying; asserted index renamed away; a second index put into the plan; REINDEX and VACUUM ANALYZE both removed (stale statistics); the sweep's write removed. * Review round 3 on #935: key-column and btree-only FK check, column-statistics freshness - ForeignKeyIndexesTest: leading KEY columns only (INCLUDE columns are sliced off at indnkeyatts), btree only, and a partial index counts only when its predicate is NOT NULL on one of the foreign key's own columns. - assert_only_index_used/4 requires the index to be valid and ON the named relation, and judges freshness by assert_column_stats_current!/2: pg_stats.null_frac of the named column within 0.02 of its actual NULL fraction. reltuples, which VACUUM and index builds also write, no longer stands in for column statistics. One quoted, public-qualified name is used throughout, and a missing relation or column raises the assertion, not a MatchError. - The migration's shape regex is anchored at CREATE INDEX, so a same-named UNIQUE index is rebuilt. - The backlog expires minutes ago, clear of app/database clock skew. - The test drops only invalid _ccnew/_ccold rebuild leftovers of its own index, on public.dispatches. - The scale-job gates also require the grant step to have succeeded. Mutations (exit 0 or caught, each): FK index dropped; replaced by a partial index on another column's NOT NULL; by a tenant_id index INCLUDE-ing the key; by a hash index; worker predicate made non-implying; a second index in the plan; the index checked against the wrong table; a different seed shape without ANALYZE (stale null_frac 0.0035 vs 0.20); the sweep's write removed.
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
1 participant
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.
Follow-up to #932, which removed the default suite's plan-choice assertion for AC-32.1.2. This PR brings that guarantee back where the planner's statistics can be controlled.
What it asserts:
RevokeExpiredDispatchesPlanScaleTest(:scale, run by a new step in CI'sscale-testjob) does the following:dispatchestable: 20k revoked history rows and 50 live rows. It then runsANALYZE.RevokeExpiredDispatchesWorker.perform/1actually issues, through repo telemetry.enable_seqscan = off) must readdispatchesexactly once, by an Index Scan ondispatches_expires_at_active_indexwith an Index Cond onexpires_at.Why it runs in the scale job:
ANALYZEstill writespg_class.reltuples/relpagesin place. Its statistics outlive the rollback and skew every other test's plans in the shared database. That is the flake class Two gate flakes: a whole-VM log assertion, and a plan pinned to one of two usable indexes #932 removed.Seed shape: it mirrors a table that has been running for a while: mostly revoked history, a small live set, and a small expired backlog. On a clean table the index is used at every live fraction tried, from 50 up to 20,000 live rows (half the table).
Docs: the migration comment, the worker test's moduledoc, AC-32.1.2 and the epic 35 README now point at this test.
Evidence:
mutate.sh), all exit 0:coalesce)ANALYZEremovedSCALE_TESTS=true.Review round 1 found 7 issues. All 7 are fixed:
PlanAssertions.assert_index_range_scan/4replaces the test's own plan walker.on_exitis registered before seeding.setup :verify_on_exit!is added.Review round 2 found 9 issues. All 9 are fixed:
CI=true.relation_scan_nodes, accepts a bitmap scan driven by the index, and refuses a table that was never ANALYZEd.dispatches, one unindexed, so deleting 20k rows scanned the table per row). About 200k rows had leaked. The cleanup now deletes with FK triggers off inside its own transaction, and it is measured to leave nothing.Mutations (
mutate.sh,CI=true), each exit 0:Raising the live set and removing the
ANALYZEboth pass now, which matches the measurement: neither is load-bearing on a clean table.Review round 3 is running.