fix: a suite must not report checks against the previously installed .so

[jdatcmd]

Reported by @ChronicallyJD, who caught it the only way it can be caught from the
outside: by fingerprinting the installed .so across a source change and seeing
the same hash twice.

The defect

pgc_setup ran both make steps with no status check:

make -C "$PGC_SRCDIR" PG_CONFIG="$PGC_PG_CONFIG" >/dev/null
make -C "$PGC_SRCDIR" install PG_CONFIG="$PGC_PG_CONFIG" >/dev/null

lib.sh sets set -uo pipefail but not -e, so a failed compile does not
stop the suite. It carries straight on and runs every check against whatever
.so was installed last, then prints a full report for code that does not exist.

Scope: the matrix was never at risk, only the standalone path

The obvious reviewer objection, checked rather than assumed:
run_all_versions.sh status-checks both steps and records
FAIL PG$major (build) / (install), and separately fails the major on compiler
warnings. Suites run under PGC_SKIP_BUILD therefore never built at all and
could not hit this. The gap was exactly and only the standalone
test/<suite>.sh path — which is what a person runs while iterating, i.e. the
case where the source most often does not compile.

Proof by removal

this is not valid C and cannot compile; appended to src/columnar_bloom.c.

With the guard:

-- building
src/columnar_bloom.c:260:1: error: unknown type name 'this'
make: *** [<builtin>: src/columnar_bloom.o] Error 1
FATAL: the build failed, so there is nothing new to test
       (refusing to report checks against the previously installed .so)

No checks run.

With the guard removed, same broken tree:

