From 10d779e9655d51c095f4f262b588d42fc877c24f Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sat, 8 Aug 2026 08:41:16 -0600 Subject: [PATCH 1/2] fix: a suite must not report checks against the previously installed .so pgc_setup ran the build and the install 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 did not stop the suite. It carried on and ran every check against whatever .so was installed last, then printed a full report for code that does not exist. Reported by ChronicallyJD, who caught it only by fingerprinting the installed .so across a source change and seeing the same hash twice. Proven by removal, with `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: *** [: src/columnar_bloom.o] Error 1 FATAL: the build failed, so there is nothing new to test and no checks run. With the guard removed, the same broken tree: PASS the filter still skips groups it can prove empty (#467) PASS a column above the distinct cap is refused a filter, not given a bad one ... checks run: 19 bloom_sizing.sh: PASSED Nineteen green checks against a source file that cannot compile. That is worse than a wrong answer, because it is indistinguishable from a right one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FeNm2Gw6h16Z123We3F1vJ --- test/lib.sh | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/test/lib.sh b/test/lib.sh index e9eb366..4699aa2 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -121,11 +121,27 @@ pgc_setup() { # PGC_SKIP_BUILD so parallel suites do not each rebuild (a no-op relink) or # race on writing the shared .so during "make install". A suite run on its own # still builds and installs. + # + # Both steps are status-checked. lib.sh sets `set -uo pipefail` but not -e, so + # an unchecked make that fails to compile does not stop the suite: it carries + # on and runs every check against the PREVIOUSLY INSTALLED .so, then prints a + # full PASS/FAIL report for code that does not exist. That is indistinguishable + # from a real result, and it was caught only because someone fingerprinted the + # installed .so and saw the same hash either side of a source change that could + # not have produced it. if [ -z "${PGC_SKIP_BUILD:-}" ]; then echo "-- building" - make -C "$PGC_SRCDIR" PG_CONFIG="$PGC_PG_CONFIG" >/dev/null + if ! make -C "$PGC_SRCDIR" PG_CONFIG="$PGC_PG_CONFIG" >/dev/null; then + echo "FATAL: the build failed, so there is nothing new to test" >&2 + echo " (refusing to report checks against the previously installed .so)" >&2 + exit 1 + fi echo "-- installing" - make -C "$PGC_SRCDIR" install PG_CONFIG="$PGC_PG_CONFIG" >/dev/null + if ! make -C "$PGC_SRCDIR" install PG_CONFIG="$PGC_PG_CONFIG" >/dev/null; then + echo "FATAL: the install failed, so the .so under test is not the one just built" >&2 + echo " (refusing to report checks against the previously installed .so)" >&2 + exit 1 + fi fi # initdb and pg_ctl cannot run as root; use postgres when we are root. From 5ec150138e0a04170218d7740a75e451bfdce5ec Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sat, 8 Aug 2026 09:00:54 -0600 Subject: [PATCH 2/2] fix: arm the teardown trap before the build, not after it (#508 review) The exit added by the previous commit sat between `mktemp -d` at :108 and `trap pgc_teardown EXIT`, so a failed build left /tmp/pgcolumnar-test.XXXXXX behind every time -- and it fires exactly when someone is iterating on code that does not compile, so they accumulate. Before that commit the path ran on to the trap. Caught in review by ChronicallyJD. The trap now arms as soon as there is a directory to remove. Stopping a cluster that never started is a no-op, so arming it early costs nothing. The privilege block moved above it rather than the trap alone moving up, which is the part the review did not have to notice: pgc_teardown reaches pg_ctl through pgc_pg, which expands PGC_RUNPG, and that array was set AFTER the build. A trap armed before it would have turned any early failure into an unbound-variable error under `set -u` instead of a cleanup. PGC_PGDATA was already set at :109 and needed no move. Proven by removal, broken source, counting /tmp/pgcolumnar-test.*: trap above the build (this commit) exit 1, workdirs left: 0 trap below the build (as shipped) exit 1, workdirs left: 1 And the passing path is unchanged: a clean run of bloom_sizing.sh reports 19 checks, PASSED, leaves 0 workdirs and 0 leftover clusters. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FeNm2Gw6h16Z123We3F1vJ --- test/lib.sh | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/test/lib.sh b/test/lib.sh index 4699aa2..4989fd4 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -117,6 +117,26 @@ pgc_setup() { echo "version=$("$PGC_PG_CONFIG" --version)" echo "workdir=$PGC_WORKDIR" + # initdb and pg_ctl cannot run as root; use postgres when we are root. + # + # Settled BEFORE the build, and before the trap below, because pgc_teardown + # reaches pg_ctl through pgc_pg, which expands PGC_RUNPG. Installing the trap + # while that array is still unset would turn any early failure into an + # unbound-variable error under `set -u` instead of a cleanup. + if [ "$(id -u)" = "0" ]; then + PGC_RUNPG=(runuser -u postgres --) + chown -R postgres "$PGC_WORKDIR" + chmod 777 "$PGC_WORKDIR" "$PGC_SQLDIR" + else + PGC_RUNPG=(env) + fi + + # Armed here rather than after the build: the build below can exit, and + # between mktemp above and this line there is nothing to remove the workdir. + # Stopping a cluster that was never started is a no-op, so arming it early + # costs nothing and covers every failure path after the directory exists. + trap pgc_teardown EXIT + # The matrix runner builds and installs once per version and sets # PGC_SKIP_BUILD so parallel suites do not each rebuild (a no-op relink) or # race on writing the shared .so during "make install". A suite run on its own @@ -144,17 +164,6 @@ pgc_setup() { fi fi - # initdb and pg_ctl cannot run as root; use postgres when we are root. - if [ "$(id -u)" = "0" ]; then - PGC_RUNPG=(runuser -u postgres --) - chown -R postgres "$PGC_WORKDIR" - chmod 777 "$PGC_WORKDIR" "$PGC_SQLDIR" - else - PGC_RUNPG=(env) - fi - - trap pgc_teardown EXIT - echo "-- initdb" pgc_pg "initdb -D '$PGC_PGDATA' -A trust" >/dev/null 2>&1 {