docs: pgcolumnar.compression picks encodings, not only a codec (#1074, #1076) - #1082
Conversation
…#1076) `columnar_encoding.c` returns "FSST helps" UNCONDITIONALLY when the codec is `none`, before it looks at the corpus or at the margin: if (compressionType == COLUMNAR_COMPRESSION_NONE) return true; The reasoning is right. Whether FSST is worth keeping depends on the size AFTER the codec, because what lands on disk is the encoded stream compressed, and with no codec the encoded length already IS the stored length. The consequence is that `none` is a DIFFERENT cascade rather than the same cascade minus a step, and nothing in `docs/` said so. `configuration.md` called the setting "default codec for new chunks", which is exactly the narrow reading that hides the coupling. TWO PUBLISHED CLAIMS ARE REFUTED BY MEASUREMENT, not by argument. `administration.md` described `none` as "Lowest write cost, largest size". On 200,000 rows per shape, as backend instruction counts, `zstd` against `none`: high-entropy hex text 0.92 zstd CHEAPER to write repetitive text 0.99 zstd CHEAPER to write Neither shape made `none` the cheaper one. Repetition spreads were 0.08% and 0.19%, so the 0.67% on repetitive text is small and outside the noise. On the hex shape `zstd` also produced a table 1.4 percent LARGER, because dropping FSST cost more than the codec saved. The pages say `none` is not RELIABLY cheapest and give both numbers. They do NOT claim zstd is cheaper in general: these are instruction counts on one box with `fsync` off, and an instruction count cannot see an I/O saving at all. `best-practices.md` said each chunk "takes the encoding that makes it smallest". Wrong twice. The target is the smallest STORED result, so it depends on the codec; and a FSST win below `fsst_min_gain_percent` is given up on purpose. ARMS IN BOTH HARNESSES, AND EVERY ONE IS A DIFFERENTIAL. "FSST is kept under `none`" on its own is satisfied by any corpus FSST always wins on, which would be green and empty. So each arm loads the same corpus at the same margin WITH a codec, where FSST is dropped, and the only variable is the setting under test. One arm exists only to assert the corpus is marginal rather than one-sided. `fsst_margin.sh` gains three checks. `test_compression_reaches_the_cascade.py` is independent -- its own corpus, its own descriptor parse, neither file naming the other. Removal proof, run on BOTH harnesses. Deleting the early return and rebuilding moves the `.so` `c8e5c790dbac` -> `6455728725e5`: fsst_margin.sh 14 passed + 2 failed + 0 unrunnable + 0 skipped = 16 pytest 8 pass + 2 fail + 0 unrun = 10 Only the codec arms redden. Every content invariant stays green on both sides, which is the right answer: the mutant still writes correct rows, so this is a decision changing rather than corruption. Five majors on this tree, own `make clean` and own `.so` each: PG15 16 PASS PG16 16 PASS PG17 16 PASS PG18 16 PASS PG19 16 PASS `cluster_tests` re-derived by collection 406 -> 410. `guard_tests` re-derived in the same run and did NOT move (342), which was checked rather than assumed. No ledger change: `fsst_margin` has 0 rows in main's ledger. TWO TOOLS CAUGHT ME AND BOTH WERE RIGHT. `plain_language_check.py` failed the first draft on six over-long sentences and two prose double-hyphens, in prose written to explain a measurement; rewritten to its rules rather than argued with. `test_docs_cover_the_corpus.py` caught the new pytest file having no numbered `TESTS.md` section, which is #1024's arm doing its job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK
…ntees TWO FIXES, one of them a hole in this PR that review did not look for. THE CHANGELOG ENTRY WAS NEVER COMMITTED. `f115d0b` contains only `test/native_fetch_projection.sh`. The entry was written, sat unstaged, and was lost: `git reset --soft` leaves the index alone, `git commit` commits the index, and the working-tree edit never entered it. I checked with `git diff --stat origin/main` AFTER committing, which compares the WORKING TREE to main rather than the commit to main, so it showed 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`, which reads the commit. Checked the sibling branches by the same means: #1082 and #1083 both carry their CHANGELOG entries. Only this one was affected. AND THE THIRD PREMISE'S COMMENT OVERSTATED ITS JOB. It read "the whole arm rests on it". It does not. A broken prefix relationship cannot pass silently, because the other two arms already contradict each other under it: if `Cols(` matched the wide pattern then every narrow call would be counted twice, so `full >= cols`, and `cols >= 1` with `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 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. Correction from @OffgridwithJD's review. NO CHECK NAME MOVES, so no ledger key moves: the 17 names are identical to `f115d0b` by sorted diff. Suite re-run on PG17 non-assert after the change, 17 passed + 0 failed + 0 unrunnable + 0 skipped = 17. Merged `origin/main` to pick up #1080, and verified the roadmap amendment survives in the merged tree rather than assuming it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK
The only conflict is CHANGELOG.md, and both sides add entries to `### Fixed`, so both are kept. #1080, #1081 and #1083 landed while this was open. Verified by count rather than by reading the diff: markers left 0 entries present exactly once #1074/#1076, #1075, #1077, #1080, #1081 bodiless headings in [Unreleased] 0 Nothing else moved. Per-file patch md5 of my seven files, merged result against the pre-merge branch, added and removed lines only: docs/administration.md 6a456b0edcac same docs/best-practices.md 0c84c03e77bd same docs/configuration.md 68b2705ebe8a same test/fsst_margin.sh 978e4e448429 same test/pytest/TESTS.md 787346d68405 same test/pytest/expected_tests.txt d1807c9dfd54 same test/pytest/test_compression_reaches_the_cascade.py 025da1ba7426 same main moved none of `expected_tests.txt`, `check_ledger.tsv` or `check_ledger_budget.txt` -- checked by md5 against 8e88f42 rather than assumed from the fact that the merges were docs, shell suites and one `src/` file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK
|
Merged main at Both sides add entries to That last one is not paranoia: the same resolution on #1062 would have shipped a Nothing else moved. Per-file patch md5 of the seven files, merged result Counts re-derived by collection on the MERGED tree, not carried forward from I also checked by md5 that main moved none of CI re-running at the new head. |
|
APPROVE at The corpus premise is bracketed by three points, not guarded by one armYou framed it as "there is an arm for exactly that". There are three points on the same corpus, and it is the pair that does the work: The first two bracket the win rather than asserting a point. So both drift directions redden:
A single arm at one margin would have covered one of those. The bracket covers both, and that is the answer to your question: I could not find a corpus drift that leaves the differential green and meaningless. And the bracket guards
|
OffgridwithJD
left a comment
There was a problem hiding this comment.
APPROVE. 14/14 green. The detailed review is in my comment above; this is the formal approval now that both legs have landed.
The short version of what I checked rather than read:
- The corpus premise is bracketed by three points, not guarded by one arm, so both drift directions redden. I could not find a corpus drift that leaves the differentials green and meaningless — which was the question you put.
- That bracket also falsifies
fsst_vectors()itself in both directions, which matters because it parses the encoding descriptor by byte offset. Worth a sentence in the comment so nobody removes the low-margin arm as redundant. 0, 99is really the GUC's range (columnar_tableam.c:3269), so the maximum-margin arm is the strongest form of that test rather than an arbitrary high value.- The docs correction replaces a claim your own arms refute.
noneas "Lowest write cost, largest size" was wrong in both halves.
CHANGELOG.md only, from #1082 landing. Both sides add to `### Fixed`. conflict markers left 0 each entry present exactly once 6 checked by name bodiless headings in [Unreleased] 0 The suite file is untouched: patch md5 against main unchanged, and the sorted check-name list hashes identically to 58f921f, so no ledger key moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK
`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
…o-falsifies-the-descriptor-reader test: say why the low-margin arm is not redundant (#1082 follow-up)
pgcolumnar.compressiondoes not only choose a codec. It also decides whichlightweight encodings your data gets, and we documented the opposite.
columnar_encoding.creturns "FSST helps" unconditionally when the codec isnone, before it looks at the corpus or at the margin:The reasoning is right — with no codec the encoded length already is the stored
length, so the per-vector test is measuring the right thing. The consequence is
that
noneis a different cascade, not the same cascade minus a step.Two published claims are refuted by measurement
docs/administration.mddescribednoneas "Lowest write cost, largest size".Measured on 200,000 rows per shape, backend instruction counts,
zstdagainstnone:Neither shape made
nonethe cheaper one. Repetition spreads were 0.08% and0.19%, so the 0.67% on repetitive text is small and outside the noise. On the hex
shape
zstdalso produced a table 1.4 percent larger, because dropping FSSTcost more than the codec saved.
What the pages deliberately do NOT say. They do not claim zstd is cheaper in
general. These are instruction counts on one box with
fsyncoff, and aninstruction count cannot see an I/O saving at all — the same limit
@OffgridwithJD established on #890. The wording is "not reliably the cheapest",
with both numbers shown.
docs/best-practices.mdsaid each chunk "takes the encoding that makes itsmallest". Wrong twice: the target is the smallest stored result, so it
depends on the codec; and a FSST win below
fsst_min_gain_percentis given up onpurpose.
Arms in both harnesses, every one a differential
"FSST is kept under
none" on its own is satisfied by any corpus FSST always winson — green and empty. So each arm loads the same corpus at the same margin with
a codec, where FSST is dropped, and the only variable is the setting under test.
One arm exists only to assert the corpus is marginal rather than one-sided.
fsst_margin.shgains three checks.test_compression_reaches_the_cascade.pyisindependent: its own corpus, its own descriptor parse, neither file naming the
other.
Removal proof, on both harnesses
Deleting the early return and rebuilding moves the
.soc8e5c790dbac→6455728725e5:Only the codec arms redden. Every content invariant stays green on both sides,
which is the right answer — the mutant still writes correct rows, so this is a
decision changing rather than corruption.
Five majors, this tree
Own
make clean, own build and own.soeach:cluster_testsre-derived by collection 406 → 410.guard_testsre-derived inthe same run and did not move (342) — checked rather than assumed. No ledger
change:
fsst_marginhas 0 rows in main's ledger.Two tools caught me
plain_language_check.pyfailed my first draft on six over-long sentences and twoprose double-hyphens, in prose written to explain a measurement. Rewritten to its
rules rather than argued with. And
test_docs_cover_the_corpus.pycaught the newpytest file having no numbered
TESTS.mdsection, which is #1024's arm doingexactly its job.
Closes #1076. Addresses the documentation half of #1074; its remaining half is the
measurement, which stays on the issue.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK