feat: decode honours the skip vector it already builds (#452 phase 1b-i) - #539
Merged
Conversation
"Columnar Vectors Skipped" never meant what its name suggests. Decode ran before the skip vector existed -- pgcolumnar_native_decode_chunk at columnar_reader.c:1598, pgcolumnar_native_build_skipvec at :1615 -- so a vector the zone maps had ruled out was decoded in full and merely not turned into Datums. The counter said "not emitted", never "not decoded", and no existing counter could tell those apart. Nothing about that ordering was necessary. The vector count lives in the encoding descriptor header, which is chunk metadata already in hand, and build_skipvec's only other inputs are the predicates and the zone map catalog. So a metadata-only pre-pass reads the count and decides allDescriptor without touching a value byte, the skip vector is built before the decode loop, and decode takes the mask. A skipped vector leaves a hole in the raw buffer. That is safe only because nothing reads it: the row producer steps its cursors past skipped vectors, and the fold refuses a group whose decode skipped anything (#512, #523), which is the guard this change makes load-bearing rather than theoretical. Adds "Columnar Vectors Decoded" beside "Columnar Vectors Skipped", because neither number means anything alone -- "Skipped: 30" beside "Decoded: 32" is the honest reading of the old behaviour, and "Skipped: 30" alone invited the reader to assume the work had been avoided. The counter is reported from the loop that does the work and is never derived from the mask. That is not a stylistic preference. It was written the derived way first, and the suite passed with the skip removed -- the counter would have reported the saving whether or not decode honoured the mask. test/native_vecdecode.sh, 16 checks. Proved by removal, twice: - decode ignoring the mask turns "a ruled-out vector is not decoded" and the arithmetic check red. - a HALF-working mask, skipping only the first ruled-out vector, still PASSES "decodes fewer" and is caught only by "decoded plus skipped is the group's vectors". That is why the exact check is there rather than an inequality. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WmQJqcXdwyuoAiHHt2znBr
…phase 1b-i) Completes the previous commit, which was not shippable on its own. Teaching decode to skip put holes in the packed value stream, and #523's tripwire fired exactly as it was written to: the vectorized fold reads that stream directly and re-checks every value, so it would have re-checked uninitialised memory. The fold now walks the per-vector map it was already handed. Skipping is not merely safe there, it is exact: pgcolumnar_batch_shape_eligible requires every qual to be convertible to a scan key, and those keys are what the reader built the skip vector from, so a vector the zone maps rule out holds no row the fold would have counted. The present index still advances across a skipped vector, as it does across a deleted row, because the stream is packed by presence. What replaces the tripwire is narrower, not absent: if the reader reports a skip without the per-vector map, refuse rather than guess. ASSERT BUILDS NOW POISON A SKIPPED VECTOR'S BYTES with 0xA5, and that is the substance of this commit rather than a debugging aid. Reading a hole is undefined behaviour whose symptom is data-dependent: whatever palloc last left there is normally rejected by the fold's own scan-key recheck, so a fold that reads every hole returns the RIGHT answer on ordinary data. It was measured, not reasoned about -- with the skip removed from the fold, all 20 checks passed. The bug was unfalsifiable. A fixed poison makes it deterministic. 0xA5 as an int4 is -1515870811, and the suite adds a predicate chosen to accept that value while still letting the zone maps rule most vectors out. With the fold's skip removed that check now reports 32,696 rows where heap says 3,000. test/native_vecdecode.sh is 23 checks. Proved by removal, three ways: - decode ignoring the mask: the arithmetic check goes red. - the fold ignoring the mask: only the poison check catches it. - the present index not advancing over a skipped vector: the deep-range check returns 0 rows, the shallow one merely returns a wrong count. That is why the deep range is there. Gate: PG17 assert 131 ran 5 skipped, all pass. PG19 assert 136 ran, one failure, temporal, which is btree_gist missing in this container and fails identically on unmodified main; #448 made that a failure by design and PGC_ALLOW_MISSING_BTREE_GIST=1 is its override. Refs #452, #512
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#452 phase 1b-i, specified in
design/ISSUE_452_LATE_MATERIALIZATION.md.What was wrong
Columnar Vectors Skippednever meant what its name suggests. Decode ranbefore the skip vector existed:
So a vector the zone maps had ruled out was decoded in full, and the skip only
stopped it being turned into Datums. The counter said "not emitted", never "not
decoded", and no existing counter could tell those apart.
Nothing about that ordering was necessary. The vector count lives in the
encoding descriptor header, which is chunk metadata already in hand, and
build_skipvec's only other inputs are the predicates and the zone map catalog.A metadata-only pre-pass reads the count and decides
allDescriptorwithouttouching a value byte.
Two things this deliberately does not do
LIKEyieldsnumPredicates == 0and a NULL skip vector. The ~3200 ms budget there is1b-ii, and none of the six Citus columnar loads the same data 3x faster than we do, which contradicts why #300 was closed #445 losses is helped by this. It pays on queries
with a usable predicate, which is most analytic filters.
chunk; that is cost 1 and needs a format change (Phase 2). This removes cost 2,
the per-vector decode.
The fold had to change too, and #523's tripwire is why I knew
Skipping decode leaves holes in the packed value stream. The row producer is
safe (it steps its cursors past skipped vectors); the fold is not, because it
reads that buffer directly and re-checks every value. #523 landed a guard for
exactly this ordering, and it fired on the first run:
That guard did its job. The fold now walks the per-vector map it was already
handed, and skipping there is exact rather than merely safe:
pgcolumnar_batch_shape_eligiblerequires every qual to be convertible to a scankey, and those keys are what the reader built the skip vector from. The present
index still advances across a skipped vector, as it does across a deleted row,
because the stream is packed by presence.
The part worth reviewing: two instruments that could not see their subject
The counter was unfalsifiable. I first derived "vectors decoded" from the
mask the caller passed in. It reported the saving whether or not decode honoured
the mask, and the removal proof passed with the fix removed. It is now
reported from the loop that does the work.
The correctness checks could not see the hazard. Reading a hole is undefined
behaviour whose symptom is data-dependent: whatever
palloclast left there isnormally rejected by the fold's own scan-key recheck, so a fold that reads every
hole returns the right answer on ordinary data. Measured, not reasoned about:
with the skip removed from the fold, all 20 checks passed.
So assert builds now poison a skipped vector's bytes with
0xA5, which as anint4is-1515870811, and the suite adds a predicate chosen to accept thatvalue while still letting the zone maps rule most vectors out. With the fold's
skip removed, that check reports 32,696 rows where heap says 3,000.
Proof
test/native_vecdecode.sh, 23 checks. Every premise is asserted first, so aplan change or a split row group cannot masquerade as a decode regression.
Proved by removal, three ways:
decoded + skipped == the group's vectorsdecodes fewerstill passes, which is why the check is an equality and not an inequalityGate
temporal. That isbtree_gistmissingin my container, it fails identically on unmodified main, A suite that ran no checks must stop reporting PASSED (#447) #448 made a
missing dependency a failure by design, and
PGC_ALLOW_MISSING_BTREE_GIST=1isits documented override. Not this change.
Refs #452, #512