monitor: run pg_regress tests serially via a real schedule file#1151
Merged
Conversation
pg_regress runs an unscheduled (bare) REGRESS list as a single parallel group. None of src/monitor/'s regression tests were written to tolerate that: they all register nodes via pgautofailover.register_node() against the one pgautofailover.node table (and its one shared nodeid sequence) in the whole contrib_regression database, and their expected/*.out files hardcode small literal node ids on the assumption that they have that sequence -- and the table -- to themselves. dummy_update.sql and upgrade.sql additionally run their own CREATE/ALTER/DROP EXTENSION pgautofailover against the single shared extension object, a second, separate way for two tests to corrupt each other if run concurrently. Observed in CI as sporadic "not ok - upgrade" and "not ok - lock_and_fetch_migration" failures (most recently on PR #1149's "Build run image (PG18)" job), depending on which pair of tests the scheduler happened to interleave that run. Confirmed locally: running the old flat REGRESS list surfaced a *second*, previously-undiagnosed instance of the same class of bug -- monitor/workers/etc. leaking node-id and formation rows into each other when run in a naive "everything but the three extension-owning tests" parallel group. Fix: add src/monitor/regress_schedule, a real pg_regress --schedule file that puts every test in the original REGRESS list on its own serial "test:" line, in dependency order (create_extension first, then the functional tests, then dummy_update -> drop_extension -> upgrade last). Wire it in via REGRESS_OPTS += --schedule=..., and override installcheck to invoke pg_regress without also passing the bare $(REGRESS) list -- pg_regress treats trailing bare test-name arguments as extra tests run one at a time *after* the schedule finishes (pg_regress.c's extra_tests/run_single_test()), so passing both would silently re-run every test a second time. REGRESS itself is left in place, still listing every test: PGXS's `ifdef REGRESS` gates the installcheck/clean recipes entirely (verified: an empty-valued REGRESS makes `ifdef REGRESS` false), and it still documents the full test set. Verified: `make build-pg18` (the exact Dockerfile/pg_virtualenv path CI uses) -- all 12 REGRESS tests and all 6 ISOLATION tests pass, in the expected serialized order.
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.
Problem
src/monitor'spg_regresssuite is flaky in CI: sporadicnot ok - upgradeornot ok - lock_and_fetch_migrationfailures, most recently on PR #1149's "Build run image (PG18)" job.Root cause
src/monitor/Makefile'sREGRESSvariable is a flat, space-separated list. Passed topg_regresswith no--schedule, that whole list runs as one parallel group — but none of these tests were written to tolerate that:pgautofailover.register_node()against the singlepgautofailover.nodetable (and its one sharednodeidsequence) in the sharedcontrib_regressiondatabase. Theirexpected/*.outfiles hardcode small literal node ids (e.g.assigned_node_id | 2) on the assumption that they have that sequence, and the table, to themselves. Run two of them concurrently and one test's inserts bump the other's node ids and leak extra rows into the other'sselect * from pgautofailover.nodeoutput.dummy_update.sqlandupgrade.sqladditionally each run their ownCREATE/ALTER/DROP EXTENSION pgautofailoveragainst the single shared extension object — a second, independent way for two tests to corrupt each other if run at once.Confirmed locally: my first attempt at a fix only isolated the three extension-owning tests (
dummy_update,drop_extension,upgrade) into their own serial steps and left the other 8 in one parallel batch, on the assumption that they only read the extension. Running that surfaced the node-id variant of the race directly — proof it isn't limited to the extension-lifecycle tests.Fix
Add
src/monitor/regress_schedule, a realpg_regress --schedulefile that puts every test from the originalREGRESSlist on its own serialtest:line, in dependency order (create_extensionfirst — everything else needs the extension to exist — then the functional tests, thendummy_update→drop_extension→upgradelast, matching the extension-lifecycle dependency between them).src/monitor/Makefile:REGRESS_OPTS += --schedule=$(SRC_DIR)regress_scheduleinstallcheckis overridden to invokepg_regresswithout also passing the bare$(REGRESS)list.pg_regresstreats trailing bare test-name arguments as extra tests, run one at a time after the schedule finishes (pg_regress.c'sextra_tests/run_single_test()), so passing both the schedule and the bare list would silently re-run every test a second time against a since-mutated database — not merely redundant, but wrong.REGRESSitself is left in place, still listing every test: PGXS'sifdef REGRESSgates theinstallcheck/cleanrecipes entirely (verified locally — an empty-valuedREGRESSmakesifdef REGRESSfalse and silently drops the whole recipe), and it still documents the full test set for readers.checkis untouched: under this PGXS setupmake checkalready prints "not supported, use install + installcheck instead" (no functioning temp-instance path exists here), so there was nothing working to preserve.Verified
make build-pg18— the exact Dockerfile/pg_virtualenvpath CI uses for the "Build run image" jobs:All 12
REGRESStests and all 6ISOLATIONtests pass, correctly serialized in the intended order.Cost
Losing intra-suite parallelism adds a small amount of wall-clock time (each test is ~10-30ms; going from one parallel batch to 12 serial steps costs roughly a couple hundred milliseconds total) in exchange for a suite that was never actually safe to parallelize.