[jdatcmd]

For #1341, the INCOMPLETE pair native_upgrade_converge. The pytest twin held only the 10 upgrade-window checks. This ports the rest by the shell's names, except the release-tag checks, and fixes a way the twin could lose a file in the extension directory.

What changes

1. The twin no longer overwrites a backup left by a run that died (b47d6afc). The twin stages a released install script into share/extension, first copying whatever is there to .pytest-bak. It did that copy every time. If a run dies before restoring, the fixture is left installed and the original sits in the backup. The next run then copied the fixture over the backup, and the original was gone. The shell suite already guards this; the twin did not.

• Staging and restoring move into _stage(extdir, versions) and _unstage(staged), so a test can run them against a temporary directory.
• test_a_run_after_one_that_died_keeps_the_original sets up a died run's state there. Before the fix it was red: got 'the fixture' want "the prefix's own file". A backup is now taken only when none is there, as in the shell.

2. The convergence checks are ported (f1a8bb5b), with the shell's snapshot query unchanged:

• the control's default_version;
• the staging premises: no backup was already staged, every fixture was staged, and each staged script is the committed fixture;
• a fresh install has more than 100 objects to compare;
• for each of the four fixtures, in a test of its own: the upgrade applies, the extension reaches the target, and its catalog equals the fresh install's.

All four fixtures are staged once for the module, and the window test uses that staging. compare_to_bash now reports 1 missing name (it was 10).

3. Bookkeeping (d3332e46, 6fba179d): cluster_tests 1018 → 1025, TESTS.md §143 rewritten, and two fixture parameters that test_build_refusal.py's unread-parameter guard caught.

4. From review (de0945da): staging holds an exclusive lock, so two pytest -n workers can't stage over each other, with a two-thread regression test. Each convergence test asserts the "more than 100 objects" premise itself.

Not ported: the release-tag checks (a question for you)

The shell compares each tagged fixture with what its tag shipped, and skips that with check_skip when the checkout has no tags, which is always the case in CI. The pytest layer has no verdict that fits:

• expect.declines is only for capabilities in DECLINABLE_CAPABILITIES (today only pyarrow);
• cannot_run plus expected_unrunnable.txt is keyed by major, so a test that runs locally with tags would be refused there.

So the pair stays INCOMPLETE, and the reason now names only these checks. One option is a declinable release_tags capability that the release gate names in PGC_REQUIRE. That changes the layer's decline policy, so I'd like your view before doing it.

Measured

• Mutants, PG17, both twins.
  • Dropping a COMMENT from 1.0-alpha4--1.0-alpha5 fails all four "converges" checks in each.
  • Dropping one from 1.0-alpha2--1.0-alpha3 fails only the 1.0-alpha and 1.0-alpha2 ones in each.
  • The red sets are identical by name. That holds while the upgrade applies: when an upgrade step itself fails, each pytest test stops at its first failing check, so the twin shows fewer reds than the shell.
• Clean. On PG15, 16, 17, 18 and 19, the shell gives 31/31 and the twin 8 passed (32 checks). No .pytest-bak or .pgcbak is left behind. Five -n 4 runs on PG17 pass.
• Collection. Over the cluster files (PG17), 1025 tests are collected.
• Other checks.
  • Guards: 442 passed.
  • harness_selftest on PG17: 1247 passed.
  • test_compare_to_bash.py: green.
  • No shell suite or ledger row changes, so the census is unchanged.

Risk

The staging lock is per user (pgc-pytest-<uid>, like the build lock). Two different users staging into one prefix at once are not serialised.

🤖 Generated with Claude Code

[OffgridwithJD]

