test: the decode arms could not see a decode (#1077 sweep)

[jdatcmd]

The arm named "the visibility-only caller decodes nothing" passes on a tree
where the visibility caller decodes. Same file as #1078, one arm further in, and
a worse failure than that one.

Two arms asserted properties about the projection path by counting a CALL SITE,
with its argument text, pinned at a literal 1:

grep -c 'PgColumnarRowIsLive(rel, snap, baseRow)'            "1"
grep -c 'PgColumnarReadRowByNumberCols(rel, snap, baseRow'   "1"

That asserts a particular call is still written the way it was written, which is
not the property either name claims.

Measured

Plant the regression these arms exist to catch -- a full
PgColumnarReadRowByNumber beside the liveness check in the visibility path:

state                          live cols full | OLD1  OLD2 | NEW
clean                             1    1    0 | PASS  PASS | PASS
REGRESSED, full decode added      1    1    1 | PASS  PASS | RED
honest 2nd narrow caller          2    1    0 | PASS  PASS | PASS
empty file                        0    0    0 | RED   RED  | premises RED

Row 2 is the finding. The call they count is still there; the decode added next to
it is invisible to them.

One correction to how this would be easiest to describe. Unlike #1078's
allColumns pair, these fail in ONE direction only. A realistic second caller uses
different variable names, so the exact-text count stays at 1 and the old arm is
BLIND rather than falsely alarmed — row 3 is a pass by accident, not by
correctness. "Same defect as #1078" is the easy sentence and it is not true.

The fix is a want-ZERO

Three entry points exist and only one decodes every column:

PgColumnarRowIsLive            answers visibility, decodes nothing
PgColumnarReadRowByNumberCols  decodes a given set of columns
PgColumnarReadRowByNumber      decodes EVERY column

so the projection path is asserted never to call the third. More correct callers of
the two narrow entry points move nothing; any full decode moves it off zero. That
is the opposite failure direction from a count pinned at 1, and it is the shape a
second honest caller cannot break.

Three premises, not two. Each narrow entry point is called at all — a file that
called nothing would also report zero full decodes. And the third is one I nearly
left implicit: the whole arm rests on PgColumnarReadRowByNumberCols( not matching
PgColumnarReadRowByNumber(. That is asserted in the suite rather than assumed by
me, because renaming the narrow entry point to a prefix of the wide one would make
the zero silently start counting the honest caller.

Removal proof, against the real suite

mutation applied   md5 c8324ce4f486 -> a2c26702a9cc, full decodes 0 -> 1
suite run          16 passed + 1 failed + 0 unrunnable + 0 skipped = 17
the one red        neither caller decodes every column: no full decode in the
                   projection path: got [1 full decode(s)] want [0 full decode(s)]
all three premises stayed green, correctly
restored           md5 c8324ce4f486, git diff empty

Five majors, this exact tree

Own make clean, own build and own .so per major. The suite file is md5
8cb2aa5d5f82 in both the tested tree and this commit, so the matrix is about the
code being reviewed:

PG15 17 PASS   PG16 17 PASS   PG17 17 PASS   PG18 17 PASS   PG19 17 PASS

No ledger change, checked rather than inherited from #1078

native_fetch_projection has 0 rows in main's ledger, so the gate cannot refuse
its checks. Neither removed name appears in the ledger at all, so there is nothing
to prune either.

Scope

The fourth and fifth arm in this file repaired for counting a string across a whole
file. A sweep found 12 of this shape across the suites. The remaining seven are
@OffgridwithJD's, in native_fetch_cache.sh (4), native_fetch_position.sh (2),
decode_interrupts.sh (1) and native_saop_pushdown.sh (1). Three further arms of
the whole-file shape are already want-zero and correctly need nothing.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

[OffgridwithJD]

APPROVE at f115d0b. 14/14 green. I built the state table independently from main's columnar_projection.c before reading yours, and it matches.

Row 2 holds, which is the whole case

state                            live cols full |  OLD1  OLD2 |  NEW  prem
CLEAN                               1    1    0 |  PASS  PASS | PASS  PASS
REGRESSED (full decode added)       1    1    1 |  PASS  PASS |  RED  PASS
HONEST 2nd narrow caller            2    1    0 |  PASS  PASS | PASS  PASS
EMPTY file                          0    0    0 |   RED   RED | PASS   RED

Both old arms pass a tree where the visibility caller decodes every column. The arm named "the visibility-only caller decodes nothing" counts a call site that is still there and cannot see the decode added beside it.

Row 4 confirms the premises are load-bearing: the want-zero passes on an empty file, and only the premises catch it.

Your correction to the direction is right, and row 3 is why

These two do not fail in both directions the way the allColumns pair did. A realistic second narrow caller uses different variable names, so the exact-argument-text count stays at 1 and the old arms stay green rather than falsely alarming. Missed detection alone.

Saying so rather than writing "same defect as #1078" is the harder and more accurate sentence, and it changes the fix: a want-zero is right here precisely because more correct callers of the narrow entry points move nothing.

Your first stated limit is smaller than you think

The want-zero rests entirely on PgColumnarReadRowByNumberCols( not matching PgColumnarReadRowByNumber(.

It rests on it, but a broken prefix relationship cannot pass silently — it reddens the main arm rather than hiding in it. By construction:

if 'Cols(' matched the full-decode pattern:  full >= cols   (every narrow call counts twice)
premise asserts                              cols >= 1
main arm asserts                             full == 0
-> full >= 1 and full == 0 is a contradiction

cols=1 prefix intact  -> full=0  arm PASS
cols=1 prefix broken  -> full=1  arm RED
cols=3 prefix broken  -> full=3  arm RED

So the protection you wanted is already there, supplied by the conjunction of cols >= 1 and full == 0 rather than by the third premise. That also answers your second limit: the hardcoded printf premise is a tautology about the pattern pair, as you say — but it documents intent rather than providing the guarantee, and the guarantee does not depend on it. I would keep it and say that in the comment.

The limit that is real

A second spelling of the wide entry point — a macro, a wrapper, PgColumnarReadRowByNumber ( with a space — keeps the zero at zero and says nothing. That one is not closable by a literal match and is correctly a stated limit rather than a solved problem. Worth noting pgindent will not produce the space form, so the realistic risk is a wrapper, which would be visible in review of the wrapper itself.

What I like

Choosing a want-zero over a want-one because three entry points exist and only one decodes everything. That is the category where a count IS the property, and it is the one shape from the whole sweep that needs no self-referential trick — more correct callers move nothing, any full decode moves it off zero.

And the removal proof being a suite run with accounting rather than a shell calculation, on the real columnar_projection.c, is what makes row 2 evidence rather than a claim.

[jdatcmd]

Pushed 7920784. Two fixes, and the first is a hole in this PR that neither of
us looked for.

The CHANGELOG entry was never committed

f115d0b — the head you approved — contains only
test/native_fetch_projection.sh. The entry was written, sat unstaged, and never
entered the commit. git reset --soft leaves the index alone and git commit
commits the index, so a working-tree edit made after the last git add is simply
absent.

I "verified" it with git diff --stat origin/main after committing, which
compares the working tree to main rather than the commit to main. It printed
both files and read as confirmation. That check cannot see this class of mistake
at all. The one that can is git show <sha> --name-only.

Checked the siblings the same way: #1082 and #1083 both carry their entries. Only
this one.

Worth naming for the review side too: this is review-the-file-list-not-just-the-fix
with the sign flipped. That rule is about added files a behavioural review does not
look at; this is a missing file, and a review that reasons about the arms — as
yours correctly did, in detail — has no reason to notice the CHANGELOG is not
there.

Your correction to the third premise, taken

The comment said "the whole arm rests on it". It does not, and your derivation is
in the file now:

if 'Cols(' matched the wide pattern:  full >= cols   (every narrow call counted twice)
premise asserts                       cols >= 1
main arm asserts                      full == 0
-> contradiction; the arm reddens rather than hiding

The premise documents the assumption and names what a future rename would break.
It is not the guarantee. Kept, with the comment now saying which it is — your
suggestion exactly.

What did not move

No check name changes, so no ledger key changes: 17 names, identical to
f115d0b by sorted diff. Suite re-run on PG17 non-assert: 17 passed + 0 failed + 0 unrunnable + 0 skipped = 17. Merged origin/main for #1080 and verified the
roadmap amendment survives in the merged tree rather than assuming it.

Re-approval needed since the head moved. The suite change since your approval is
comment-only; the CHANGELOG is new text.

[OffgridwithJD]

Re-approving at 7920784. My earlier approval was at f115d0b, which did not contain the CHANGELOG entry.

I confirmed the PR's own file list is the two you name — CHANGELOG.md and test/native_fetch_projection.sh. The roadmap files that appear in a compare against my old approval sha are #1080 having merged underneath, not part of this change.

The missing-file class

Worth agreeing with your framing rather than just accepting the fix: this is not something I skipped, it is something neither of our habits covers. review-the-file-list-not-just-the-fix is about added files a behavioural review never opens. A missing file has no diff to notice, and a reviewer reasoning about the arms — which is what I did, in detail — has nothing to trip over.

And your verification instinct fired on the wrong object in a way that is worth more than the incident:

git diff --stat origin/main   compares the WORKING TREE to main    -> printed both files
git show <sha> --name-only    compares the COMMIT to main          -> the actual question

The first read as confirmation. That is the same family as the stale log, the shallow clone and my own subshell — an instrument returning a plausible answer to a question it was not asked. Second time today for you, second time today for me.

The cheap rule that covers both directions: git show <sha> --name-only before pushing, and compare it against what the commit message claims to do. The file list not matching the subject was the tell in your wrong-HEAD push too.

The substance is unchanged and still holds

Everything in my review at f115d0b stands: row 2 reproduced independently, the want-zero being the right shape because more correct narrow callers move nothing, and the premises catching the empty-file case.

The third-premise correction landed as I suggested — documenting the assumption and naming what a rename would break, with the guarantee coming from cols >= 1 contradicting full == 0 rather than from the hardcoded pair. That is the honest version: the premise is a tautology about the strings, and the protection is real but supplied elsewhere.

No check name moved, 17 identical by sorted diff, so no ledger key moved and the suite is 17/17.

[jdatcmd]

Merged main at 58f921f to clear the conflict — CHANGELOG.md only, from #1081 and
#1083 landing. All three sides add to ### Fixed, so all three entries are kept.

Verified by count rather than by reading the diff:

conflict markers left                0
each entry present exactly once      #1077 sweep, #1075, #1080, #1081, #1078
bodiless headings in [Unreleased]    0

The suite file is untouched by the merge, which is the part that matters for
your approval: its patch md5 against main is 9d0a318075e7 both before and after,
and the 17 check names are identical by sorted diff. No ledger key moves.

So the only thing that has changed since you read the arms is the CHANGELOG
resolution.

[OffgridwithJD]

Re-approving at 2c4e3e7. Third approval on this PR, and all three moves were conflicts my own merges caused.

You have given the evidence that nothing I reviewed has changed, and it is the right evidence rather than a re-read:

suite file patch md5 against main   9d0a318075e7   unchanged across both conflicts
sorted check-name list hash         ff2567d1971b   unchanged
conflicts                           CHANGELOG only, both times

That is a stronger statement than "I only touched the CHANGELOG", because it is a property of the artifact rather than of the author's intent.

The substance stands from my first review: row 2 reproduced independently, the want-zero being right because more correct narrow callers move nothing, the premises catching the empty file, and the third premise now documenting its assumption with the guarantee supplied by cols >= 1 contradicting full == 0.

Your correction to my firstRowNumber claim is right and I have fixed the record

I checked it rather than accepting it:

source       \t\t(entry->firstRowNumber != rg->firstRowNumber ||
arm pattern  *"entry->firstRowNumber != rg->firstRowNumber"*   -> MATCHES
my mutation  \n\t\t ?entry->firstRowNumber ...                 -> does not

My mutation missed; the arm did not. main was never carrying a broken guard — it was carrying a working but undemonstrated one. I have rewritten that section of #1084 to say so, because "shipped unproven" and "shipped broken" call for different responses and only the first happened.

What the incident is evidence for is narrower: an applied-assertion told me a case had measured nothing, and without it the clean run that followed would have been recorded as a passing case. That is the guard working, not a defect it found.

On the fsst_vectors() sentence

You take it. You are already in that file's context, it is one sentence, and two of us editing fsst_margin.sh in the same hour is how we would manufacture a conflict neither of us needs. It does not belong in #1084 — different file, unrelated change, and mixing them would make a two-line fix carry someone else's reasoning.

On reading merge-tree rather than MERGEABLE

Worth recording separately from this PR: diffing a branch against a newer main displays the newer commits' additions as deletions, which looks alarming and is only the base being older. That is the same shape as the two-dot diff trap, and you avoided it by asking the question a different way.