test: the last fifty-eight checks in those ten suites record too (#965)

[OffgridwithJD]

Closes the remainder of #965: all ten suites now record every check they run. Second and final pass, after #969 converted each suite's own check helper.

Stacked on #969. Its parent is #969's head 19ddb4a3, so until #969 merges the diff on this page also shows #969's eleven files. The commit that belongs to this PR is the single one on top; after #969 lands, the diff reduces to it.

What was left and why it needed a second pass

#969 converted one helper per file and recorded 236 of the 293. The other 58 go through four further helpers, each printing a different display, so the same substitution would not reach them:

suite	helper	checks
phase6	eq_on_off	39
phase4	expect_fail 5, assert_plan 2, assert_plan_seq 1, and one check written inline in no helper at all	9
audit	expect_error	5
phase5	assert_plan	5
		58

The inline one is phase4.sh:249. It was not in any helper, so it does not appear in a helper census — I found it only because phase4's four helpers account for 8 and the measurement says 9. A count that does not close is the only reason it was found, which is the same way #969 found the citext bug.

Measured, PG18, four suites

Baseline is #969's head, same box, each tree building its own .so:

suite    records before -> after   checks run:   human lines   rc   human output
audit                26 ->  31             31            31     0   identical
phase4               29 ->  38             38            38     0   identical
phase5               31 ->  36             36            36     0   identical
phase6                4 ->  43             43            43     0   identical
                          +58

Every suite's record count now equals both its own human check lines and its checks run: total. Human output is byte-identical in all four: diff on the PASS/FAIL/SKIP lines is empty.

Three things in the conversion that are not mechanical

eq_on_off has three outcomes and two of them return early. Each one records. A return that skips the record leaves the check counted nowhere, which is precisely the state #965 exists to end, and it would have been the easy thing to miss in the 39 largest population here.

Two displays span more than one line. assert_plan in phase4 and phase5 prints the whole plan under a header when it fails. The dump is passed as part of the display rather than echoed after the record, so a failing run's output is byte-identical too — not just a passing one. Echoing it after would wedge the RESULT line between the header and the plan.

expect_error prints PASS <name> (rejected). (rejected) is kept verbatim. pgc_record takes the display whole, which is the property that made #969's conversions lossless, and it is the reason none of these delegate to lib.sh's own check.

A version-gated arm was invisible to the ledger, and now records a SKIP

audit.sh:293 gates its partitioned-parent arm on server_version_num >= 170000, because PG16 and earlier refuse PARTITION BY ... USING pgcolumnar. The gated branch printed a bare note and recorded nothing, so PG16 handed the ledger three fewer rows for audit with nothing saying why — a reader reconciling against a PG18 figure sees three checks that look lost.

It now records a SKIP with its reason, the way unique_conc.sh:546 already does for its own version gate. Driven on PG16, in its own build tree:

PG16 audit: rc=0  records=28  checks run: 28  human=28

SKIP  the partitioned-parent arm (PG16 refuses PARTITION BY ... USING pgcolumnar)
RESULT	audit	audit	the partitioned-parent arm	SKIP	PG16 refuses PARTITION BY ... USING pgcolumnar

23 + 4 expect_error + 1 SKIP = 28, and PG16's human output gains that SKIP line in place of the -- note.

One SKIP for the block, not one per gated check. Naming each of the four would make the count identical on every major, which is tempting and wrong here: it puts four check names in a branch that never runs them, where they drift from the four it is standing in for. Comparing counts across majors needs a major dimension in the ledger, which is #432's problem, not this file's.

Gate

docs_style           rc=0   checks run: 9     FAILs=0
harness_selftest     rc=0   checks run: 803   FAILs=0
bash -n              all four parse
shellcheck -S warning   one SC2154 at audit.sh:65, PRE-EXISTING
ledger + budget      untouched (no rows, no census, no budget)
four suites PG18     rc=0 each, verdict lines unchanged
audit on PG16        rc=0, 28 records

The shellcheck warning is rc is referenced but not assigned in audit.sh's EXIT trap at line 65, which this PR does not touch. Verified present on upstream/main and on #969's head as well as here, so it is pre-existing and not introduced. I am not fixing it in this PR because a trap's rc=$? assignment is a different change with its own argument.

Ledger

No rows, no census change, no budget change. A shell suite becoming coverable is not the same as covering it, and seeding these is still blocked on the major dimension — now with a second measured instance, since audit's own count differs by major.

What this closes and what it does not

After this, the remainder of #965 is zero — all 293 checks across the ten suites record, plus the citext SKIP #969 added and the gate SKIP this one adds. What #965 still asks for, and what neither PR does, is put these suites in the ledger.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

[OffgridwithJD]

There are eleven more verdict-printing lines in the ten suites, and neither my counts nor linuxhikerpm's could have found them. Swept after the conversion, with a control to show the sweep has teeth.

concurrency.sh   4   timeout waiting for $name/$label, ...sentinel, ...to block, ...idle-in-transaction
unique_conc.sh   4   the same four
update_conc.sh   3   three of the four
                --
                11

Control, the same grep against upstream/main, so it is not a grep that cannot match: 2, 4, 6, 2, 2, 10, 4, 5, 6, 5 across the ten. After this PR the only survivors are the eleven above.

Why the counts missed them

They are timeout paths. On a green run not one of them executes. That is why 293 human lines reconciled exactly against 236 + 58 on both our boxes — we were both counting a population that excludes them by construction.

		if [ "$i" -ge 1200 ]; then          # 1200 x 0.05s = 60s
			echo "FAIL  timeout waiting for $name/$label"
			fail=1
			return 1
		fi

What the gap actually is, stated precisely

It is not a false green. They set the suite-local fail, and all three suites derive their verdict from it (concurrency.sh:394, unique_conc.sh:588, update_conc.sh:478), so a timeout does turn the suite red. What is lost is that the failure touches neither PGC_CHECKS nor the record stream, so:

• the ledger never sees the failure, which is the checks_never_observed_red hole for these suites specifically;
• a human reading a red log sees a FAIL line that checks run: does not count, so the two disagree;
• and it becomes a false green the day any of these three switches its verdict from the local fail to PGC_FAILED. That is a plausible next step for this very conversion, which makes this worth recording now rather than when it bites.

Why I am not converting them in this PR

Two reasons, both about the work rather than the size of the diff:

A conversion here cannot be proven by running the suite. These lines do not execute on a green run, so every count stays identical whether I convert them correctly, incorrectly, or not at all — the property this PR rests on (records = human lines = checks run:) is blind to them. Proving it needs an induced timeout: a fixture that makes a wait actually exhaust 60 seconds. That is its own piece of work with its own arm, not a line in a mechanical pass.

The check name is dynamic, and that is a ledger decision. "timeout waiting for $name/$label" interpolates a session name and a sentinel label, so converting as-is puts unbounded names into a ledger keyed on (suite, part, name) — rows that can never be seeded because nobody can enumerate them. The right shape is almost certainly a fixed name per helper with the dynamic part in pgc_record's REASON field, which exists for exactly this. But choosing that is a decision about the key space, and it belongs in #965 where the seeding question lives rather than being settled quietly inside a substitution pass.

I will take it next, with an induced-timeout arm, unless you would rather it went with the seeding design. Recorded on #965 either way so it is not carried in a PR comment.

[OffgridwithJD]

I induced one of the eleven timeouts rather than leaving the argument unmeasured, and the result is worse than what I wrote an hour ago. It is not that the failure is invisible. It is that the accounting will balance while asserting there were none.

send_wait's iteration bound lowered from 1200 (60 s) to 1 in a scratch tree — all four bounds rewritten, count asserted before the run — so the first wait reports a timeout although its sentinel would have arrived. That changes when the path fires, not what it does; the three lines under test (the echo, the fail=1, the return 1) are untouched and a real hang runs the same three.

rc                              1
suite verdict                   CONCURRENCY TEST FAILED
FAIL lines printed              1      FAIL  timeout waiting for s1/a_del
RESULT records with FAIL        0
RESULT records total            7
checks run:                     7
human verdict lines             8
human - records                 1      <- the timeout, counted nowhere

The part I had wrong

I said the gap was that "the ledger never sees the failure". True but not the point. Records 7 and checks run: 7 agree with each other. The log reconciles. Nothing refuses it. And it reconciles on a run that printed a FAIL and exited 1.

So when these suites eventually emit an accounting: line — which is the whole direction of #917 and of this conversion — it will read:

accounting: 7 passed + 0 failed + 0 unrunnable + 0 skipped = 7

Zero failures, on a failing run, in a statement that balances. An aggregate that is missing data can be caught by a count that does not close; one that closes cannot. That is the same shape as a-derived-term-makes-a-reconciliation-an-identity, arrived at from the other side: here the failing term is not derived, it simply never increments.

What that changes about sequencing

Nothing about this PR — these eleven lines are untouched by it and behave identically before and after. But it does change what I would argue for next: I had filed the conversion as tidy-up behind the naming decision. It is not tidy-up. Until those eleven record, adding an accounting: line to these three suites would manufacture a false green, and adding that line is exactly what makes the checks run: totals this PR adds actually do something.

So the order that falls out: the eleven first, then the accounting line, not the reverse. The naming question (fixed name per helper with the dynamic part in pgc_record's REASON field) is still yours or jd's to settle, and I will not guess it — but it is now on the critical path rather than beside it.

Recorded on #965 with the measurement.

[OffgridwithJD]

13/13 green at 50368ba6.

For the reviewer, the three things worth driving rather than reading:

1. bash test/phase6.sh $PG_CONFIG — 43 records where there were 4, and the human lines unchanged. That is the largest single helper in the ten.
2. bash test/audit.sh /usr/local/pg16a/bin/pg_config in a tree of its own — 28 records, and the gated arm records SKIP the partitioned-parent arm (PG16 refuses PARTITION BY ... USING pgcolumnar). A tree already built for another major installs that major's .so into this prefix and the postmaster will not start; that cost me a run.
3. The eleven timeout paths in the comment above, which this PR does not touch and which a green run cannot see.

[jdatcmd]

Approved. Driven on PG16, which matters here — one of your claims is specifically about PG16 and you measured on PG18, so this is the half you could not check.

The four suites, against origin/main (post-#969)

suite      base rec   PR rec   checks run:   human   rc   human output
audit            23       28            28      28    0   gains ONE line
phase4           29       38            38      38    0   identical
phase5           31       36            36      36    0   identical
phase6            4       43            43      43    0   identical
                       +58

Records == human check lines == checks run: in all four. That three-way equality is the thing worth having: two of the three could agree while both being wrong about the third.

(My first run of this table showed base rec = 0 everywhere. My local main was at 226f805, four merges stale. The baseline was rebuilt from origin/main; the numbers above are the second run. Recording it because a stale baseline is the third instrument fault of mine today and it fails in the flattering direction — it would have shown your PR adding 28 records to a suite that already had 23.)

The strongest claim in the PR, tested rather than read

a failing run's output is byte-identical too — not just a passing one

Broke the first assert_plan identically in both trees so a red run happens, then diffed:

CONTROL   base vs base, two runs of the SAME tree     identical
TEST      base vs PR, both failing                    identical

The control is there because the raw diff did show differences — server log timestamps, PIDs and ports — and without running the same tree twice I could not have said whether those were your change or the clock. They are the clock.

And the placement claim holds, which is the actual risk:

FAIL  btree plan is index scan: plan was:
        Index Scan using bt_a_idx on bt
          Index Cond: (a = 12345)
RESULT	phase4	phase4	btree plan is index scan	FAIL

The RESULT line lands after the whole dump, not wedged between the header and the plan.

The three non-mechanical parts

eq_on_off's three outcomes each record, both early returns included. Checked against the merged source, not the diff. In a population of 39 that is the one that would have gone unnoticed, because a return skipping the record leaves the check counted nowhere and the human line still prints.

phase4's arithmetic closes: expect_fail 5 + assert_plan 2 + assert_plan_seq 1 = 8, and the measurement says 9. The inline check at :249 is the ninth. A count that does not close is the only reason it was found — the same mechanism that found the citext bug in #969, twice in two PRs.

The block-level SKIP precedent is real. unique_conc.sh:547 does exactly this for its own version gate, and check_skip is pgc_record SKIP so it records rather than printing. Verified both rather than taking the citation.

The PG16 half you could not measure

18a19
> SKIP  the partitioned-parent arm (PG16 refuses PARTITION BY ... USING pgcolumnar)

Exactly as you predicted, on the box that produces it. The gated branch previously printed a bare note and recorded nothing, so PG16 handed the ledger three fewer audit rows with nothing saying why.

One thing for #432's list, not a blocker here

This adds one more instance of the per-major name divergence, and your comment already argues the alternative is worse — naming each of the four gated checks in a branch that never runs them would duplicate names and let them drift. I agree with the call.

But the consequence should be written down where the seeding design will look: audit's check-name set now differs by major in both directions — the partitioned-parent arm exists only below PG17, and the four real names only at PG17 and above. That is fk_referencing again, and it is now in a second suite. Worth a line on #432 so the eventual major-dimension design is sized against the real number of diverging suites rather than the one I happened to measure.

Merging

CI 13/13, ms=CLEAN. Gated on a342cc03, and I will re-verify the head if it moves — #969 taught me that an approval state survives a change of scope without saying so.

#965 closes with this one, and the count in it should be read as I measured: 58 records added, of which 57 were previously-unrecorded checks and one is the new SKIP that did not exist before.