US-32.1 AC-32.1.2: guard the revoke sweep's plan in the scale job - #935
Merged
Merged
Conversation
…places #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.
…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.
…r-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.
…atistics 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
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.
Replaces #934. #934's third review round still found material defects, so under the review ceiling this is a rewrite rather than a fourth round. It follows up #932, which removed the default suite's plan-choice assertion for AC-32.1.2.
What it asserts:
RevokeExpiredDispatchesPlanScaleTest(:scale, run by a new step in CI'sscale-testjob):dispatchestable in the shape a running one settles into: a large revoked history, a small live set, and a small expired backlog.VACUUM ANALYZE.RevokeExpiredDispatchesWorker.perform/1actually issues, through repo telemetry.dispatches_expires_at_active_index, through the existingPlanAssertions.assert_index_used/2.The query reads one table, so a plan that uses the index cannot also Seq Scan it. With 50 live rows the planner uses an Index Scan. With a larger live set it uses a Bitmap Index Scan. Both seek the index. Measured on a clean table, the index is used from 50 live rows up to half the table.
What #934 got wrong, and this doesn't:
parent_dispatch_idmakes a plain delete of 20k dispatches scan the table once per row, so every local run leaked its seed. About 200k rows piled up, and that skewed the plan measurements US-32.1 AC-32.1.2: guard the revoke sweep's natural plan in the scale job #934 recorded.REINDEXon the index andVACUUM ANALYZE, so every run plans against the state CI's fresh database has.PlanAssertionshelper, no CI-only skip, and no database-name guard. It is tagged:scalelike every other scale test.Evidence:
mutate.sh, all exit 0):coalesce)expires_atreplaced by a date castReview round 1 found 10 issues. All 10 are fixed:
Index at the cause: the cleanup worked around an unindexed foreign key. Migration
20260928140000now adds concurrent partial indexes on both unindexed foreign keys intodispatches:dispatches.parent_dispatch_idstory_acceptance_criteria.verified_by_dispatch_idEvery bulk delete of dispatches used to scan the referencing table once per row. The test's cleanup is now a plain delete with triggers on.
ForeignKeyIndexesTestfails the default suite whenever a foreign key intodispatcheshas no index.Sweep rolled back: the sweep is cross-tenant, so it now runs in a transaction the test rolls back. Inside that transaction the test checks that exactly the backlog was revoked.
Composite ruled out:
PlanAssertions.refute_index_used/2rules out the tenant-leading composite index, so a bitmap scan that reads it whole fails.Smaller fixes:
SELECT ... FROM "dispatches", not by one spelling of its predicate;REINDEX CONCURRENTLY;fixture(:tenant);Mutations (
mutate.sh, each exit 0):ForeignKeyIndexesTestGate: 12003 tests, 0 failures. The first attempt hit one
57014inKnowledgeAutoExtractTest: an insert into a table that referencestenantswas cancelled. That is the same unconfirmed lock-wait flake recorded in #932. The retry passed.Review round 2 found 9 issues. All 9 are fixed:
stale?/2, so a re-run never drops a valid index.Review round 3, the last, found 10 issues. All 10 are fixed. None was in the migration or in what the guard catches; all were about how precise the helper checks are:
pg_stats.null_fracagainst the actual NULL fraction.Mutations (each caught):
ANALYZE: stale null_frac 0.0035 against an actual 0.20Gate: 12003 tests, 0 failures.