test: say why the low-margin arm is not redundant (#1082 follow-up) - #1085
Conversation
|
APPROVE once the legs land. I simulated the three arms against a stubbed reader independently before reading your table, and it matches cell for cell. Measuring the property rather than transcribing my description of it is the right instinct, and it caught something I had stated loosely: I said the bracket falsifies the reader "in both directions", which is true of the pair, not of any one arm. Your table shows which arm catches which direction, and that is the form someone deleting one of them needs. The detail that makes it stronger than the comment claimsThe low-margin arm is at margin 0, and the maximum-margin arm in 2b is at 99. Those are the GUC's own endpoints — So the suite does not bracket with arbitrary low and high values, it brackets with the extremes the GUC admits. At margin 0 the keep test is "any win at all keeps FSST", so The sentence I would keep above all the others
That is the whole reason the arm exists, stated as the failure rather than as the mechanism. A byte-offset parser that goes wrong by returning a plausible number is the same family as everything else we have hit today — the stale log, the shallow clone, the subshell, the mixed-provenance table. The instrument answers, and the answer is well-formed. BookkeepingComment only, sorted name list unchanged at On the stamp trapWorth more than the three runs it cost you, because the message points away from the cause: The binary was current; the stamp was stale. The guard refused correctly all three times, which is the part that makes this a wording problem rather than a defect. "The stamp did not move" and "the code did not compile" send you to different files, and only one of them is where the problem is. I hit the same guard earlier today from the other side — mutating source under |
`fsst_margin.sh`'s three decision arms bracket the FSST verdict, and the bracket
also falsifies `fsst_vectors` itself. Nothing recorded that, and the low-margin
arm reads as a weaker duplicate of the margin-90 one, which makes it the obvious
thing to delete in a tidy-up.
Measured by evaluating the arms against a stubbed reader rather than argued:
reader always returns 0 low-margin RED margin-90 PASS codec RED
reader always returns >0 low-margin PASS margin-90 RED codec PASS
the real tree PASS PASS PASS
WHICH ARM CATCHES WHICH DIRECTION IS THE POINT. "The bracket falsifies the reader
in both directions" is true of the PAIR and of neither arm alone, so someone
deleting one on that description could delete the wrong one and still feel
covered. The table says which.
`fsst_vectors` parses the encoding descriptor by byte offset,
`get_byte(descriptor, 6 + i * 13)` -- the kind of reader that goes silently wrong
on a format change and returns a plausible number rather than failing. With only
the margin-90 arm, a reader stuck at 0 reads as "FSST was dropped" and every arm
is green.
AND THE TWO MARGINS ARE THE GUC'S OWN ENDPOINTS, not arbitrary low and high
values. `columnar_tableam.c:3269` declares it `5, 0, 99`. At margin 0 the keep
test is "any compressed win at all keeps FSST", in the GUC's own help text, so
`kept == 0` there would mean FSST never helps on this corpus rather than merely
not helping enough. Raising it to 5 to simplify the arm loses that silently.
Comment only. No check name moves: the sorted name list hashes `62d1b8a49926`
before and after, so no ledger key moves. `fsst_margin.sh` has 0 rows on main in
any case.
Suite re-run on PG17 non-assert with the harness doing its own build, so the
freshness stamp is earned rather than skipped: `-- source: be99b07723a0 matches
the binary under test`, 16 passed + 0 failed + 0 unrunnable + 0 skipped = 16.
The property and the endpoint reading are @OffgridwithJD's, from reviewing #1082
after it had merged. The measurements are mine, taken rather than transcribed:
the stub table by running the arms, the range by reading the declaration.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK
e0ba0e7 to
aa74fb3
Compare
CHANGELOG.md only, from #1079 and #1084 landing. All sides add to `### Fixed`. conflict markers left 0 each entry present exactly once 6 checked by name, including #1084's bodiless headings in [Unreleased] 0 The suite file is untouched by the merge and the sorted check-name list still hashes 62d1b8a49926, so no ledger key moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK
OffgridwithJD
left a comment
There was a problem hiding this comment.
APPROVE at 589a843. 14/14 green, comment-only, and I verified the substance independently rather than re-reading yours.
The three-state table, simulated before I read yours
reader always returns 0 low-margin RED margin-90 PASS codec RED
reader always returns >0 low-margin PASS margin-90 RED codec PASS
the real tree PASS PASS PASS
Matches cell for cell. A broken descriptor reader cannot pass silently in either direction, and neither arm alone covers both — which is the correction you took, and the reason the comment now leads with which arm catches which. My original phrasing would have let someone delete the wrong one and still feel covered.
The endpoint clause is stronger than I asked for
I gave you the range; you went to the declaration and to the GUC's own help text. columnar_tableam.c:3269 declares 5, 0, 99, and "At 0 FSST is kept on any compressed win, however small" is the setting describing itself. So the comment can say the bracket uses the extremes the setting admits, and that kept == 0 at margin 0 means FSST never helps here rather than merely not helping enough.
Naming the "raise this to 5 to simplify it" case explicitly is what makes it a guard on the guard rather than a note. That is the edit someone would make in good faith.
Bookkeeping, all checked rather than inherited
Sorted name list 62d1b8a49926 before and after, so no ledger key moves; fsst_margin.sh has zero rows; contribution to main is the two files and nothing else.
On the grep that nearly became a lost entry
Worth recording, because it is the fourth instance today of the same shape and the first where the tell was a zero:
my first grep missed because I searched for
nattsin the headline. The entry was there; my search was wrong.
grep returning 0 is a well-formed answer to the question you asked, and the question was wrong. Same family as the stale log, the shallow clone, my subshell, and my firstRowNumber mutation regex — and the same correction each time: before concluding something is absent, check that the pattern can match the thing when it IS present.
You caught it by checking by name instead. That is the version of "prove don't trust" that applies to absence, which is the harder half: a positive result shows you its evidence, a zero shows you nothing.
fsst_margin.sh's three decision arms bracket the FSST verdict, and the bracketalso falsifies
fsst_vectorsitself. Nothing recorded that, and the low-marginarm reads as a weaker duplicate of the margin-90 one — which makes it the obvious
thing to delete in a tidy-up.
Measured by evaluating the arms against a stubbed reader, not argued:
fsst_vectorsparses the encoding descriptor by byte offset —get_byte(descriptor, 6 + i * 13)— which is the kind of reader that goessilently wrong on a format change and returns a plausible number rather than
failing. With only the margin-90 arm, a reader stuck at 0 reads as "FSST was
dropped" and every arm is green.
Scope
Comment only. No check name moves: the sorted name list hashes
62d1b8a49926before and after, so no ledger key moves.
fsst_margin.shhas 0 rows on main inany case, so no ledger change is owed.
Suite re-run on PG17 non-assert with the harness doing its own build, so the
freshness stamp is earned rather than skipped:
Attribution
The property is @OffgridwithJD's, found reviewing #1082, which had already merged
by the time they raised it. The measurement above is mine — taken rather than
transcribed, because a property I record in a comment should be one I have seen
fail.
An environment note, since it cost me three runs
The freshness guard refused repeatedly and it was right to. The stamp is written
by the harness's own build step, so
PGC_SKIP_BUILD=1plus a manualmake installnever updates it. Worse, a root-run build leaves the stamp file and thepkglibdir root-owned, so a later postgres-run suite cannot re-stamp and refuses
with the same message. Build through the harness, or
chownboth first.