Refuse to compare a measurement nobody took (#418)

[ChronicallyJD]

Closes #418. @jdatcmd for review.

The shape

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.

column_projection.sh piped its buffer count through bc, bc was absent on one
machine, and it failed only because one side happened to be non-empty. I produced two
green checks the same day that measured nothing at all.

What is here

Three assertions in lib.sh, each naming the reason rather than the subject, because
got [] want [] is the message that cost the time.

helper	refuses
check_num	a side that is not a number
check_ratio	an empty side, and a zero denominator, before dividing
pgc_require_tools	a missing tool, once at the top, rather than an empty string three checks later

check_ratio uses awk rather than bc. bc is not in a base install and its absence
is what started this; awk is required by POSIX.

Proved by removal, since a guard nobody tests is a guard that quietly stops working

harness_selftest.sh gives each helper what it must reject. All thirteen pass on five
majors:

PASS  check compares two empty strings and passes, which is why the rest exist
PASS  check_num refuses two empty strings
PASS  check_num refuses a psql error message
PASS  check_num refuses the word a yes/no check would produce
PASS  check_num still compares two real numbers
PASS  check_num still fails two unequal numbers
PASS  check_num accepts a decimal and a sign
PASS  check_ratio refuses an empty measurement
PASS  check_ratio refuses a zero denominator rather than dividing by it
PASS  check_ratio passes a ratio inside its bound
PASS  check_ratio fails a ratio outside its bound
PASS  pgc_require_tools passes on tools that exist
PASS  pgc_require_tools fails on one that does not

The first line is deliberate. Plain check still passes on two empty strings, and pinning
that is what stops someone "fixing" check and quietly changing every suite in the tree.

The runner, which you asked me to add here

run_all_versions.sh printed ALL VERSIONS PASSED and exited 0 having built nothing.
Same box, same defaults, before and after:

BEFORE (main)         exit=0   ALL VERSIONS PASSED            5 SKIP (not executable)
AFTER  (this branch)  exit=1   NO VERSIONS RAN: every configured pg_config was missing
                               versions run: 0 of 5 configured
                               configured: /usr/local/pg15/bin/pg_config ...

It now counts versions run, prints that count in the summary, fails at zero, and says
VERSIONS RUN PASSED (n of m) rather than ALL VERSIONS PASSED when only some were
present. The CI invocations pass one pg_config, so one of one still reads
ALL VERSIONS PASSED and nothing downstream changes.

audit.sh

My note in the issue said it exits non-zero while printing no FAIL line. That was not
quite right and the real cause is worse. It runs under set -euo pipefail with its
verdict at the bottom, so an unguarded failure ends the run before the verdict, and
every check below the failure silently did not run. Observed when a CREATE TABLE with
COLLATE "en_US" met a box without that locale: five PASS lines, one ERROR, no verdict.

An EXIT trap now says so. It does not change the exit status and it cannot turn an abort
into a pass. Proved by injecting an unguarded false:

exit=1
AUDIT TEST ABORTED (exit 1) before reaching its verdict.
The checks after the failing command did not run.

column_projection.sh

The suite that surfaced this adopts check_ratio and drops bc.

Gate

Five-major assert matrix on the bench, fresh clone, git clean -fdx:

ALL VERSIONS PASSED
  versions run: 5 of 5 configured
  PASS  PG15   PASS  PG16   PASS  PG17   PASS  PG18   PASS  PG19
  harness_selftest=PASS x5   column_projection=PASS x5   audit=PASS x5

Zero failing suites.

Not in scope

No sweep of the other suites. They can adopt check_num as they are touched, which is
what the issue proposed. @jdatcmd, the local guard you added to agree() in #420 is the
first candidate: check_num covers it, and dropping the local one keeps a single shape.

🤖 Generated with Claude Code

[jdatcmd]

Verified, and it is good work. One hole survives in the new helper, and it is the

same hole one level down.

I checked the claims rather than reading them.

The runner guard, removal-proved

Same command, same box, only the branch differing:

--- main ---
exit=0
  SKIP   /nonexistent/bin/pg_config
ALL VERSIONS PASSED

--- this PR ---
exit=1
  versions run: 0 of 1 configured
NO VERSIONS RAN: every configured pg_config was missing or not executable.

That is the most valuable thing in the PR. ALL VERSIONS PASSED, exit 0, having built
nothing, is the line we paste into pull requests as the gate. Both of us have quoted that
runner this week.

harness_selftest.sh runs 27 checks and all 13 new probes pass on PG18. _probe running
each in a subshell so a deliberate failure does not corrupt PGC_FAIL is the right shape.

Blocking: check_ratio passes a zero numerator, and the comment says it does not

# 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.

I agree with every word. The code does not do it:

$ check_ratio "projected read touched ZERO buffers (a=0)" 0 900 0.33
PASS  projected read touched ZERO buffers (a=0) (0.00x, bound 0.33x, from a=0 b=900)

Only the denominator is checked. And this is not theoretical, because of the other change
in the same PR:

grep -oE '(hit|read)=[0-9]+' | cut -d= -f2 |
awk '{ n += $1 } END { print n + 0 }'

bufs() now returns 0 where it used to return the empty string. That was the right
call for the premise check, which tests -gt 1000. But it converts the failure mode from
one check_ratio rejects (empty) into one it accepts (zero), and then:

check_ratio "one column costs less than a third of reading all of them" \
	"$B_ON" "$B_OFF" 0.333

If EXPLAIN's Buffers: line ever changes shape, or the query errors, or the grep stops
matching, B_ON is 0, the ratio is 0.00, and it passes. A projected read that touched
no buffers at all reports success. That is exactly #418, moved rather than closed, in the
very check that started this.

B_OFF is protected only by the premise above it, and only because that premise happens
to exist here. The helper should not depend on each call site remembering.

Add the check the comment already promises, and a probe for it beside the other twelve:

check "check_ratio refuses a zero numerator" \
	"$(_probe check_ratio "zero numerator" "0" "100" "0.5")" "1"

If a legitimate call site ever needs to permit zero, that is worth an explicit
check_ratio_allow_zero rather than a silently permissive default.

Two smaller things, neither blocking

The inversion in column_projection.sh is correct, and I checked the algebra rather
than assuming: B_ALL > B_OFF * 8 / 10 is B_OFF / B_ALL < 1.25. Worth keeping the
comment, since the next reader will do that division too.

audit.sh's trap is right and the observed case is worth having in the file. One
note: trap ... EXIT fires on a normal exit too, which is why AUDIT_REACHED_END exists.
That works. It is also worth knowing the trap does not fire for SIGKILL, so a suite the
OOM killer takes still ends silently. Not worth chasing; worth not being surprised by.

On scope

You said you were not proposing a sweep of every suite, and I agree. check_num and
check_ratio earn their place by being adopted as files are touched.

I have a local non-empty guard in native_index_projection.sh's agree() on #420 that
should become check_num once this lands. I will do that rather than leave two mechanisms.

Fix the zero numerator and this has my approval.

[ChronicallyJD]

You are right, and the way it was wrong is worse than a stray inconsistency.

Fixed in 97a783f. Either side being zero now fails, and the numerator case is probed
beside the other eleven. Your exact call:

before  PASS  projected read touched ZERO buffers (a=0) (0.00x, bound 0.33x, from a=0 b=900)
after   FAIL  projected read touched ZERO buffers (a=0): a side of the ratio is zero,
              so nothing was measured: a=[0] b=[900]

The part I had not seen until you laid it out is that the same commit made it
reachable. Changing bufs() from paste -sd+ | bc to an awk sum was right for the
premise check, and it converted the empty string, which check_ratio rejects, into zero,
which it accepted. So the one call site this whole issue came from would have reported
success on a read that touched no buffers. I closed #418 in the helper and reopened it in
the caller, in one commit, while writing a comment that said I had not.

That is worth more than the fix. The helper cannot depend on each call site remembering,
which is the argument for the helper existing at all, and I wrote a default that required
exactly that.

The other two

The inversion algebra is right and the comment stays. On the audit.sh trap: agreed
about SIGKILL, and I would rather not chase it. A suite the OOM killer takes exits 137
and the runner records the suite as failed, so the loss is the verdict line and not the
result.

Gate

Re-gating on the bench once the ClickBench run for #421 finishes. Those are timings, and
the matrix runs six suites at once, so starting the matrix underneath them would make
both untrustworthy. I will post the five-major result rather than say it passed.

[jdatcmd]

Fixed and verified. Approving.

The case I raised now fails:

FAIL  projected read touched ZERO buffers: a side of the ratio is zero, so nothing was measured: a=[0] b=[900]
FAIL  zero denominator: a side of the ratio is zero, so nothing was measured: a=[100] b=[0]
PASS  inside bound  (0.10x, bound 0.5x, from a=10 b=100)
FAIL  outside bound: 0.90x exceeds the 0.5x bound (a=90 b=100)

One condition covering both sides, rather than a second branch, and the message names the
reason instead of the subject. harness_selftest.sh is 28 checks, all green on PG18, with
the new probe among them.

I also ran column_projection.sh, the suite that started this, since it is the one whose
call sites changed shape:

PASS  premise: the unprojected read is large enough to measure (off: 2524)
PASS  one column costs less than a third of reading all of them (0.04x, bound 0.333x, from a=109 b=2524)
PASS  SELECT * reads as much as projection-off does (1.00x, bound 1.25x, from a=2524 b=2524)
35 checks, PASSED

The from a=... b=... in the pass line is worth more than it looks. 0.04x against a
0.333x bound tells the next person the check has margin; the old boolean form told them
only that it did not fail.

The comment now records that the helper's first version claimed both sides and checked
one, and why the bc-to-awk change in the same commit made that reachable. Leaving
that in is the right call. It is the second time in this PR that a comment described a
property the code did not have, and the file now says so about itself.

What I said I would do

I have a local non-empty guard in native_index_projection.sh's agree() on #420, added
before this existed. Once this lands I will replace it with the shared helper so there is
one mechanism, as the comment above it already promises.

Not blocking, for whenever check_ratio is next touched

pgc_is_number accepts 5. and .5 but rejects 1e5. Fine for buffer counts and
timings, and stricter is the right default here. Worth knowing before someone feeds it a
psql float that came back in exponent form, which EXPLAIN's JSON output can do for
large costs.

Merging once CI is green.