From 10f7434695b72429d61995766eefed03d8822e2d Mon Sep 17 00:00:00 2001 From: "Joshua (D) Drake" <136637981+ChronicallyJD@users.noreply.github.com> Date: Wed, 5 Aug 2026 09:11:00 -0600 Subject: [PATCH 1/2] test: refuse to compare a measurement nobody took (#418) check "$label" "$a" "$b" with both sides empty compares "" with "" and prints PASS. Every way a measurement goes missing produces exactly that: a tool that is not installed, a grep that matched no line, a psql against a cluster that is down, a substitution that expanded to nothing. That is not hypothetical. column_projection.sh piped its buffer count through bc, bc was absent on one machine, and the checks failed only because one side happened to be non-empty. Two green checks were produced the same day that measured nothing at all. lib.sh gains three assertions, all of which name the reason rather than the subject. "got [] want []" is the message that cost the time. check_num both sides must look like a number check_ratio a over b within a bound, refusing an empty side and a zero denominator, with awk rather than bc, because bc is not in a base install and its absence is what started this pgc_require_tools one clear line at the top instead of an empty string three checks later They are load-bearing now, so harness_selftest.sh proves each one by giving it what it must reject: two empty strings, a psql error message, a yes, a zero denominator. It also pins that plain check DOES pass on two empty strings, since that is the behaviour the rest of this exists to work around. run_all_versions.sh printed ALL VERSIONS PASSED and exited 0 when every configured pg_config was missing. The defaults name /usr/local/pg15 through pg19; a box whose assert builds are pg15a through pg19a skips all five, and that output then gets pasted into a pull request as the gate. It now reports how many versions ran, fails when that is zero, and says VERSIONS RUN PASSED rather than ALL VERSIONS PASSED when only some were present. One config in, one config run, so the CI invocations are unaffected. audit.sh runs under set -e with its verdict at the bottom, so an unguarded failure ended the run non-zero having printed neither a FAIL line nor a verdict, and every check below the abort silently did not run. Observed when a CREATE TABLE with COLLATE "en_US" met a box without that locale. An EXIT trap now says the suite aborted before reaching its verdict. It does not change the status. column_projection.sh, the suite that surfaced this, adopts the helpers and drops bc. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UqprqkCXuH8SegiZejE1Tw --- test/audit.sh | 20 +++++++++ test/column_projection.sh | 18 +++++--- test/harness_selftest.sh | 42 +++++++++++++++++ test/lib.sh | 94 +++++++++++++++++++++++++++++++++++++++ test/run_all_versions.sh | 30 ++++++++++++- 5 files changed, 197 insertions(+), 7 deletions(-) diff --git a/test/audit.sh b/test/audit.sh index 04f8943..065e7bf 100755 --- a/test/audit.sh +++ b/test/audit.sh @@ -48,6 +48,25 @@ set -euo pipefail +# set -e aborts this suite at the first unguarded failure, and the verdict block +# is at the bottom. So an abort ends the run non-zero having printed neither a +# FAIL line nor AUDIT TEST FAILED, and every check below the abort silently did +# not run (#418). +# +# Observed: a CREATE TABLE carrying COLLATE "en_US" on a box where that locale +# is not generated killed this suite partway through. The runner recorded +# audit=FAIL, correctly, and the output showed five PASS lines, one ERROR, and +# no verdict at all, which reads like a suite that stopped for no reason. +# +# The trap says which it was. It does not change the exit status, and it does +# not turn an abort into a pass; it stops an abort from being mistaken for a +# short run that went fine. +AUDIT_REACHED_END=0 +trap 'rc=$?; if [ "$AUDIT_REACHED_END" = 0 ]; then + echo; echo "AUDIT TEST ABORTED (exit $rc) before reaching its verdict."; + echo "The checks after the failing command did not run."; +fi' EXIT + . "$(dirname "${BASH_SOURCE[0]}")/portlib.sh" PG_CONFIG="${1:-/usr/local/pg17/bin/pg_config}" @@ -285,6 +304,7 @@ check "same-statement duplicate inserted nothing" \ q "DROP TABLE ss_dup;" >/dev/null echo +AUDIT_REACHED_END=1 if [ "$fail" = "0" ]; then echo "AUDIT TEST PASSED" else diff --git a/test/column_projection.sh b/test/column_projection.sh index a04e4c7..ff45da2 100755 --- a/test/column_projection.sh +++ b/test/column_projection.sh @@ -33,7 +33,8 @@ bufs() { # bufs -d "$PGC_DB" -Atq -c "$NOPAR" -c "SET $GUC=$1" \ -c "EXPLAIN (ANALYZE, BUFFERS) $2" 2>&1 | grep -m1 -oE 'Buffers: shared[^)]*' | - grep -oE '(hit|read)=[0-9]+' | cut -d= -f2 | paste -sd+ | bc + grep -oE '(hit|read)=[0-9]+' | cut -d= -f2 | + awk '{ n += $1 } END { print n + 0 }' } # The core oracle: projection on must equal projection off, byte for byte. @@ -94,13 +95,16 @@ check "premise: columnar and heap hold the same rows" \ # thing. B_ON="$(bufs on "SELECT sum(v) FROM t")" B_OFF="$(bufs off "SELECT sum(v) FROM t")" +# This premise exists because the ratio below means nothing on a tiny read. It +# now also catches a missing measurement: bufs() sums with awk and yields 0 +# rather than the empty string bc left behind when it was not installed (#418). check "premise: the unprojected read is large enough to measure (off: $B_OFF)" \ - "$([ "$B_OFF" -gt 1000 ] && echo yes || echo no)" yes + "$([ "${B_OFF:-0}" -gt 1000 ] && echo yes || echo no)" yes # One float8 column out of fourteen. A third is far above the ideal ~1/14 and # far below the 1.0 a broken projection would give, so it fails on a regression # without tracking encoding-ratio drift. -check "one column costs less than a third of reading all of them (on: $B_ON, off: $B_OFF)" \ - "$([ "$B_ON" -lt $((B_OFF / 3)) ] && echo yes || echo no)" yes +check_ratio "one column costs less than a third of reading all of them" \ + "$B_ON" "$B_OFF" 0.333 # A qual-only column must be read (it is filtered on) even though it is never # emitted -- if it were skipped the filter would silently match nothing. @@ -113,8 +117,10 @@ check "qual-only column still produces the right count" \ # count(*) metadata path, which reads no columns at all and would make this # check pass for the wrong reason. B_ALL="$(bufs on "SELECT count(*) FROM (SELECT t.* FROM t OFFSET 0) s")" -check "SELECT * reads as much as projection-off does ($B_ALL vs $B_OFF)" \ - "$([ "$B_ALL" -gt $((B_OFF * 8 / 10)) ] && echo yes || echo no)" yes +# Inverted, so the same helper carries it: projection-off over SELECT * must not +# exceed 1.25, which is "SELECT * costs at least 80 percent of projection-off". +check_ratio "SELECT * reads as much as projection-off does" \ + "$B_OFF" "$B_ALL" 1.25 # ---- 2. it is faithful ------------------------------------------------------ ab "single column agg" "SELECT sum(v) FROM t" diff --git a/test/harness_selftest.sh b/test/harness_selftest.sh index ca73439..54ac06f 100755 --- a/test/harness_selftest.sh +++ b/test/harness_selftest.sh @@ -257,4 +257,46 @@ done check "no test picks a port from inside the ephemeral range" \ "$([ -z "$_offenders" ] && echo none || echo "$_offenders")" "none" +# ---- the assertions that refuse an empty measurement (#418) ----------------- +# +# check "" "" prints PASS. Every way a measurement goes missing produces exactly +# that, so check_num and check_ratio exist to refuse it. Those two are now +# load-bearing, and a guard nobody tests is a guard that quietly stops working. +# +# Each probe runs in a subshell, because a deliberate failure must not fail this +# suite: PGC_FAIL and PGC_CHECKS are the harness's own state. The probe reports +# PGC_FAIL, so 1 means the assertion rejected what it was given. +_probe() { # _probe -> 0 when the assertion passed, 1 when it failed + ( PGC_FAIL=0; PGC_CHECKS=0; "$@" >/dev/null 2>&1; echo "$PGC_FAIL" ) +} + +check "check compares two empty strings and passes, which is why the rest exist" \ + "$(_probe check "empty vs empty" "" "")" "0" +check "check_num refuses two empty strings" \ + "$(_probe check_num "empty vs empty" "" "")" "1" +check "check_num refuses a psql error message" \ + "$(_probe check_num "error text" "ERROR: relation does not exist" "42")" "1" +check "check_num refuses the word a yes/no check would produce" \ + "$(_probe check_num "yes" "yes" "yes")" "1" +check "check_num still compares two real numbers" \ + "$(_probe check_num "equal" "42" "42")" "0" +check "check_num still fails two unequal numbers" \ + "$(_probe check_num "unequal" "41" "42")" "1" +check "check_num accepts a decimal and a sign" \ + "$(_probe check_num "decimal" "-1.5" "-1.5")" "0" + +check "check_ratio refuses an empty measurement" \ + "$(_probe check_ratio "empty" "" "100" "0.5")" "1" +check "check_ratio refuses a zero denominator rather than dividing by it" \ + "$(_probe check_ratio "zero denom" "10" "0" "0.5")" "1" +check "check_ratio passes a ratio inside its bound" \ + "$(_probe check_ratio "inside" "10" "100" "0.5")" "0" +check "check_ratio fails a ratio outside its bound" \ + "$(_probe check_ratio "outside" "90" "100" "0.5")" "1" + +check "pgc_require_tools passes on tools that exist" \ + "$(_probe pgc_require_tools awk sed)" "0" +check "pgc_require_tools fails on one that does not" \ + "$(_probe pgc_require_tools pgc_no_such_tool_exists)" "1" + pgc_summary diff --git a/test/lib.sh b/test/lib.sh index 6fe9f76..288326a 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -309,6 +309,100 @@ check() { fi } +# ---- assertions that refuse to compare a measurement nobody took ------------ +# +# check "$label" "$a" "$b" with both sides empty compares "" with "" and prints +# PASS (#418). Every way a measurement goes missing produces exactly that: a +# tool that is not installed, a grep that matched no line, a psql against a +# cluster that is down, a substitution that expanded to nothing. The suite then +# reports success for a number nobody has. +# +# This is not hypothetical and it is not rare. column_projection.sh piped its +# buffer count through bc, bc was absent on one machine, and both sides came +# back empty. It failed there only because one side happened to be non-empty. +# Two green checks were produced the same day that measured nothing at all. +# +# So: a measurement must look like a number before it is compared, and the +# failure says which side was not one. "got [] want []" is the message that cost +# the time. + +# Integer or decimal, optional leading sign. Deliberately strict: an empty +# string, a psql error message, and "no" are all not numbers. +pgc_is_number() { # $1 -> 0 when $1 is a number + local v="${1#-}" + v="${v#+}" + case "$v" in + '' | . | *[!0-9.]* | *.*.*) return 1 ;; + esac + return 0 +} + +# check_num LABEL GOT WANT -- check, with both sides required to be numbers. +check_num() { + local name="$1" got="$2" want="$3" + if ! pgc_is_number "$got" || ! pgc_is_number "$want"; then + PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_FAIL=1 + echo "FAIL $name: not a measurement, so nothing was compared:" \ + "got [$got] want [$want]" + return 1 + fi + check "$name" "$got" "$want" +} + +# check_ratio LABEL A B MAX -- assert A divided by B is at most MAX. +# +# awk rather than bc, on purpose. bc is not part of a base install and its +# absence is what produced the empty measurement in the first place; awk is +# required by POSIX and is present wherever these suites can run at all. +# +# A zero denominator is not a ratio. A zero numerator is reported rather than +# passed, because "the thing we measured cost nothing" is nearly always "the +# thing we measured did not happen", which is the same defect one layer down. +check_ratio() { # $1 label, $2 a, $3 b, $4 max + local name="$1" a="$2" b="$3" max="$4" ratio + + if ! pgc_is_number "$a" || ! pgc_is_number "$b" || ! pgc_is_number "$max"; then + PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_FAIL=1 + echo "FAIL $name: not a measurement, so no ratio was formed:" \ + "a=[$a] b=[$b] max=[$max]" + return 1 + fi + if [ "$(awk -v x="$b" 'BEGIN { print (x + 0 == 0) ? "yes" : "no" }')" = yes ]; then + PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_FAIL=1 + echo "FAIL $name: the denominator is zero, so there is no ratio:" \ + "a=[$a] b=[$b]" + return 1 + fi + ratio="$(awk -v a="$a" -v b="$b" 'BEGIN { printf "%.2f", a / b }')" + PGC_CHECKS=$((PGC_CHECKS + 1)) + if [ "$(awk -v r="$ratio" -v m="$max" 'BEGIN { print (r <= m) ? "yes" : "no" }')" = yes ]; then + echo "PASS $name (${ratio}x, bound ${max}x, from a=$a b=$b)" + else + echo "FAIL $name: ${ratio}x exceeds the ${max}x bound (a=$a b=$b)" + PGC_FAIL=1 + fi +} + +# pgc_require_tools TOOL... -- one clear line at the top, rather than an empty +# string three checks later. A suite that needs a tool it does not have has not +# been skipped; it has been silently narrowed. +pgc_require_tools() { + local t missing="" + for t in "$@"; do + command -v "$t" >/dev/null 2>&1 || missing="$missing $t" + done + if [ -n "$missing" ]; then + echo "FAIL the tools this suite measures with are missing:$missing" + PGC_CHECKS=$((PGC_CHECKS + 1)) + PGC_FAIL=1 + return 1 + fi + return 0 +} + # A check whose subject is a wall-clock ratio. # # PGC_SKIP_TIMING exists because a shared runner cannot hold a ratio still, and diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 3bb2cce..811c6df 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -338,12 +338,18 @@ runs_alone() { esac } +# How many majors were actually built and run. A matrix that ran nothing is not +# a matrix that passed, and until #418 it said "ALL VERSIONS PASSED" and exited +# 0 when every configured pg_config was missing. See the summary block. +VERSIONS_RUN=0 + for pgc in "${CONFIGS[@]}"; do if [ ! -x "$pgc" ]; then echo "SKIP $pgc (not executable)" SUMMARY+=("SKIP $pgc") continue fi + VERSIONS_RUN=$((VERSIONS_RUN + 1)) ver="$("$pgc" --version)" major="$(echo "$ver" | sed -E 's/^[^0-9]*([0-9]+).*/\1/')" @@ -581,9 +587,31 @@ echo "===================== MATRIX SUMMARY ============================" for line in "${SUMMARY[@]}"; do echo " $line" done +echo " versions run: $VERSIONS_RUN of ${#CONFIGS[@]} configured" echo "================================================================" + +# A run that built nothing is not a pass (#418). +# +# The default list names /usr/local/pg15 through /usr/local/pg19. On a box whose +# assert builds are pg15a through pg19a, every entry misses, each prints one +# SKIP line, and this block used to print ALL VERSIONS PASSED and exit 0. That +# output then gets pasted into a pull request as the gate. It is the same defect +# as check "" "" one level up, and it is worse, because this is the line people +# read instead of the checks. +# +# Reported rather than merely counted, because the count is what nobody looks at. +if [ "$VERSIONS_RUN" = 0 ]; then + echo "NO VERSIONS RAN: every configured pg_config was missing or not executable." + echo " configured: ${CONFIGS[*]}" + echo " Pass the pg_configs this box has, e.g. test/run_all_versions.sh /usr/local/pg18a/bin/pg_config" + exit 1 +fi if [ "$overall" = 0 ]; then - echo "ALL VERSIONS PASSED" + if [ "$VERSIONS_RUN" -lt "${#CONFIGS[@]}" ]; then + echo "VERSIONS RUN PASSED ($VERSIONS_RUN of ${#CONFIGS[@]}; the rest were skipped)" + else + echo "ALL VERSIONS PASSED" + fi else echo "SOME VERSIONS FAILED" fi From 97a783fdb50b081c8006278649b0f2ad3faab21f Mon Sep 17 00:00:00 2001 From: "Joshua (D) Drake" <136637981+ChronicallyJD@users.noreply.github.com> Date: Wed, 5 Aug 2026 10:44:30 -0600 Subject: [PATCH 2/2] test: check_ratio refused a zero denominator only, and claimed it refused both (#418) The comment said a zero numerator is reported rather than passed. The code checked the denominator. A measurement of zero is inside every bound, so it passed. That is not a stray inconsistency, because the same commit made it reachable. column_projection.sh's bufs() now sums with awk and returns 0 where it used to return the empty string. The empty string was a failure mode check_ratio rejects; zero was one it accepted. So the projected-read check would have reported success on a read that touched no buffers at all, which is #418 moved rather than closed, inside the check that started it. Either side being zero now fails, and harness_selftest.sh probes the numerator case beside the eleven others. The exact call jdatcmd quoted: before PASS ... (0.00x, bound 0.33x, from a=0 b=900) after FAIL ... a side of the ratio is zero, so nothing was measured A call site that genuinely needs to permit zero should say so under its own name rather than get it by default. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UqprqkCXuH8SegiZejE1Tw --- test/harness_selftest.sh | 5 +++++ test/lib.sh | 19 ++++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/test/harness_selftest.sh b/test/harness_selftest.sh index 54ac06f..e232613 100755 --- a/test/harness_selftest.sh +++ b/test/harness_selftest.sh @@ -289,6 +289,11 @@ check "check_ratio refuses an empty measurement" \ "$(_probe check_ratio "empty" "" "100" "0.5")" "1" check "check_ratio refuses a zero denominator rather than dividing by it" \ "$(_probe check_ratio "zero denom" "10" "0" "0.5")" "1" +# The numerator matters as much, and for a while this helper only checked the +# denominator while its comment claimed both. A measurement of zero is inside +# every bound, so it passed. +check "check_ratio refuses a zero numerator, which is inside every bound" \ + "$(_probe check_ratio "zero numerator" "0" "100" "0.5")" "1" check "check_ratio passes a ratio inside its bound" \ "$(_probe check_ratio "inside" "10" "100" "0.5")" "0" check "check_ratio fails a ratio outside its bound" \ diff --git a/test/lib.sh b/test/lib.sh index 288326a..4e63340 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -356,9 +356,18 @@ check_num() { # absence is what produced the empty measurement in the first place; awk is # required by POSIX and is present wherever these suites can run at all. # -# A zero denominator is not a ratio. A zero numerator is reported rather than -# passed, because "the thing we measured cost nothing" is nearly always "the -# thing we measured did not happen", which is the same defect one layer down. +# Zero on EITHER side is refused, not only the denominator. +# +# The first version of this checked only the denominator while this comment +# claimed both. The same commit changed column_projection.sh's bufs() to sum with +# awk, which returns 0 where it used to return the empty string, and that +# converted a failure mode this helper rejects into one it accepted: a projected +# read touching no buffers gives a ratio of 0.00, inside any bound, and passes. +# #418 moved rather than closed, inside the very check that started it. +# +# A zero numerator is "the thing we measured cost nothing", which is nearly +# always "the thing we measured did not happen". A call site that genuinely needs +# to permit zero should say so under its own name rather than get it by default. check_ratio() { # $1 label, $2 a, $3 b, $4 max local name="$1" a="$2" b="$3" max="$4" ratio @@ -369,10 +378,10 @@ check_ratio() { # $1 label, $2 a, $3 b, $4 max "a=[$a] b=[$b] max=[$max]" return 1 fi - if [ "$(awk -v x="$b" 'BEGIN { print (x + 0 == 0) ? "yes" : "no" }')" = yes ]; then + if [ "$(awk -v x="$a" -v y="$b" 'BEGIN { print (x + 0 == 0 || y + 0 == 0) ? "yes" : "no" }')" = yes ]; then PGC_CHECKS=$((PGC_CHECKS + 1)) PGC_FAIL=1 - echo "FAIL $name: the denominator is zero, so there is no ratio:" \ + echo "FAIL $name: a side of the ratio is zero, so nothing was measured:" \ "a=[$a] b=[$b]" return 1 fi