From d33e579190e222f7e6978f50e9b07597a802c13c Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Wed, 22 Jul 2026 12:13:58 +0200 Subject: [PATCH] monitor: run pg_regress tests serially via a real schedule file 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. --- src/monitor/Makefile | 14 ++++++++++++ src/monitor/regress_schedule | 44 ++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 src/monitor/regress_schedule diff --git a/src/monitor/Makefile b/src/monitor/Makefile index 5d7a491c4..7e0022fd3 100644 --- a/src/monitor/Makefile +++ b/src/monitor/Makefile @@ -57,6 +57,20 @@ override CFLAGS := $(DEFAULT_CFLAGS) $(CFLAGS) include $(PGXS) +# See regress_schedule for why a custom schedule is needed instead of +# PGXS's default behavior (the whole $(REGRESS) list as one parallel group). +REGRESS_OPTS += --schedule=$(SRC_DIR)regress_schedule + +# Override PGXS's installcheck: with --schedule in REGRESS_OPTS, pg_regress +# runs the schedule's groups first and then re-runs every bare $(REGRESS) +# name again, one at a time, as extra tests appended after the schedule +# (pg_regress.c's extra_tests/run_single_test()). So $(REGRESS) must not +# also be passed on the command line here -- the schedule file is the only +# source of truth for which tests run and how they're grouped. +installcheck: submake $(REGRESS_PREP) + $(pg_regress_installcheck) $(REGRESS_OPTS) + $(pg_isolation_regress_installcheck) $(ISOLATION_OPTS) $(ISOLATION) + ifdef USE_SECURITY_FLAGS # Flags taken from: https://liquid.microsoft.com/Web/Object/Read/ms.security/Requirements/Microsoft.Security.SystemsADM.10203#guide SECURITY_BITCODE_CFLAGS=-fsanitize=safe-stack -fstack-protector-strong -flto -fPIC -Wformat -Wformat-security -Werror=format-security diff --git a/src/monitor/regress_schedule b/src/monitor/regress_schedule new file mode 100644 index 000000000..1f9952ffe --- /dev/null +++ b/src/monitor/regress_schedule @@ -0,0 +1,44 @@ +# Regression test schedule for the pgautofailover monitor extension. +# +# pg_regress runs an unscheduled (bare) REGRESS list as a single parallel +# group. None of these 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 (e.g. "assigned_node_id | 2") on the +# assumption that they have that sequence -- and the table -- to +# themselves. Running any two of them concurrently lets one test's inserts +# bump the other's node ids and leak extra rows into the other's +# "select * from pgautofailover.node" style queries. dummy_update.sql and +# upgrade.sql additionally run their own CREATE/ALTER/DROP EXTENSION +# pgautofailover against the single shared extension object, which is its +# own, separate way for two tests to corrupt each other if run at once. +# +# Net effect: every test here needs the whole database to itself, in the +# exact dependency order below. This is why each gets its own "test:" +# line (pg_regress runs schedule lines strictly one after another, so a +# solo line is a full serial step). Observed in CI as sporadic +# "not ok - upgrade" / "not ok - lock_and_fetch_migration" / node-id +# mismatches in other tests, depending on which pair the scheduler +# happened to interleave that run. +# +# create_extension must run first: every other test needs the extension +# to exist. dummy_update, drop_extension, and upgrade must run last, in +# this exact order: dummy_update leaves the extension at a fake "dummy" +# version, drop_extension removes it outright, and upgrade does its own +# from-scratch CREATE EXTENSION '1.0' -> ALTER EXTENSION UPDATE TO ... -> +# DROP EXTENSION cycle that assumes it owns the extension's version state +# exclusively. + +test: create_extension +test: monitor +test: workers +test: node_active_protocol +test: guard_data_loss +test: fast_forward +test: drop_node +test: stale_primary_report +test: lock_and_fetch_migration +test: dummy_update +test: drop_extension +test: upgrade