Actually run test/extension_upgrade.sh (#396) - #400
Conversation
The suite landed in commandprompt#389 with pg_upgrade's exemption from the registration check and without pg_upgrade's invocation, so nothing ran it. Not the matrix, not CI, not the Makefile. It ran only when a human typed its name. That is the gap commandprompt#257 existed to close, and it is worse here than there. The break this suite catches is invisible to a build and to every suite that creates the extension from scratch, so a guard nobody runs leaves exactly the failure it was written for undetected. It is invoked now from run_all_versions.sh under PGC_RUN_UPGRADE=1, the same switch and the same terms as pg_upgrade. One major is enough, so it runs once against the first config rather than per pair. Asking for the gate and getting nothing is a failure rather than a quiet pass, matching the pair check above it. Second problem from the same report: it could not run in the documented container loop at all, because that loop copies the tree without .git and the ref form needs tags. The second argument now takes either a git ref or a path to an already-checked-out old source tree. A tree that is not a git checkout now says so, and says what to pass instead, rather than failing on a missing tag. docs/testing.md gains a section for it, since the cross-major upgrade had one and this is a different upgrade with a different failure.
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Ran it in the container, which is the environment #396 was about. The suite is fixed; the runner is not.
Half of this works and is verified. The other half does not reach the place the issue
named.
What works
Form B, the directory form, passes in the container with no .git anywhere --
the case I could not run when reviewing #389:
$ ls -d /root/pgc400/.git -> absent (tree under test)
$ ls -d /root/oldsrc400/.git -> absent (v1.0-alpha source)
== extension_upgrade: PG18, old /root/oldsrc400
old source: directory /root/oldsrc400
old install: 1000 rows, extversion 1.0-dev
ok ALTER EXTENSION UPDATE -> 1.0-alpha
PASS existing rows still readable: 1000
PASS insert into an existing table: 1001
PASS new columnar table creatable: 3
PASS access method still bound: pgcolumnar
PASS maintenance function callable
PASS every C function's link name is namespaced
== extension_upgrade: PASS
The error text for the ref form on a non-git tree is genuinely helpful: it names the
container loop and prints the exact command to use instead.
What does not
Through the runner, in the container, the gate FAILS. Full run,
PGC_RUN_UPGRADE=1 test/run_all_versions.sh /usr/local/pgsql/bin/pg_config:
FAIL extension_upgrade PG18
== extension_upgrade: PG18, old v1.0-alpha
test/extension_upgrade.sh <pg_config> /path/to/old/source
===================== MATRIX SUMMARY ============================
PASS PG18 harness_selftest=PASS docs_style=PASS ... (all 112 suites)
FAIL extension_upgrade PG18
Every one of the 112 suites passed. The only real red is the new gate. (FAIL upgrade (no runnable pair) in that summary is mine -- I passed a single config and the
cross-major gate needs two. Correct behaviour.)
The cause is line 545:
if bash "$SRCDIR/test/extension_upgrade.sh" "$_ex" >"$_exlog" 2>&1; thenNo second argument, so the runner always takes the ref form, and $SRCDIR in
the documented dev loop has no .git. I checked for a way to supply the directory and
there is none: no PGC_* variable reaches that call. So the directory form you added
is unreachable from the gate, and the gate is unusable in the container -- which is
exactly the environment #396 called out.
Why this is worth another round rather than a note
It creates a new source of false red in the documented loop, and this project's own
runner argues against precisely that:
a gate that goes red for reasons unrelated to the change teaches its readers to
discount red, which is worse than not running it
A developer following docs/testing.md who opts into PGC_RUN_UPGRADE=1 gets a
failure that is not a product defect. That is the same lesson as #396 wearing different
clothes: the first time the guard did not run, this time it runs and lies.
Smallest fix I can see
Either of these, and the second is cheap:
- An env var the runner passes through --
PGC_UPGRADE_OLD_SRC=/pathappended as
the second argument when set. Two lines, and it makes the directory form reachable. - SKIP rather than FAIL when
$SRCDIRis not a git checkout, with the reason
printed. There is precedent in this file:PGC_SKIP_TIMINGprintsSKIPand
returns 0 rather than failing.
I would do both: (1) so it can run in the container, (2) so it does not lie when it
cannot.
Everything else about the PR is right, including running it once against the first
config rather than per pair, and treating "asked for the gate, got nothing" as a
failure. Re-run my container check after the change and I will merge.
…an lie jdatcmd ran the previous commit in the container, which is the environment commandprompt#396 was about, and the gate went red there. The directory form was reachable by a person and not by the runner: line 545 passed no second argument, so the runner always took the ref form, and the documented container loop has no .git. So it created a new source of false red in the loop the docs tell people to use. This runner's own comment argues against exactly that, saying a gate that goes red for reasons unrelated to the change teaches its readers to discount red. That is commandprompt#396 wearing different clothes. The first time the guard did not run. This time it ran and lied. Two changes, both suggested in the review. PGC_UPGRADE_OLD_SRC names a directory holding the old source, and the runner appends it when set. That makes the directory form reachable from the gate. Absent any named old source, a tree with no .git now reports SKIP and exits 2 rather than failing. The runner reports SKIP and records SKIP in the summary, following PGC_SKIP_TIMING above it. Skip and fail are different answers and the difference is who asked for what. A caller who names an old source that cannot be honoured gets a failure. A caller who names nothing, in a tree that cannot supply one, gets a skip. The skip is loud, prints its reason, and is never reported as a pass, because a gate reporting green having run nothing is the defect this suite was written for.
Verifying the exit codes turned up a misleading message. A PGC_UPGRADE_OLD_SRC that names a path which does not exist fell through to the ref branch, so the user got an error about git checkouts and refs in answer to a question about a directory. The exit code was right and the message pointed at the wrong thing. A value containing a slash is now treated as a path, and a missing one says that, rather than being reinterpreted as a ref. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Closes #396.
You were right, and the way you framed it is the part that stung: the suite had
pg_upgrade's exemption without pg_upgrade's invocation. That is the same shape as
the bug the suite exists to catch, one level up. It reported nothing and nothing ran it.
What changed
It runs. Invoked from
run_all_versions.shunderPGC_RUN_UPGRADE=1, the same switchand the same terms as
pg_upgrade.sh. One major is enough, so it runs once against thefirst config rather than per pair. Asking for the gate and getting nothing is a failure
rather than a quiet pass, matching the pair check above it.
It works without
.git. The second argument now takes either a git ref or a path toan already-checked-out old source tree. A tree that is not a git checkout says so, and
says what to pass instead, rather than failing on a missing tag as it did when you
reviewed #389.
docs/testing.mdgains a section for it, beside the cross-major one, since it is adifferent upgrade with a different failure.
Proof
Inspection is not proof for this one, given that the whole complaint is that nothing ran
it. All three entry points, on PG18:
And through the runner, which is the actual claim:
It appears in the live output and in the matrix summary, next to the gate it was modelled
on.
One thing I did not do
I did not put it in CI.
PGC_RUN_UPGRADEis opt-in because of cost, and this suite buildsthe extension twice, so adding it to the per-PR gate is a decision about CI minutes rather
than about coverage. Say the word if you would rather it ran nightly, which seems like the
natural home given
nightly.ymlalready exists.