ci: run the extension-upgrade guard, and stop the coverage runner failing on it (#741)

[jdatcmd]

Closes #741.

test/extension_upgrade.sh guards the break that is invisible until a user
upgrades. In CI it had never verified an upgrade. Two separate defects, fixed
separately.

1. Nothing ran it

No workflow set PGC_RUN_UPGRADE, so the block at run_all_versions.sh:855-900
never executed and the suite was absent from every suites (PG N) job.

Added a nightly upgrade-guard job on PG18. Three details are deliberate:

• fetch-depth: 0, because actions/checkout fetches no tags by default and the
  suite's ref fallback then cannot resolve. That is the whole of defect 2's
  symptom.
• The previous release is asserted to resolve before any expensive setup. A
  tagless checkout must be a loud red, never a skip, since "the old source was
  not available" is exactly the state that made this guard look like it was
  running when it was not.
• The old source is materialised with git archive into a plain tree and passed
  explicitly, i.e. the suite's documented directory form. The suite runs
  under sudo (its runpg is an unconditional runuser), and a root
  git clone of a workspace owned by the runner user trips git's
  dubious-ownership refusal. Passing it explicitly also sets the suite's
  EXPLICIT flag, so its "not a git checkout" SKIP branch is unreachable from
  here. Exit 2 is a failure of this job rather than a waiver, for the same
  reason.

2. The one path that did reach it could not work

run_coverage.sh discovers every test/*.sh not named in not_a_suite(), which
excluded pg_upgrade but not extension_upgrade, although run_all_versions
gates both in one block for one reason. It ran with no old source, and since
run_coverage maps only rc = 66 to SKIP, the environment shortfall was counted
as a failed suite. Hence 1 failed ( extension_upgrade) every night.

Both upgrade suites are now excluded together. Deciding to run an opt-in guard is
run_all_versions.sh's job, not the coverage runner's.

The comment was already false

There are two copies of that list, and run_coverage.sh's comment said it was
"Kept in step with harness_selftest.sh's list". It was not: the selftest copy in
040 already named extension_upgrade. A comment claiming two things agree is
not a mechanism that makes them agree, so test/selftest/220 now asserts it over
the whole population of test files (227 compared), in both directions.

The gated suite list is derived from the PGC_RUN_UPGRADE block rather than
naming the two suites, so a third one added there is covered the day it is added.

Verified end to end, not by inspection

On PG17 in the container, one variable between the arms:

arm	change	result
ARM	tagless checkout, invoked as run_coverage.sh does	rc 1, v1.0-alpha is not present
CONTROL	same tree, old source materialised by git archive	rc 0

The ARM reproduces the nightly's failure exactly. The CONTROL upgrades 1.0-dev to
1.0-alpha2 with 7 PASS lines including the 149-object catalog convergence check,
so the upgrade path itself is sound today and the red really was the missing old
source.

Removal proofs, each in isolation

mutation	check that goes red
revert not_a_suite	"every PGC_RUN_UPGRADE-gated suite is excluded", got [extension_upgrade]
remove the workflow job	"CI runs the extension-upgrade guard somewhere"
mention the suite only in a comment in a workflow	still red, so a commented-out line cannot satisfy it
drift either not_a_suite copy	agreement check red, naming which copy drifted

And the instrument is shown to say both things: not_a_suite is asserted to
REFUSE an ordinary suite, so its acceptance of these two means something.

harness_selftest: 119 checks, PASSED.

What this does not do

• It does not fix The nightly coverage report has never captured any coverage (lcov capture produced nothing) #740, and the nightly stays red until that lands. Once
  lcov succeeds, run_coverage reaches its real verdict at line 142. The two
  are a pair, and neither alone reaches green.
• It does not teach run_coverage.sh the exit-2 SKIP contract that
  run_all_versions.sh honours. With both upgrade suites excluded, no discovered
  suite exits 2 today, so that would be untested handling for a case that cannot
  currently arise. Worth doing if a suite ever needs it.
• It does not run the pg_upgrade cross-major pair matrix, which is the other
  half of PGC_RUN_UPGRADE and a much larger job.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EYZNZ2hPeFxTNQNeCmce4E

[OffgridwithJD]

Reviewed as ChronicallyJD on the merged tree (real 3-way merge of up/main 43ee2b7

• pr742 77edf5e, merges CLEAN), pg17a assert, container. This closes the issue I filed,
  so I verified the claims rather than reading them.

Every claim in the description reproduces

I re-ran all four removal proofs independently, each in isolation, restoring between:

mutation	result
revert not_a_suite	RED — got [extension_upgrade] want [], plus the agreement check naming coverage=run,selftest=excl
remove the workflow job	RED — CI runs the extension-upgrade guard somewhere: got [no]
comment out the invocation only	RED — a mention genuinely does not satisfy it
drift the selftest's not_a_suite copy	RED — coverage=excl,selftest=run, naming which copy drifted
restored	GREEN, 119 checks PASSED

The headline numbers are also as stated: harness_selftest 119 checks PASSED, the
agreement check compares 227 test files with 0 disagreements, and the derived list
really does come out as extension_upgrade pg_upgrade — so the derivation is doing the
work, not a hand-written pair.

The ^[^#]* guard on the CI-invocation check deserves specific credit: I tried to defeat
it with a commented-out invocation and it stayed red, which is the failure mode that
would have made this whole guard theatre.

Finding 1 — the rc = 2 branch is unreachable (low; the gate still holds)

In Run the extension-upgrade guard:

set -uo pipefail
sudo -E env "PATH=$PATH" bash test/extension_upgrade.sh ... 
rc=$?
if [ "$rc" = 2 ]; then echo "::error::..."; fi
exit "$rc"

GitHub runs a run: step as bash -e {0}. set -uo pipefail does not cancel that
-e — only set +e would. So on any non-zero exit the step aborts at the suite line,
and rc=$?, the ::error:: annotation and exit "$rc" never execute.

Proven, with a control:

$ bash -e -c 'set -uo pipefail; false; rc=$?; echo "REACHED rc=$rc"; exit "$rc"'
(no output)                     step exit=1
$ bash -e -c 'set +e;           false; rc=$?; echo "REACHED rc=$rc"; exit "$rc"'
REACHED rc=1                    step exit=1

The safety property you describe still holds — exit 2 aborts the step, the job fails,
and it is not a waiver. What is lost is only the diagnostic: the one case the comment
singles out is the one case that can never announce itself. set +e on that line restores
the intent. I flag it because the branch is written as deliberate, so a later reader will
believe it runs.

Finding 2 — the PR carries no CHANGELOG entry

.github/workflows/nightly.yml, test/run_coverage.sh, test/selftest/220-*.sh — and
nothing under CHANGELOG.md or docs/. jd's standing rule is that no PR ships without
its CHANGELOG and docs in the same PR, and the four most recent merges (#736, #737, #738,
#739) each carried a CHANGELOG entry. docs_style does not check for this, so CI will not
catch it.

Finding 3 — this PR makes a sentence in docs/testing.md false

docs/testing.md:303-306 enumerates what the nightly contains:

Nightly at 06:00 UTC (.github/workflows/nightly.yml): the full packaged suite
matrix across 15 to 18 on x86_64, and the current major on aarch64. The nightly run also
includes the ASAN and UBSAN sanitizer gate, against an instrumented PostgreSQL, and the
coverage report.

This PR adds a fourth nightly job and leaves that enumeration untouched, so it
under-describes the nightly from the moment it merges. It is an enumeration, so it goes
stale by addition — the same way #671's sentences did.

One operational point, which I think is the important one

This lands the guard into a workflow that has been red every night for 25 nights and
that nobody was reading — which is the exact condition that let #741 hide in the first
place. You say so plainly under "What this does not do", and I agree with the analysis.
The consequence worth making explicit: while the nightly is red for #740, this job
flipping pass -> fail produces no new observable signal at the workflow level. The
guard runs, but its verdict still arrives inside a chronic red.

That is an argument for sequencing #740 first or landing the two together, not against
this change.

Verdict

Not approving yet, on Finding 2 (the CHANGELOG rule is jd's, not mine to waive) and
because CI is still in flight. Findings 1 and 3 are small and I would not block on either
alone. The mechanism, the selftest and every removal proof are sound, and I could not
break the instrument where I tried. Ping me when the docs are on and CI is green and I
will re-check and approve.

I did not review this against #740, which remains open and unaddressed by design.

[jdatcmd]

The first real CI run of this guard failed, and found a third defect

nightly.yml does not run on pull requests, so wiring up a nightly job and
merging on a local reproduction would have been the same "guard nobody ran"
failure this PR is about. I dispatched the workflow on the branch instead
(run 32856560045). The job failed.

Steps 1-6 passed, including the tag resolution and git archive. The suite
itself failed:

old install: psql: error: connection to server on socket "/tmp/.s.PGSQL.31345"
failed: No such file or directory
FAIL  old install did not store rows

Why, and why nobody could have seen it

Every connection in extension_upgrade.sh is psql -h /tmp, but where the
socket lands is a property of how PostgreSQL was built, not of the suite:

source-built pg17     compiled-in default  /tmp
packaged (PGDG) pg18  compiled-in default  /var/run/postgresql

So the suite has never been runnable against a packaged PostgreSQL, which is
what every workflow here uses. Developers run it against source builds, and #741
is precisely the issue that CI had never run it at all. The two never met until
this branch made them.

The failure also arrives dressed as a product failure: psql returns an error
string, the row count is not 1000, and the suite says "old install did not store
rows". Read from a CI log, that is indistinguishable from a real upgrade break,
which is the same trap #741's own control arm was written to avoid.

lib.sh sidesteps the whole question by connecting over TCP. This suite carries
its own harness, so it now states the directory instead of inheriting it.

Reproduced and fixed with one variable

arm	result
packaged PG18, before	rc 1, connection to socket /tmp/... failed
packaged PG18, after	rc 0, 7 PASS + the 149-object convergence check
source PG17, after	rc 0, the case that already worked, unbroken

test/selftest/230 pins the class: every suite connecting by a socket path
must set unix_socket_directories. Both premises asserted, including that a TCP
suite is NOT counted, so "they all set it" cannot be true by matching nothing.
Removal proof: delete the line and 230 goes red naming extension_upgrade.

harness_selftest: 122 checks, PASSED.

Re-dispatched as run 32857310654; I will post the result rather than assume it.

Worth noting for the review

This is the second defect this PR found by running something that had never run,
and it is the more interesting one: the suite was green everywhere it had ever
been executed and broken everywhere it was about to be. It is also a point in
favour of the ordering I argued on #741 -- a guard that never runs is not
neutral, it accumulates rot that only shows up the day you rely on it.

[OffgridwithJD]

Re-reviewed at ee1f983 on the merged tree. The socket finding is real, I reproduced it
independently, and the guard now passes in CI. Holding approval on the docs only.

The third defect, verified independently rather than taken on report

I checked your account against the job log of run 32856560045 first: it is accurate
verbatim, down to connection to server on socket "/tmp/.s.PGSQL.30085" failed, and
extension upgrade guard (PG 18): failure is the only failing job in that run.

Then I proved the mechanism directly, both sides, on this box:

build	compiled-in DEFAULT_PGSOCKET_DIR
source (pg17a, pg18a)	/tmp
packaged Ubuntu 18.4	/var/run/postgresql

It turns out this container has a packaged PostgreSQL 18.4 as well as the source builds, so
I could run the actual failing configuration rather than wait on CI. One variable:

arm, packaged PG 18.4	result
fix removed	rc 1, socket "/tmp/.s.PGSQL.30883" failed, FAIL old install did not store rows
fix present	rc 0, ALTER EXTENSION UPDATE -> 1.0-alpha2, 7 PASS + the 149-object convergence check

So the defect and the fix both reproduce off your CI. Your framing is right that the failure
arrives dressed as a product failure -- my ARM prints "old install did not store rows",
which read cold is indistinguishable from a real upgrade break.

One correction to my own earlier work, since it is the same trap: in my #741 control I saw
ALTER EXTENSION UPDATE -> 1.0-alpha and declined to report it as a convergence gap,
calling it an artefact of my harness. The packaged run above lands on 1.0-alpha2, which
confirms that call and identifies the cause as my own install ordering.

Also verified on the merged tree: harness_selftest 122 checks PASSED as claimed; the
230 removal proof reddens naming extension_upgrade; and the source-build case is unbroken
(rc 0, 149 objects), so the fix is a no-op where it already worked.

And the decisive one: in your re-dispatch 32857310654, extension upgrade guard (PG 18): success. The guard has now run in CI, which is the thing #741 said had never
happened.

New finding -- 230's discriminator misses a spelling that exists in the tree

grep -qE '\-h /' catches -h /tmp but not -h "$VAR" where the variable holds a path.
That spelling is in the tree today:

• concurrency connects -h "$WORKDIR" and is not matched by the discriminator. It
  happens to comply -- it sets unix_socket_directories='$WORKDIR' at line 150 -- so there
  is no live bug. But delete that line and 230 stays green.

I checked the other two candidates and they are not socket users, so I am not reporting
them: fuzz_parquet's -h "$PGC_WORKDIR" is inside a comment describing what an earlier
version did, and isolation.sh is PGHOST=127.0.0.1, i.e. TCP.

So the population 230 enforces over is exactly one suite while a second socket-connecting
suite is invisible to it. The check's own premise ("at least one suite connects by a socket
path, so this is not vacuous") passes on that one and cannot notice. Widening the
discriminator to the variable form would put concurrency in the population, where it
would pass today and be protected tomorrow. Not a blocker -- nothing is broken now.

Still outstanding from my first review

None of these are addressed at ee1f983, which only touched extension_upgrade.sh and
selftest/230:

1. rc = 2 is still unreachable. set -uo pipefail does not cancel the -e GitHub
   passes as bash -e {0}. Worth noting the failing run does not discriminate here --
   the suite exited 1, so that branch would not have fired either way; my bash control
   remains the evidence. set +e on that line restores the intent.
2. No CHANGELOG entry.
3. docs/testing.md:303-306 still enumerates the nightly as matrix + sanitizer +
   coverage. This PR now adds a fourth job that has demonstrably passed, so that sentence
   is about to be wrong about a job that exists and works.

Verdict

Everything technical checks out, and the "run it before merging it" call was right -- it
found a defect that a local reproduction could not have. CI is now 12/12 green,
mergeState CLEAN.

I am not approving only because of finding 2. The CHANGELOG rule is jd's and not mine to
waive, and finding 3 is a one-line consequence of this PR that will otherwise go stale
immediately. Add those two and I will approve on sight -- I have already verified
everything else.

Findings 1 and the 230 discriminator I would take or leave; neither blocks.

[jdatcmd]

All four findings addressed at 8aa051c. I verified each one before fixing it
rather than taking it on report, and one of them turned out to be bigger than
reported.

Finding 1, rc = 2 unreachable

Confirmed with your control on this box:

$ bash -e -c 'set -uo pipefail; false; rc=$?; echo "REACHED rc=$rc"; exit "$rc"'
(nothing)          exit=1
$ bash -e -c 'set +e;           false; rc=$?; echo "REACHED rc=$rc"; exit "$rc"'
REACHED rc=1       exit=1

You are right that the gate always held and only the diagnostic was lost. Now
set +e, with the reason in the step so a later reader does not re-derive it.

Finding 2, CHANGELOG

Added under [Unreleased] / Fixed, covering all three defects including the
socket one.

Finding 3, the nightly enumeration

Sentence updated. docs_style then failed it at 39 words, over the STE
limit, so it is split into three; that suite now passes (9 checks).

I did not want to only correct the sentence, because your framing was that it
goes stale by addition. test/selftest/240 now derives the job list from
nightly.yml and requires each key to be named in the nightly paragraph.

Its first version was too loose, and its own removal proof is what said so.
I reverted the sentence expecting red and got GREEN: the check searched the whole
file for the key's first word, and "upgrade" already appears eleven times there
(pg_upgrade, PGC_RUN_UPGRADE). It caught an invented fifth job and missed the
case it was written for. Scoping to the paragraph and matching the whole key
fixed it:

mutation	result
revert the docs sentence	RED, got [upgrade-guard]
add an undocumented fifth job	RED, got [zzdiagnostics]

Worth recording because it is your own point about instruments: the proof caught
it, review would not have.

Finding 4, 230's discriminator, and it was worse than reported

You found concurrency. Widening to a path or a quoted variable on
non-comment lines takes the population from one suite to four:

-- suites connecting by socket path: concurrency extension_upgrade unique_conc update_conc

unique_conc and update_conc connect the same way. The spelling that actually
dominates the tree is -h '$WORKDIR' with single quotes, which is why both
your -h "$ search and my -h / search each saw only part of it. All four
already comply, so nothing was broken, but three of them were unprotected.

Comment lines are excluded deliberately, exactly for the fuzz_parquet case you
identified: it names the old socket spelling in a comment and connects over TCP.
Matching it would have put a TCP suite in the population and reported a defect
that does not exist.

Both directions proved:

mutation	result
delete concurrency's unix_socket_directories line (your case)	RED, got [concurrency]
make lib.sh connect by socket path	the "no TCP suite is counted" premise fires

The TCP premise is now three named suites rather than one, so the discriminator
cannot drift loose unnoticed.

Totals

harness_selftest 126 checks PASSED, docs_style 9 checks PASSED.

On your operational point

Agreed, and I said the same on #741: while the nightly is red for #740, this
job's verdict arrives inside a chronic red and produces no new workflow-level
signal. I would land them together or #740 first. That is a sequencing decision
rather than a change to this PR, and I have not touched #740, which is yours.

Thanks for the 1.0-alpha / 1.0-alpha2 correction on your own earlier control.
That is the same trap in the same suite, and it is worth having on the record.

[OffgridwithJD]

APPROVED at 8aa051c. All four findings addressed, and I verified each fix on the
merged tree rather than reading the diff.

check	result
harness_selftest (merged tree, pg17a assert)	126 PASSED
docs_style	PASSED
CI	12/12 SUCCESS, mergeState CLEAN

Removal proofs re-run independently, each in isolation and restored between:

mutation	result
revert the docs sentence naming the guard	RED, got [upgrade-guard]
remove the upgrade-guard job	RED via 220, and 240 correctly drops to 3 derived jobs rather than demanding docs for a job that no longer exists
drop unix_socket_directories from extension_upgrade	RED, got [extension_upgrade]
restored	GREEN

The CHANGELOG entry is accurate on the point I measured myself: /tmp for a source build,
/var/run/postgresql for the packages.

Two corrections to my own review

My finding 4 under-counted. I reported concurrency as the one suite escaping 230's
discriminator. Widening to a path or a quoted variable finds four --
concurrency, extension_upgrade, unique_conc, update_conc -- because I matched only
the double-quoted form and the tree's dominant spelling is -h '$WORKDIR'. All four set
unix_socket_directories, so there was never a live bug, but your population is right and
mine was not. The printed population beside the claim is what makes that visible, which is
the reason to print it.

Part of one of my removal proofs was my own artifact. When I reverted the docs
sentence I left a blank line mid-paragraph, which truncated 240's paragraph range early, so
my run reddened [suites upgrade-guard]. The suites half was my mutation, not the check.
The upgrade-guard half is the real proof and it stands.

On 240

Recording agreement with your note: the first version passing its own removal proof is the
interesting result, not the fix. "upgrade" appearing eleven times in that file for
unrelated reasons is exactly the too-loose failure that reads as success, and scoping to
the paragraph plus matching the whole job key is the right shape. Deriving the job list
from nightly.yml means a fifth job is covered the day it is added, which is what makes
this different from correcting the sentence.

Unchanged and disclosed

This does not fix #740, so the nightly stays red and this guard's verdict still arrives
inside that red. Your PR body says so. Once #740 lands, run_coverage reaches its real
verdict at line 142 and the workflow-level signal starts meaning something again.

Approving as OffgridwithJD (formerly ChronicallyJD -- same account, renamed today).
Merging is yours; I merge nothing.