PASS  the filter still skips groups it can prove empty (#467)
PASS  premise: one row group holds more than the refusal threshold
PASS  a column above the distinct cap is refused a filter, not given a bad one
PASS  and the load itself succeeded, rather than erroring on the way

checks run: 19
bloom_sizing.sh: PASSED

Nineteen green checks, including four premise assertions, against a source file
that cannot compile. That is worse than a wrong answer because it is
indistinguishable from a right one — and it is green, so nobody looks.

Second commit: the trap had to move too

Review caught that the new exit 1 sat between mktemp -d and
trap pgc_teardown EXIT, leaking a workdir on every failed build — precisely
when someone is iterating on code that does not compile. Fixed, proven by
removal:

trap position	suite exit	workdirs left
above the build (now)	1	0
below the build (first commit)	1	1

Moving the trap alone would have been wrong: pgc_teardown reaches pg_ctl
through pgc_pg, which expands PGC_RUNPG, set after the build. A trap armed
above it with the array unset turns an early failure into an unbound-variable
error under set -u. The privilege block moved up with it.

Passing path unchanged: clean bloom_sizing.sh gives 19 checks, PASSED, 0
workdirs, 0 leftover clusters.

Class

Third instance of the same shape found this week: a step that fails silently and
a report that speaks anyway. #506 is the ClickBench reporter printing the
cold-cache tag on hosts where the drop was refused; #503 lists four sites where a
cost estimate is re-derived and one copy stops moving. The common repair is that
the reporting step must not be reachable when the step it reports on did not
happen.

[ChronicallyJD]

Approving. The removal proof is the part that should go in the commit message if
it is not already there — 19 green checks, four of them premise assertions,
against a source file containing this is not valid C and cannot compile; is a
better argument for this guard than any description of it. It is also the exact
shape I hit: my run reported 8 checks including the very check I had just written,
and the only thing that gave it away was the installed .so hash being identical
either side of a source change that could not have produced it.

I checked the obvious follow-on question and it comes out in your favour: the
matrix path was already covered. run_all_versions.sh status-checks both steps
at :559 and :580 and records FAIL PG$major (build) / (install), so
PGC_SKIP_BUILD suites were never at risk from their own build — the gap was
exactly and only the standalone path this patch fixes. Worth a line in the body,
because the natural reviewer worry is "the authoritative gate skips this block".

One thing to fix, small

The new exit 1 sits between the workdir being created and the trap that
removes it:

lib.sh:108   PGC_WORKDIR="$(mktemp -d /tmp/pgcolumnar-test.XXXXXX)"
lib.sh:125   echo "-- building"      <-- new exit 1 here
lib.sh:127   echo "-- installing"    <-- and here
lib.sh:140   trap pgc_teardown EXIT

So a build failure now leaks /tmp/pgcolumnar-test.XXXXXX every time. Before this
patch the same path ran on to the trap and cleaned up; the guard is right, but it
turns a compile error into an accumulating one. On a box where someone is
iterating on a change that does not compile — which is precisely when this fires —
that is one directory per attempt, and each holds a full data directory once it
gets further.

Cheapest fix is to move trap pgc_teardown EXIT above the build block. It is
already safe to run early: pgc_teardown keys off PGC_PGDATA and PGC_WORKDIR,
both set by then, and stopping a cluster that was never started is a no-op. If you
would rather not move it, rm -rf "$PGC_WORKDIR" before each exit 1 does the
same job less tidily.

Two things I checked and am happy with

• exit 1 and not exit 2. #499 established 1 for a hard failure and 2 for
  too-few-samples; a build that did not happen is unambiguously the former.
• Message on stderr, signal in the exit status. A caller that captures only
  stdout still fails, because it fails on the status. That is the right way round —
  the reverse (message on stdout, status 0) is the bug being fixed.

Beyond this PR

You are right that it is the third instance this week, and I would add a fourth
from today: the .so fingerprint is what caught this one, and nothing in the
harness prints it. Every suite already knows $($PGC_PG_CONFIG --pkglibdir), so
one line in pgc_setup — echo "-- .so: $(md5sum ...)" — would put the hash in
every log, and a red-on-change proof would then carry its own evidence that the
two arms were different binaries. I have been doing it by hand all day and it has
paid for itself twice. Happy to send that as a follow-up rather than pile it here.

[jdatcmd]

Both points taken. Pushed as 5ec1501.

The workdir leak is real and is fixed

Verified before fixing: mktemp -d at lib.sh:108, my exit 1 at :135, trap pgc_teardown EXIT at :156. Proven by removal, counting /tmp/pgcolumnar-test.* after a failed build:

trap position	suite exit	workdirs left
above the build (now)	1	0
below the build (as shipped)	1	1

And the passing path is unchanged — a clean bloom_sizing.sh reports 19 checks, PASSED, leaving 0 workdirs and 0 leftover clusters.

One thing the fix needed that the review did not have to catch. Moving the trap alone would have been wrong: pgc_teardown reaches pg_ctl through pgc_pg, which expands PGC_RUNPG, and that array was set at :149 — after the build. A trap armed above the build with it still unset turns any early failure into an unbound-variable error under set -u instead of a cleanup, which is a worse failure than the leak. So the privilege block moved up with it. PGC_PGDATA was already set at :109 and needed no move.

The matrix path was already covered

Confirmed rather than taken on trust — run_all_versions.sh status-checks both steps and records FAIL PG$major (build) / (install), and separately fails on compiler warnings. So PGC_SKIP_BUILD suites were never at risk from their own build, and the gap was exactly and only the standalone path. Added to the PR body, since it is the first thing a reviewer should worry about.

On the .so hash follow-up

Agreed, and I think it is stronger than a convenience. It is the only reason this defect was findable at all: the evidence was two identical hashes across a source change that could not have produced them. A red-on-change proof that prints its own binary fingerprints stops being an assertion about what was tested and becomes a record of it.

Worth deciding one thing when you write it: printing the hash is useful, but the check that pays is comparing it across the two arms of a removal proof. A hash in a log nobody diffs is another passing-suite line. If pgc_setup records it somewhere a proof can read back, the "before" and "after" arms can assert they were different binaries rather than leaving the reader to trust it.