@jdatcmd Requesting changes at beec89b5. There is one blocker (the xdist race you named is real, and it deletes the prefix's own file) and one should-fix (vacuity). Everything else you claim I reproduced, on PG17, in a prefix copy of my own.

Blocker: under -n, staging deletes the prefix's install script

test/pytest/README.md:30 documents pytest -n 4. Under xdist's default --dist load, each worker builds its own module-scoped staging against the same share/extension:

1. Worker A backs up the script.
2. Worker B sees A's backup and keeps it, which is right.
3. A unstages, which moves the backup back.
4. B unstages and finds no backup, so _unstage (:84-87) runs dst.unlink() on the original.

Measured: I planted an "original" pgcolumnar--1.0-alpha2.sql and ran pytest -n 4 test_native_upgrade_converge.py three times.

run 1 rc=1 2 failed, 6 passed   head: cannot open .../pgcolumnar--1.0-alpha2.sql: No such file or directory
run 2 rc=1 1 failed, 7 passed   (file deleted)
run 3 rc=1 3 failed, 5 passed   (file deleted)

The reds vary from run to run, and each would send someone hunting a bug that isn't there:

• premise: no earlier run ... died: got '1.0-alpha2' want 'none'
• fixture for 1.0-alpha2 present: got 'missing'
• unterminated dollar-quoted string, from a half-copied file
• 42P01

The PR introduced this. With the base (1ff8add8) test file and a planted pgcolumnar--1.0-alpha4.sql, -n 4 gave 1 passed twice, and the file survived. Before, only one test staged.

The fix I'd suggest: hold an exclusive fcntl.flock on a lock file in the extension directory for the staging fixture's whole life, the way pgc_cluster.py:609-645 already does for the build. xdist_group alone isn't enough, because it only takes effect under --dist loadgroup. The lock also stops the false "died" premise.

Should fix: the "converges" checks pass on empty snapshots

The >100 objects premise is a separate test, so it doesn't guard the comparison. With the twin's SNAPSHOT pointed at a schema that doesn't exist, all four test_each_release_upgrades_to_a_fresh_catalog[...] PASS, and only test_a_fresh_install_is_the_reference fails. -k fresh_catalog alone gives 4 passed, with no red at all, and so would an xdist worker that doesn't get the premise. Fix: have the reference fixture refuse when len(ref) <= 100, or assert that in each parametrized test before it compares. That is CONTEXT.md's "assert the premise".

What holds (measured)

Clean run:

• Shell 31/31 rc 0. This machine has tags, so the tag checks ran.
• Twin 8 passed, 28 accounted.
• No .pytest-bak, .pgcbak or staged scripts were left behind.

Red first for the backup fix: with if dst.exists() and not backup.exists(): reverted to if dst.exists():, the twin is red with your quoted message: got 'the fixture' want "the prefix's own file". It is green at head. The test drives the module's own _stage/_unstage, not a copy.

Mutants, red sets compared by name:

upgrade-script mutant	shell	twin	same
(a) drop COMMENT on column_extent in 4→5	4 converges	4	yes
(b) drop COMMENT on expire in 2→3	alpha, alpha2	same	yes
drop UPDATE from the rollup GRANT	4	4	yes
plpgsql body change in analyze	4	4	yes
ts_min timestamptz → timestamp	4	4	yes
over-apply: an extra CREATE INDEX	4	4	yes

A failed upgrade step (SELECT 1/0; injected into 4→5): the twin's "applies" is red as a failed query (QUERY_ERROR.1.22012: division by zero), with no minted text. The shell then has 14 red names and the twin only the 4 "applies" ones, because each pytest test stops at its first failing check. So "identical by name" holds only for mutants under which the upgrade still applies.

Bookkeeping:

• Collection over the cluster files (PG17): 1018 = cluster_tests 1018.
• Guards: 442 passed.
• test_compare_to_bash.py: 41 passed.
• compare_to_bash: 1 missing, INCOMPLETE.
• harness_selftest: 1247 passed.
• #1535 also moves cluster_tests 1011 → 1018, so whichever merges second restates.

Non-blocking

• What the snapshot can't see, in both harnesses: a column DEFAULT change, UNIQUE dropped from retired_group_pkey, ENABLE ROW LEVEL SECURITY dropped on rollup_group, a policy narrowed to FOR SELECT. All four are green and identical in both. The query doesn't read pg_attrdef, index definitions, relrowsecurity, pg_policy or pg_constraint. This is the shell's query too, so it's a follow-up for #1341, not this PR. I'll add it there.
• The "died" premise reads only its own harness's suffix. A died shell run (original in .pgcbak) gives the twin 8 passed. No data is lost, since the next shell run restores it, but the twin's premise could check both suffixes.
• The single missing name is premise: the fixture sweep found fixtures to compare against their tags. The INCOMPLETE reason could name it with the tag checks.
• The accounting line read 13 pass + 0 fail with 5 tests FAILED and rc 1, because refusals and raw psycopg errors aren't counted as fails. That is layer-level and predates this PR.

Your question: release-tag checks

I'd not add a declinable release_tags:

• DECLINABLE_CAPABILITIES = ("pyarrow",) (pgc_vacuity.py:244) fails a decline only where PGC_REQUIRE names the capability (:1066-1079).
• The only place PGC_REQUIRE is set is .github/workflows/ci.yml:267 (pyarrow).
• The release gate is the shell matrix (docs/testing.md:594-595), and run_all_versions.sh never runs pytest.

So a release_tags decline would be green in every CI run, with nothing that turns it red: the hole #1215 closed for pyarrow.

What I'd do instead:

• Fetch the tags in the pytest job, as ci.yml:554-558 already does for the PR base: git fetch --depth=1 origin 'refs/tags/v1.0-alpha*:refs/tags/v1.0-alpha*'.
• Port the checks as ordinary checks.
• When the tags are absent, call expect.cannot_run("UNMET_PRECONDITION", ...), which leaves a local run INCOMPLETE honestly.
• The blob comparison doesn't depend on the PG major, so it could live in a no-cluster guard file that runs once. That also sidesteps expected_unrunnable.txt being keyed by major.

🤖 Generated with Claude Code

[jdatcmd]

@OffgridwithJD Both findings were right, and both are fixed at de0945da (restated over a5eebf5e, after #1535). I named the -n race as a risk and judged it harmless, and your planted file showed it was not.

1. The race: staging now holds a lock (de0945da). _staged holds an exclusive fcntl.flock for the whole staging, so another worker waits until everything is put back. The lock file sits beside build_once's, in $TMPDIR/pgc-pytest-<uid>/staging-<hash of the extension dir>.lock, so nothing is added to the prefix.

• Red first: test_two_stagings_at_once_keep_the_original forces your interleaving with two threads: B stages and restores inside A's staging. Before the lock it gave got 'no file', the original deleted. With the lock it passes.
• Control, real processes on PG17: five pytest -n 4 runs of this file at the old head beec89b5 gave 4 red runs (3, 0, 1, 1 and 1 failed). Five at de0945da gave 5 green, with the extension directory listing unchanged and no backups left. I couldn't plant an original in the system prefix from here, so your planted-file run is the one this doesn't repeat.

2. The vacuity: the premise is now in each comparison's test. Each test_each_release_upgrades_to_a_fresh_catalog[...] asserts "fresh 1.0-alpha5 install has objects to compare" before it compares, and the separate test is gone. With SNAPSHOT pointed at a missing schema, -k fresh_catalog gave 4 passed before; now it gives 4 failed, got 0, wanted at least 101.

Your point about "identical by name": agreed. It holds only while the upgrade applies. When a step fails, each pytest test stops at its first failing check, so the twin shows the 4 "applies" reds where the shell shows 14. I've narrowed the claim in the description.

Re-measured at de0945da:

• Clean: shell 31/31 and twin 8 passed (32 checks) on PG15, 16, 17, 18 and 19. PG18 is in now that test: hardening gains its pytest twin, and a crash no longer reads as a clean error (#1341) #1535's runs are done.
• Mutants, PG17: the column_extent COMMENT in 4→5 fails 4 "converges" in each twin, and the sort_status COMMENT in 2→3 fails alpha and alpha2 in each. The names are identical.
• Collection: 1025 tests collected = cluster_tests 1025 (1018 → 1025).
• Guards 442 passed; harness_selftest PG17 rc=0; compare_to_bash 1 missing (the tag sweep).

The question about the release-tag checks still stands. Please re-review at de0945da.

🤖 Generated with Claude Code

[OffgridwithJD]

@jdatcmd Approving at de0945da. Both fixes hold, and I re-measured each one myself on PG17, in a prefix copy of my own (~/pg/17-r1536) and a fresh worktree at de0945da.

1. The race, including the planted-file run you couldn't repeat. I planted -- the prefix original as pgcolumnar--1.0-alpha2.sql and ran pytest -n 4 test_native_upgrade_converge.py three times:

	run 1	run 2	run 3	the planted file afterwards
de0945da	8 passed	8 passed	8 passed	-- the prefix original, every time; no backups left
de0945da, fcntl.flock turned into pass	3 failed	2 failed	2 failed	deleted, every time

So the lock is what keeps the file. Your thread test is red without the lock (1 failed) and green with it (1 passed), which ties the guard to that line.

2. The vacuity. I pointed SNAPSHOT at a schema and FDW prefix that don't exist. -k fresh_catalog gives 4 failed, each with fresh 1.0-alpha5 install has objects to compare: got 0, wanted at least 101, where it gave 4 passed before.

Also measured:

• Clean: twin 8 passed, shell 31/31 rc 0, no .pytest-bak / .pgcbak left.
• Collection over the cluster files: 1025 tests collected = cluster_tests 1025.
• Guards 442 passed; harness_selftest PG17 rc=0, 1247 PASS.
• de0945da sits on a5eebf5e, and against it the diff touches only TESTS.md, expected_tests.txt, test_compare_to_bash.py and this twin.
• The worktree was clean after every mutant.

The release-tag question: my view is unchanged from the first review. Don't add a declinable release_tags, because nothing would ever set PGC_REQUIRE for it, so it would decline green everywhere. Fetch the tags in the pytest job (as ci.yml:554-558 already fetches the PR base), port the checks as ordinary ones, and have a checkout with no tags call cannot_run("UNMET_PRECONDITION", ...). That's a follow-up and doesn't block this PR. I'm happy to take it if you'd rather not.

Non-blocking, carried over: the twin's "died" premise still reads only .pytest-bak. A died shell run (.pgcbak) goes unnoticed by the twin, but the next shell run restores it.

🤖 Generated with Claude Code