fix: do not store a validity bitmap for a chunk that holds no null (#1130) - #1140
Conversation
OffgridwithJD
left a comment
There was a problem hiding this comment.
Reviewed against your three questions. The C is careful and the format work is documented
properly; I went after the tests, since that is what you pointed at.
One blocker, and it is in the arm your own comment calls the only one that proves the
headline property. It is one line per harness.
The per-chunk arm passes on a population that does not exist
a null-free column elides its bitmap beside a null-bearing one in the same row group is,
as the comment above it says, the only arm that distinguishes a per-CHUNK decision from a
per-ROW-GROUP one. It compares nulls_key_res against 0 — and 0 is also what
coalesce(sum(...), 0) returns over an empty set.
You anticipated exactly this and wrote the premise for it:
check "premise: the residual was summed over chunks that exist" <- guards full_res
but full_res is the one residual that needed it least, because a bug there tends to
produce a large number. The two residuals with no population premise are nulls_res,
which is compared against 12500 and so protects itself, and nulls_key_res, which is
compared against 0 and does not.
Measured, not argued. I pointed that one residual at a column index that cannot exist and
changed nothing else:
CONTROL validity_residual ve_nulls 0 key_residual=0 PASS 24 passed + 0 failed
MUTATED validity_residual ve_nulls 99 key_residual=0 PASS 24 passed + 0 failed
The suite is green against a column index that does not exist. The arm cannot tell "the
bitmap was elided" from "I measured no chunks at all".
The pytest twin has it identically, which is the part I would not have predicted —
the helper states the contract and the call sites do not keep it:
THE COUNT IS RETURNED BESIDE THE SUM because a sum over nothing is 0, which
is exactly what the headline arm wants to see. A residual of 0 is evidence
only together with the number of chunks it was summed over.full_res, full_chunks = _residual(c, "vb_full")
nulls_res, _ = _residual(c, "vb_nulls")
key_res, _ = _residual(c, "vb_nulls", KEY_COLUMN) # <- the 0 comparisonSame mutation, KEY_COLUMN = 0 -> 99:
CONTROL 24 checks, 4 passed
MUTATED 24 checks, 4 passed
The fix is the premise you already wrote, applied to the third call: chunks_measured ve_nulls 0 in the shell suite, and keeping the count instead of _ in the twin. I would
put it beside the arm rather than with the other premises, so the two move together.
Your question 2: I did not find a variant of the reverted mistake
The reverted idea was bounding a SYNTHESIZED all-ones bitmap by the row group's stored
byteLength, which refuses a correct table precisely because elision makes the group
smaller than its bitmap. Every surviving bound I can find compares a stored bitmap
against pageLength, which is the right bound and cannot have that failure:
columnar_reader.c cc->pageLength < (uint64) cvb (coalesced fetch)
cc->pageLength - cvb > PG_UINT32_MAX
cc->pageLength >= (uint64) cvb
and for an elided chunk cvb is 0, so those are vacuously satisfied rather than
wrongly restrictive. The synthesized path is bounded by MaxAllocSize and by the
accounted != rg->rowCount equality — neither of which is a stored size. Your comment
at the need > MaxAllocSize site records the reasoning; I would keep that comment
exactly as it is, because it is the thing that stops the next person re-deriving the
same wrong bound.
Your question 3: instruction count rules out the usual suspect
The usual explanation for a few percent moving under a trivial code change on this box is
code layout — alignment shifting a hot loop across a boundary, link order, even the size
of the environment block. That explanation is not available to you, and that is the
useful part: instructions retired is insensitive to layout. Alignment changes cycles,
not the number of instructions executed. A 7.9% move in instruction count means the
machine really is executing fewer instructions, so the difference is semantic and should
be visible rather than mysterious.
Two places it can come from, and they are distinguishable:
- Codegen. An explicit bound can let the optimiser prove a range and drop later
redundant checks, or unlock a vectorised form of the loop. This is static:objdump -d
the one function in both builds and diff the instruction counts of the hot loop. If the
loop body shrank, you have your mechanism and it is a real saving. - A different dynamic path. The check short-circuits work that the unchecked build
performs. This shows as a different call count, not a different loop body:perf stat
the two arms forbranchesandbranch-missesalongside instructions, and count calls
to the functions below it.
If (1), "the check is not a cost" understates it and you can say why. If neither, I would
keep your current wording — reporting the direction without a mechanism is the honest
form, and you were right not to claim a saving.
To answer what you actually asked: I have seen unexplained swings of this size on this
box, but on the CLOCK, not on instruction count — a byte-identical dsm round-trip that
measured 26% slower. I have not seen instruction count move without a mechanism, and I
would not expect to.
Everything else I checked and found sound
sqlstate() pins XX001 (ERRCODE_DATA_CORRUPTED) rather than grepping ERROR,
so a FATAL, a 42501 or a missing function cannot satisfy it
guard 1 fixture ve_nulls' v column holds 90% of rows, so setting NO_VALIDITY on it
is the exact lie the guard names
guard 2 poisons row_count rather than the descriptor, so it exercises the
refuse-before-allocate ordering the comment claims
premise: codec read back from SHOW rather than assumed from ALTER DATABASE
premise: groups asserted, so ceil(rows/8) really is the whole expected bitmap
One small thing, no action needed unless it is free: groups="$(row_groups ve_full)$(row_groups ve_nulls)"
compared against "11" concatenates two counts with no separator, so 1+1 and
11+"" are the same string. It cannot bite at this fixture size, but "$a|$b" against
"1|1" costs nothing and says what it counted.
CI is green on 12 of 14 with the two suites jobs still running. Happy to re-review as
soon as the population premise is on that third residual — and I will run the same
mutation against it, which is the check I would want against my own fix.
51bf7df to
5b5e590
Compare
Review fix, with the mutation that proves it@OffgridwithJD found the one arm in this branch that could not fail, and it was the most important one: Fixed by giving that arm its own premise, beside it rather than with the other five so the two move together: The removal proof is the reviewer's own mutation, run here.
The reusable distinction, which is now in Ledger re-seeded from five fresh runs merged in one call, so all 25 rows carry The instruction-count paragraph is weakened, not defendedThe objdump answer is in: My own instrument is now the first suspect: it pins the Composing with #1136, measured rather than predictedI composed this branch with
Derived on the composed tree so whoever merges second does not have to: Both PRs also claim |
|
Full PG17 matrix on |
5b5e590 to
3351586
Compare
|
Correction to my compose comment above. I listed #1136 is pytest-only. Those three files merge quietly because only one side writes them, which is not the failure mode — that needs two sides each writing a plausible value. The census key is this branch's, uncontested. What survives, and is live: Also pushed,
The pytest twin never had cell C: one call there returns the residual and its chunk count together. The shell now says the same thing. |
The instruction gap: three hypotheses excluded, mechanism still unknownPer-arm capture, both arms on the same tree and the same build directory, differing only in the five-line bound: So:
What remains is a reproducible 7.9% difference in instructions retired between two builds that execute the same plan over the same rows and produce the same answer, with the bounded build lower. I have no mechanism for it. The CHANGELOG says exactly that — the number bounds the check's cost from above and nothing else. Branch counts beside instructions are running now; if they do not separate the arms either, the honest end state is a measured fact with no explanation, recorded as such rather than dressed up as a saving. |
|
Correction to my own claim about cell C, two comments up. I wrote that after 25 passed + 0 failed. Green over a population that does not exist. What the variable actually buys is narrower and still worth having: the desync is no longer reachable by changing the column, which is the edit someone would really make. A deliberate replacement of the variable with a literal still reaches it, and nothing structural in shell closes that. The pytest twin does close it, and for a reason worth naming because it is the transferable part: The file will be read as precedent for the next suite that needs a population premise, so the claim in it should be the one the change earns. |
…1130) A column chunk's page was always [validity bitmap][encoded values]. The bitmap is one bit per row and is written RAW, ahead of the block codec, which therefore never saw it: a NOT NULL column, or one that simply holds no nulls, still stored ceil(rows / 8) bytes of 0xFF for ever. The writer now omits it for a chunk that holds no null and records that in the encoding descriptor's new flags byte. Measured on ClickBench hits_0.parquet (1,000,000 rows, 105 columns, no null in any of them) through pgcolumnar.import_parquet on PG17, both arms into the same cluster on the same day: stored chunk bytes 78,109,810 -> 64,984,810 -16.80% validity bytes 13,125,000 -> 0 relation size 78,381,056 -> 65,216,512 chunks with no bitmap 0 of 735 -> 735 of 735 The saving is exactly the bitmap: -13,125,000 bytes, which is 105 columns x 125,000. Nothing else moved. DECIDED FROM THE ROWS WRITTEN, not from the attribute's NOT NULL flag, so a nullable column whose rows happen to be complete gets the saving too and a constraint added later cannot make an already-written chunk lie. THE FORMAT MOVES: descriptor version 2 -> 3, spending the byte version 2 wrote as a zero reserved byte. Every field keeps its offset, so a version-2 descriptor is a version-3 one whose flags are clear; readers accept 2 and 3 and no conversion is needed. Downgrading a binary below the one that wrote the table is not supported, and the shape of that failure is measured rather than asserted: an alpha4 binary raises `unrecognized native encoding descriptor` on every sequential scan, but an index fetch of a chunk wider than the bitmap returned NULL for a row that holds a value in 16 of 40 single-row fetches. #1137 tracks the versioning gap that makes the silent case possible. THE BITMAP'S SIZE IS NOW A PROPERTY OF THE CHUNK, NOT THE ROW GROUP, and three readers had to learn it. The first implementation taught only the sequential scan and the PG17 matrix went red in 33 suites, with native_index's point lookup returning NO ROW for a row that is there. The two paths are correlated rather than independent: pgcolumnar_fetch_coalesce_read skips any chunk whose page_length is below the group's bitmap size, and eliding the bitmap is exactly what takes a well-encoded chunk below it, so the chunks this helps most are the ones that fall to the per-column fetch path. All three now call one helper, which is also what keeps the coalesced read and the per-column loop agreeing about a pointer and a length one of them computed. A HARDENING GAP THE CHANGE'S OWN COMMENTS FORCED INTO THE OPEN. The scan's fast path for fixed-width by-value types read a value without checking it against the end of the stream, trusting the bitmap to stop first; a synthesized all-ones bitmap is a new way past it, so that path now carries the bound the general path has always had. Two guards refuse the catalog that would get there. The bound costs nothing measurable: 6.81e9 backend instructions with it against 7.40e9 without, reproduced across two build directories with the arms interleaved -- 7.9% LOWER with the check, a direction this commit does not try to explain and does not claim as a saving. test/validity_elision.sh and test/pytest/test_validity_elision.py, 24 checks each, green on PG15/16/17/18/19. Three mutations were run: forcing the writer's elision decision false reddens one arm, making the reader ignore the flag reddens ten, and using the group-wide size in the fetch path reddens exactly the three fetch arms and nothing else. Two arrangements in those suites are load-bearing and both were found by an arm that failed rather than by reasoning -- a key column beside the measured one, because an index on the only column is answered by an Index Only Scan that never calls the table AM, and pgcolumnar.enable_custom_scan = off, because enable_seqscan does not govern the columnar custom scan. One attractive guard was implemented and reverted: bounding the synthesized bitmap by the row group's byteLength refuses a CORRECT table, because an elided group of 360 bytes legitimately needs 12,500 bytes of bits. Ledger rows seeded from five real runs merged in one call, so each carries 15;16;17;18;19; suites_not_covered does not move because registering the suite and seeding it happen in this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK
3351586 to
73cc36a
Compare
Q3, one level further: same control flow, different instruction mix@OffgridwithJD asked for the branch count beside the instructions, on the grounds that a 197-instruction-per-row gap with equal branches would be the odd case. It is the odd case. Equal branches means equal control flow: the unbounded build is not executing more iterations of anything. It retires 590 million more instructions over the same number of branches, on the same plan, the same 1,000,000 rows, the same 7 chunk groups and 100 decoded vectors, returning the same checksum. So the gap is the instruction mix in straight-line code, which is a compiler codegen difference rather than a difference in work. Every hypothesis with a mechanism is now excluded:
I am stopping the investigation here and recording the result as what it is. The CHANGELOG (pushed, |
|
Full PG17 matrix on |
OffgridwithJD
left a comment
There was a problem hiding this comment.
Approving. The blocker is closed and I verified the fix by running the mutation myself
rather than reading the report.
What I ran, on 3351586
A control 25 passed + 0 failed (shell)
25 pass + 0 fail (twin)
B VE_KEY_COL 0 -> 99 24 + 1 FAILED, the premise, nothing else
23 + 1 fail, same
premise: the per-chunk arm's residual was summed over chunks that exist:
got 'measured-nothing' want 'measured'
Carried forward to the head I am approving. 73cc36a differs from 3351586 by
CHANGELOG.md alone; test/validity_elision.sh and test/pytest/test_validity_elision.py
are byte-identical (md5 8436a4d6ede5 and ea33756ef103 on both), so the cells above
still describe this tree.
CI verified by SHA rather than by the PR view: 14 of 14 check-runs completed success,
non-green 0, mergeable=MERGEABLE state=CLEAN.
One thing recorded, not blocking
C', the deliberate desync, still reaches a green suite over an empty population:
nulls_key_res="$(validity_residual ve_nulls 99)"
nulls_key_chunks="$(chunks_measured ve_nulls $VE_KEY_COL)"
-> 25 passed + 0 failed
Which is why the trimmed claim on the PR is the right one: the variable makes the desync
unreachable by changing the column, the edit someone would really make, and that is
what it earns. The transferable part is the twin's, and you have already written it —
key_res, key_chunks = _residual(...) closes it because one call cannot return two
populations. That is a property of returning the measurement together with its
population, not of naming a variable, and it is the shape I would copy into the next
suite that needs a population premise.
The rest, checked rather than taken
four version-check call sites, not two reader.c:560, reader.c:4292,
vector.c:2916, vector.c:5143
coalesced fetch reader one caller, line 4473, inside
pgcolumnar_fetch_row (4235..4737),
181 lines after the stamp check
surviving size bounds all compare a STORED bitmap against
pageLength; cvb is 0 for an elided
chunk, so they are vacuously satisfied
rather than wrongly restrictive
sqlstate() pins XX001, so a FATAL or a 42501
cannot satisfy it
I did not find a variant of the reverted byteLength bound. That part is inspection over
the diff rather than mutation, so treat it as weaker evidence than the cells above.
The composed count, for whoever merges second
Derived independently — composed the two branches, unioned COMPLETE to 24 stems, and
collected:
guard_tests 380
cluster_tests 430
Same number you reached a different way, and it reconciles: main 423, +3 from #1136, +4
from here. Whoever goes second writes 430 fresh rather than trusting either derivation.
Your correction of the compose note was the right call — check_ledger.tsv,
check_ledger_budget.txt and run_all_versions.sh are touched by this PR alone, so they
merge quietly because there is one side, which is not the trap. The trap needs two sides
each writing a plausible value.
Q3
Equal branches (1.679e9 against 1.676e9) with 590M more instructions retired, same plan,
same actual rows, same checksum, is a genuinely strange result and publishing the branch
figures with the mechanism named as unidentified is the honest form. I have nothing to add
and would rather you left it unexplained than filled it in.
Closes #1130.
A column chunk's page was always
[validity bitmap][encoded values]. The bitmap is one bit per row and is written RAW, ahead of the block codec, which therefore never saw it — so aNOT NULLcolumn, or one that simply holds no nulls, still storedceil(rows / 8)bytes of0xFFfor ever.The writer now omits it for a chunk that holds no null, and records that in the encoding descriptor's new flags byte.
Measured
ClickBench
hits_0.parquet— 1,000,000 rows, 105 columns, no null in any of them — throughpgcolumnar.import_parqueton PG17. Both arms loaded into the same cluster on the same day, rather than one of them being a number from an earlier session:21aa465)The saving is exactly the bitmap: −13,125,000 bytes = 105 columns × 125,000. Nothing else moved.
Decided from the rows written, not from the attribute's
NOT NULLflag. A nullable column whose rows happen to be complete gets the saving too, and a constraint added later cannot make an already-written chunk lie.The format moves: descriptor version 2 → 3
Version 3 spends the byte version 2 wrote as a zero reserved byte. Every field keeps its offset, so a version-2 descriptor is a version-3 one whose flags are clear: readers accept 2 and 3, writers emit 3, and tables written by an older build keep reading with no conversion.
Downgrading a binary below the one that wrote the table is not supported, and the shape of that failure is measured rather than asserted. An alpha4 binary reading a table this build wrote:
ERROR: unrecognized native encoding descriptor, every timeERROR: validity bitmap longer than the chunkThe last row is why #1137 exists: the old reader tests a bit in bytes that are not a bitmap before it reaches any version check, and
pgcolumnar.storage.format_version— the stamp that runs early on both paths and could refuse the table outright — is checked for equality, so bumping it would also make this build refuse every table alpha4 wrote. That is a decision about the versioning model rather than about this change, so it is filed rather than smuggled in here.Three readers, not one, and they are correlated
The first implementation taught only the sequential scan that the bitmap's size is now a property of the CHUNK rather than of the row group. The PG17 matrix went red in 33 suites, with
native_index's point lookup returning NO ROW for a row that is there.The two paths are not independent:
pgcolumnar_fetch_coalesce_readskips any chunk whosepage_lengthis below the group's bitmap size, and eliding the bitmap is exactly what takes a well-encoded chunk below it. So the chunks this change helps most are the ones that fall to the per-column fetch path — a fixture built to exercise the elision is systematically the fixture that lands on the reader most likely to have been left behind. All three readers now call one helper, which is also what keeps the coalesced read and the per-column loop agreeing about a pointer and a length that one of them computed.A hardening gap the change's own comments forced into the open
The scan's fast path for fixed-width by-value types (
columnar_reader.c, the#289inline) read a value without checking it against the end of the stream — it trusted the validity bitmap to stop first. A synthesized all-ones bitmap is a new way to reach past it, so that path now carries the bound the general path has always had. Two guards refuse the catalog that would get there: a descriptor claimingNO_VALIDITYwhile accounting for fewer values than the group has rows, and a row count whose bitmap would not fit in memory.The bound costs nothing measurable. Backend instructions for
sumover a 1,000,000-row bigint column,cpu_core/instructions/pinned to the P-cores, three repetitions per backend, two backends per arm:The bounded build is 7.9% lower, reproduced across two build directories with the arms interleaved. It is not claimed as a saving, and the direction is unexplained. Everything with a mechanism has since been excluded by measurement, not by argument (see the comments below):
actual rows=1000000, same chunk groups and vectors decodedEqual branches with 590M more instructions retired means equal control flow and a different instruction mix in straight-line code. What the measurement supports, and all it supports, is that the bound is not a cost.
One attractive bound is wrong, and this branch is the proof. Bounding the synthesized bitmap by the row group's own
byteLength— a stored bitmap cannot exceed the bytes the group occupies — was implemented, and it refused a correct table:ve_full's group is 360 bytes on disk and needs 12,500 bytes of synthesized bits. That gap is the saving, not a defect. Reverted, and the reason is recorded where the next person will look for it.Tests
test/validity_elision.shandtest/pytest/test_validity_elision.py, 25 checks each, green on PG15/16/17/18/19.compare_to_bash.pygrades the pairmissing: 0.The 25th arrived from @OffgridwithJD's review and is the one worth reading first: the per-chunk arm expects
0, and0is also what the residual returns when it sums nothing. See the review-fix comment below for the mutation that proves it, and for what a shared variable does and does not buy over a shared literal.Five of the 24 are premises, and each answers a way the headline arm reads
0without measuring anything:coalesce(sum(...), 0)returns 0 over an empty set (so the chunk count is asserted beside it);ceil(rows/8)is one group's bitmap (so the group count is asserted); and the subtraction is only the bitmap while the block codec is off (so the setting is read back rather than assumed).One arm exists because both size arms are satisfied by a per-ROW-GROUP decision — one fixture's group holds no null anywhere, the other's holds some. The null-bearing fixture carries a null-free key column in the same row group, so only a per-chunk decision elides one and keeps the other.
Three mutations, each asserted to have applied (the
.somd5 moved) and to have been restored (the control run reproduced the original md5 byte for byte):presentCount == rowCountforced falsePgColumnarEncdescOmitsValidityreturns falseTwo arrangements in those suites are load-bearing, and both were found by an arm that failed rather than by reasoning:
Index Only Scanthat never calls the table AM at all — the first version of the fetch arms passed against a build whose fetch path was provably broken;pgcolumnar.enable_custom_scan = off, becauseenable_seqscandoes not govern the columnar custom scan and the plan was otherwiseCustom Scan (PgColumnarScan), a scan wearing a fetch's name.The premise arm asserts the PLAN NODE rather than the row, for the reason @OffgridwithJD put well in review of the mechanism: a row that comes back says nothing about which reader produced it.
Derived artefacts, each re-derived on the rebased tree
check_ledger.tsv: 14 rows, seeded from five real runs merged in ONE call, so each carries15;16;17;18;19and the short-major warning stayed silent. Eleven of the fourteen carry an observed red from the mutations above.check_ledger_budget.txt:checks_never_observed_red1407 → 1410, re-derived by counting.suites_not_covereddoes NOT move — registering the suite takes registered 260 → 261 and seeding it takes covered 11 → 12, which is why the seeding is in this change rather than after it.expected_tests.txt:cluster_tests423 → 426 by collection;guard_testsre-derived in the same run and did not move (380).Neighbours that read the descriptor
encode_post_codec(both halves) subtractedceil(rows/8)frompage_lengthunconditionally to get the value stream. With the bitmap gone that removes bytes that were never written, so both now subtract only where the flag says there is one.native_encdesc_goldenpins the version byte at 3 and adds a per-column arm on the flags byte — per column, because the flag is a property of the chunk. Four other descriptor readers had comments calling byte 1 "a reserved byte"; they are corrected. Entry field offsets do not move, which is whynative_dict_underfill's byte-offset reads at 6 and 11..14 are untouched and passed throughout.The one open decision, filed rather than smuggled in
#1137:
pgcolumnar.storage.format_versionis the stamp that runs early on both read paths and could refuse a future descriptor outright, which is what would have made the downgrade case loud everywhere. It is checked for equality, so bumping it would also make this build refuse every table alpha4 wrote — the fix is a range check plus a bump, and it changes the on-disk format's identity (PGCN v1) across the spec and the user docs. That is a decision about the versioning model, not about eliding a bitmap, so it is filed with the measurement rather than taken here.#1139: alpha5's named item, cascading, still has no measurement that favours it. Filed with the gate the plan itself states, what has changed under it since (the post-codec arbiter from #1132, and descriptor v3 being spent here so a chain entry is v4), and the measurement still owed.
Verification, so a reviewer can start from what is already done rather than repeat it.
ALL VERSIONS PASSED, 256 of 261 suites ran, 5 skipped, 0 incomplete. The ledger gate in the same run:census stated 1420, ledger holds 1420: they agreeandcoverage: registered=261 | covered=12, not covered=249, ceiling=249.5b5e590,3351586,73cc36a.shellcheck -S errorandcompare_to_bash.py(missing: 0).🤖 Generated with Claude Code
https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK