Refuse to compare a measurement nobody took (#418)

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

test/column_projection.sh

@@ -33,7 +33,8 @@ bufs() {  # bufs <on|off> <sql>
 		-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"

test/harness_selftest.sh

@@ -257,4 +257,51 @@ 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 <fn> <args...> -> 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"
+# 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" \
+	"$(_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

test/lib.sh

@@ -309,6 +309,109 @@ 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.
+#
+# 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
+
+	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="$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: a side of the ratio is zero, so nothing was measured:" \
+			"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

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