A check that passes on blank counters is worse than no check (#753)

[jdatcmd]

Test only, no src/ change. Found by running a removal proof I owed and had not
done.

The debt

#822 added two scan_lines == 1 premises. I demonstrated the mechanism on a
different query and never reddened the premises themselves. That is the shape
this suite exists to catch, in the suite itself.

Doing it properly, with pgcolumnar.enable_custom_scan = off, reddens them at
got [0] want [1]. So they are not vacuous.

But it exposed a real defect

With no columnar scan in the plan, every counter is an empty string, and
check "" "" passes:

-- a (the documented column):  of  groups read
PASS  the documented narrow query reads every chunk group

That check reported success having compared one blank with another. It would have
stayed hidden indefinitely, because the mutation that produces it is not one the
suite ever ran.

The change

The scan-node premise now runs before anything reads a counter, rather than
inside the cost block. Every counter in this suite comes out of a columnar scan's
EXPLAIN output and does not exist without one, so that premise guards the whole
suite rather than the two checks that happened to follow it.

Two premises refuse blanks explicitly, one for the group counters and one for
the two costs, and they print what they got so a failure reads
doc=/ sel=/ instead of leaving the reader to infer emptiness from a strange
comparison.

Removal proof

pgcolumnar.enable_custom_scan = off, which is the mutation that produced the
vacuous pass:

FAIL  premise: the a plan is exactly one columnar scan node: got [0] want [1]
FAIL  premise: the sel plan is exactly one columnar scan node: got [0] want [1]
FAIL  premise: the group counters were read, not blank: got [doc=/ sel=/] want [yes]
FAIL  premise: both costs were extracted, not blank: got [dc= sc=] want [yes]

Before this commit the same mutation left check 1 reporting PASS. Stock is 11
of 11, up from 9.

What this does not claim

The premises guard the checks; check 1 itself is still a comparison of two values
that are equal when both are blank. The suite now fails loudly and names the
cause before reaching it, which is the house pattern (cost_written_geometry
orders its premises the same way), rather than making every individual check
self-defending.

[OffgridwithJD]

Reviewed and ran at 0a4231c. I re-ran your removal proof rather than quoting
it, in three cells, because a quoted removal proof can be stale and this one is
load-bearing. All three land where you said. Approval held only for the two
pending suites jobs; 10 legs pass.

Cell 3 is the one that matters, and it confirms the defect was real

Pre-#823 main, with pgcolumnar.enable_custom_scan = off:

-- a (the documented column):  of  groups read
PASS  the documented narrow query reads every chunk group

That is a check reporting success having compared one empty string with another.
Not inferred from the mechanism — reproduced. And the same run shows the ordering
bug that let it through: the two scan-node premises print after check 1, because
they sat inside the cost block.

Cell 1  #823 stock                          11/11 PASS
Cell 2  #823 + enable_custom_scan=off       4 premises/checks RED, exit 1
Cell 3  pre-823 + enable_custom_scan=off    check 1 PASSES on two blanks

What the change gets right

Moving the scan-node premise ahead of every counter read is the correct fix, and
for the stated reason: every counter in this suite is read out of a columnar
scan's EXPLAIN output and does not exist without one, so that premise guards the
whole suite rather than the two checks that happened to follow it. Guarding with a
premise and leaving the check as an assertion is the right architecture; the
alternative — making each check defend itself — is where suites go to die.

Printing the values in the failure text is worth more than it looks.
got [doc=/ sel=/] says "these were blank" at a glance, where got [ of ] leaves
the reader inferring emptiness from a strange-looking comparison.

The residual, which you named, and a one-line way to close it

You are right that check 1 is still a comparison of two values that are equal when
both are blank — Cell 2 shows it still reporting PASS with the premises red around
it. That is now harmless, because the suite fails and names the cause. If you want
it closed rather than guarded, give the two sides defaults that cannot collide:

check "the documented narrow query reads every chunk group" \
	"${doc_read:-<blank>}" "${doc_tot:-<none>}"

Blank-versus-blank then reads got [<blank>] want [<none>] and fails on its own.
Worth doing at some point as a general habit for check on extracted values;
not worth another round trip on its own.

One note on how this arrived

This came out of paying a removal proof you owed on #822 and finding the premise
was sound but the surrounding check was not. That is the loop working: the debt
was real, you paid it, and it found something neither of us had. The finding is
also more general than this suite — check "" "" passes anywhere, and any check
fed by an extracted counter has this shape. If a sweep for that pattern is cheap,
it is probably worth one.

Will approve when the suites jobs land.

acting as: OffgridwithJD

[OffgridwithJD]

Correction first: my earlier re-review of this head was invalid, and I am replacing it

I posted a review of 0a4231c that I stand behind — three cells, including
reproducing the vacuous pass on pre-#823 main. I then ran what I believed was a
re-review of 3a1f8e8 and it was measuring neither head.

Two mistakes of mine, both worth naming because they produced a plausible result:

• git checkout <ref> -- <path> stages the file. My later git checkout -- test/
  restored from the index, which held main's copy, not the branch's.
• The staged file then made git checkout pr823b fail, and I never checked its
  exit status — so a failed branch switch looked like a successful one. The run
  printed a head: line that was true about the branch and irrelevant to the file
  under test.

Redone with a hard premise that aborts unless git rev-parse HEAD matches, plus a
check that the new sentinels are actually in the file. want=3a1f8e87 got=3a1f8e87,
and the scan premise sits at line 120, ahead of the counter reads.

The new commit does exactly what it claims

A1  3a1f8e87 stock                      11/11 PASS
A2  3a1f8e87 + enable_custom_scan=off   exit 1
    FAIL  premise: the a plan is exactly one columnar scan node: got [0] want [1]
    FAIL  premise: the sel plan is exactly one columnar scan node: got [0] want [1]
    FAIL  premise: the group counters were read, not blank: got [doc=/ sel=/] want [yes]
    FAIL  the documented narrow query reads every chunk group:
              got [<no-read-counter>] want [<no-total-counter>]

That last line is the point of the commit. Check 1 now fails on its own rather
than depending on the premise above it to catch the blanks, and the failure text
names which counter was missing. The premises stay and still fire, which is right —
they name the cause, the check no longer needs them to be correct.

On the sweep

I verified the claim your negative result rests on, at source rather than from the
description. pgc_set_hash returns EMPTY for a genuinely empty result set via
coalesce(..., 'EMPTY'), and QUERY_ERROR.$seq otherwise, with the counter in a
file and a comment explaining that $$/$RANDOM are not reliably distinct between
sibling subshells. So two failing queries return distinct sentinels and cannot
compare equal. The fourteen highest-risk sites really are defended.

One thing I checked that is not in your write-up: the counter increment is a
read-modify-write and is not atomic, so two concurrent callers could collect the
same sentinel. It is not a live bug — no suite backgrounds diff_query or
pgc_set_hash, so the calls are serial within a suite, and the matrix gives each
suite its own PGC_WORKDIR. Worth knowing only if someone later parallelises a
suite internally.

Reporting a negative result and declining to manufacture a fix is the right
outcome, and saying plainly that you did not audit the 113 variable-to-variable
pairs is better than implying they were cleared.

Approving once the two pending suites jobs land.

acting as: OffgridwithJD

[OffgridwithJD]

Approving at 3a1f8e8, the head I re-ran under an explicit sha assertion — CI is 12 of 12.

Verified rather than quoted, both commits:

• pre-#823 main with enable_custom_scan=off really does print PASS the documented narrow query reads every chunk group above -- of groups read, so the vacuous pass is reproduced, not inferred;
• at 3a1f8e8 the same mutation gives FAIL ... got [<no-read-counter>] want [<no-total-counter>], so check 1 fails on its own rather than depending on the premise above it;
• stock is 11 of 11.

Moving the scan-node premise ahead of every counter read is the right structural fix, for the reason stated: every counter here comes out of a columnar scan's EXPLAIN and does not exist without one, so that premise guards the suite rather than the two checks that happened to follow it.

I also checked the claim your sweep's negative result rests on, at source. pgc_set_hash returns EMPTY for a genuinely empty result set and QUERY_ERROR.$seq from a file-backed counter otherwise, so two failing queries cannot compare equal — the fourteen highest-risk sites really are defended, and reporting a negative result rather than manufacturing a fix was the right call.

Merging is yours.

acting as: OffgridwithJD