From 27da1ab6c6ceb50a9423a206136a3b8723b6af64 Mon Sep 17 00:00:00 2001 From: ChronicallyJD Date: Sun, 9 Aug 2026 14:07:56 -0600 Subject: [PATCH] fix: count a skipped vector where decode skips it, not where rows are produced (#542) vectorsSkipped was incremented in pgcolumnar_native_skip_current_vector, which only the row path enters. The batch fold takes a group whole through PgColumnarReadFoldNextGroup and never produces rows one at a time, so it never reached the counter: a fold decoding 1 of 59 vectors reported Columnar Vectors Skipped: 0 That is worse than printing nothing. It answers the question wrongly, and it is the number #522 added so a plan could say whether per-vector skipping happened on the fold path -- the path where it matters, since #512 is about the fold and the skip vector disagreeing. It is now counted from the final nativeSkipVec, after both decode passes, which both paths share. Counted from the MASK and deliberately not as (maxVecCount - groupVecDecoded). That subtraction looks equivalent and is not, because #452 phase 1b-ii made the loop two-pass: pass 0 decodes the qual columns against the mask the ZONE MAPS built, refine_skipvec sharpens it to "no row in this vector matches", and pass 1 decodes the payload columns against the sharpened mask. groupVecDecoded is the max across columns over BOTH passes, so on a zero-matching range it is the qual column's full 32 while the payload columns decoded nothing -- the subtraction reports 0 skipped for a group in which every vector was ruled out. That is not hypothetical: it made native_exact_selection's "zone maps rule out nothing" premise read -32, a count that cannot exist. The mask is what says what was skipped, so ask it. The mask-based count agrees with the row path on every fixture measured -- 58 on native_fold_skipguard's, 32 on native_exact_selection's, 30 on a 1b-ii shape -- and adds the fold arm, where the number was 0. arm before after Decoded scalar 58 58 1 fold 0 58 1 so decoded + skipped == the group's 59 vectors on BOTH arms, where it previously held on the scalar arm alone. It also repairs an invariant #541 depends on. vectorsRuledOutByValue is declared "subset of vectorsSkipped" and is incremented during mask refinement, which both arms run; on the fold arm it read 1 beside a vectorsSkipped of 0, a subset larger than its superset. The suite half matters as much. native_fold_skipguard read its premise off the SCALAR arm -- "same table, same predicate, same reader" -- and that proxy is what hid this: the premise for the check under test was measured on a plan that was not the one under test. It now reads the fold plan and asserts exact values, making it a WORK-DONE assertion rather than another correctness one. A fold that silently stopped skipping and decoded all 59 vectors passes every correctness check we own, because decoding everything reads real values rather than holes. Also corrects two statements the suite had outlived: a header describing a world where "the fold does NOT honour the skip vector" (#452 phase 1b-i ended that), and a check named "the fold answers correctly while decode skips nothing", which contradicted the premise directly above it even when written. Gated on pg18a and pg19a: native_exact_selection 16/16, native_fold_skipguard 6/6, native_vecdecode 24/24, ungrouped_vector_agg 45/45, 0 warnings, built from make clean with no stale objects (#536). Removal proof: putting the counter back on the row path fails exactly the two new checks, got [0] want [58] and got [1] want [59]. --- src/columnar_reader.c | 62 ++++++++++++++++++++++++++- test/native_fold_skipguard.sh | 80 ++++++++++++++++++++++++----------- 2 files changed, 116 insertions(+), 26 deletions(-) diff --git a/src/columnar_reader.c b/src/columnar_reader.c index 19797965..468c6d04 100644 --- a/src/columnar_reader.c +++ b/src/columnar_reader.c @@ -1984,6 +1984,61 @@ pgcolumnar_native_load_group(PgColumnarReadState *rs) } rs->vectorsDecoded += (uint64) groupVecDecoded; + /* + * Counted HERE, where the skipping is performed, and not where rows are + * produced (#542). + * + * It used to be incremented in pgcolumnar_native_skip_current_vector, which + * only the row path enters. The batch fold takes a group whole through + * PgColumnarReadFoldNextGroup and never produces rows one at a time, so it + * never reached that counter: a fold that decoded 1 of 59 vectors reported + * "Columnar Vectors Skipped: 0". That is worse than printing nothing, because + * it answers the question wrongly, and it is the number #522 added precisely + * so a plan could say whether per-vector skipping happened on the fold path. + * + * Decode is shared by both paths, and it is the only place the final mask + * exists. + * + * What this does to the scalar arm's total, stated exactly, because an + * earlier version of this comment claimed it was unchanged and that is only + * true of a scan that runs to completion: + * + * query before after + * WHERE k BETWEEN 30000 AND 30100 58 58 + * ... LIMIT 1 29 58 + * + * The old counter incremented as the PRODUCER stepped over vectors, so a + * LIMIT that stopped it halfway counted half of them. This one counts what + * decode ruled out, which does not depend on how many rows were consumed. The + * new number is the one that satisfies the invariant this change exists for: + * on a 59-vector group the LIMIT row was 1 + 29 = 30, and is now 1 + 58 = 59. + * The old number conflated "ruled out by the mask" with "stepped over before + * we stopped", which is the same conflation #542 is about, one layer down. + * + * Counted from nativeSkipVec and NOT as (maxVecCount - groupVecDecoded). + * That subtraction looks equivalent and is not, because #452 phase 1b-ii made + * the loop two-pass: pass 0 decodes the qual columns against the mask the + * ZONE MAPS built, refine_skipvec then sharpens it to "no row in this vector + * matches", and pass 1 decodes the payload columns against the sharpened one. + * groupVecDecoded is the max across columns over BOTH passes, so on a + * zero-matching range it is the qual column's full 32 while the payload + * columns decoded nothing -- the subtraction yields 0 skipped for a group + * where every vector was ruled out. Measured: it made + * native_exact_selection's "zone maps rule out nothing" premise read -32. + * + * The mask is the thing that says what was skipped, so ask it. + */ + if (rs->nativeSkipVec != NULL && rs->nativeVectorCount > 0) + { + int v; + int skipped = 0; + + for (v = 0; v < rs->nativeVectorCount; v++) + if (rs->nativeSkipVec[v]) + skipped++; + rs->vectorsSkipped += (uint64) skipped; + } + /* * The #512 tripwire's input, and it is measured rather than inferred from * the mask: a mask with nothing set in it skips nothing, and a decode that @@ -2064,7 +2119,12 @@ pgcolumnar_native_skip_current_vector(PgColumnarReadState *rs) rs->rowInGroup = rs->nativeVecStart[v + 1]; rs->nativeCurVec = v + 1; - rs->vectorsSkipped++; + /* + * vectorsSkipped is NOT incremented here any more (#542). This function is + * on the row path only, so counting here made the number a row-path counter + * wearing a plan-wide name: correct on a sequential scan, silently 0 on the + * batch fold. It is now counted in the group decode, which both paths share. + */ return true; } diff --git a/test/native_fold_skipguard.sh b/test/native_fold_skipguard.sh index 0291472c..19f8167b 100644 --- a/test/native_fold_skipguard.sh +++ b/test/native_fold_skipguard.sh @@ -3,21 +3,30 @@ # pgColumnar: the batch fold must refuse a group whose decode skipped vectors # (#512). # -# The fold at columnar_vector.c does NOT honour the skip vector. It reads every -# vector and reaches the right answer by re-checking every value against the scan -# keys, which is correct only while decode produces every vector -- and today it -# does, because pgcolumnar_native_decode_chunk takes no skip mask at all. +# That day has arrived. #452 phase 1b-i taught decode to skip the vectors the +# zone maps rule out, and taught the fold to honour the same mask, so the text +# that used to stand here -- "the fold does NOT honour the skip vector ... decode +# produces every vector" -- is no longer true of any line in the tree. # -# The day decode is taught to skip ruled-out vectors (#452 phase 1b) the decoded -# buffer gains holes, and this loop would re-check UNINITIALISED memory: a wrong -# aggregate, silently, and only on data whose zone maps rule something out. The -# row producer is safe there; the fold is not. So the ordering constraint is -# enforced by a guard rather than written in a comment. +# What the #512 guard was for: decode gaining holes before the fold learned about +# them would have made the fold re-check UNINITIALISED memory -- a wrong +# aggregate, silently, and only on data whose zone maps rule something out. It +# was an ORDERING guard, and it fired on exactly the change it was written for. +# What survives it is a narrow fallback: a skip reported without the per-vector +# map to honour it must ERROR rather than guess. # -# This suite proves the guard is REACHABLE, which is the part that is easy to get -# wrong: the fold only runs for a shape that qualifies for it, and the hazard only -# exists where vectors are skipped. Both have to be true at once, and a check that -# never reaches the fold would pass forever while guarding nothing. +# So be exact about what this suite does and does not prove, because the two are +# easy to confuse now that the guard is no longer load-bearing: +# +# this suite the fold is REACHABLE for a shape that skips vectors, and +# that it accounts for the skipping it performs +# native_vecdecode the fold is CORRECT there -- its poison check picks a +# predicate that ACCEPTS 0xA5, so a fold reading an +# undecoded hole counts rows that are not there +# +# Neutering the surviving fallback does NOT redden this suite, and that is now +# the expected result rather than a hole: the path it guards is unreachable while +# decode and the fold honour the same mask. Measured, not assumed. # # Usage: test/native_fold_skipguard.sh [PG_CONFIG] # Written fresh for pgColumnar. @@ -46,15 +55,32 @@ run() { # run -d "$PGC_DB" -At -c "$1" -c "$2" 2>&1 } -# ---- premise one: the reader really does skip vectors for this predicate ---- +# ---- premise one: the FOLD ARM does the skipping, and says so --------------- +# +# Read off the fold arm itself, which is the plan under test. It used to be read +# off the scalar arm with the note "same table, same predicate, same reader, so +# what it skips there it skips here" -- a proxy, and one that hid #542: the +# counter was incremented only where rows are produced, so the fold reported +# "Vectors Skipped: 0" while decoding 1 of 59 vectors. Measuring the premise on a +# plan that is not the one under test is what let that sit unnoticed. +# +# Exact values, not "greater than zero". 60,000 rows at chunk_group_row_limit +# 1024 is 59 vector positions in one group, and k BETWEEN 30000 AND 30100 lies +# inside one of them, so decode must touch exactly 1 and skip exactly 58. # -# Read off the SCALAR arm, because the vectorized aggregate node does not print -# "Columnar Vectors Skipped" at all -- a reporting gap of its own. Same table, -# same predicate, same reader, so what it skips there it skips here. -vskip=$(run "$SET_OFF" "EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) $Q" \ - | grep "Columnar Vectors Skipped" | grep -oE '[0-9]+' | head -1) -check "premise: the predicate rules vectors out at all" \ - "$([ "${vskip:-0}" -gt 0 ] && echo yes || echo no)" "yes" +# This is a WORK-DONE assertion, and it is the only kind that can catch a fold +# that silently stops skipping. Every other check here and in native_vecdecode is +# a correctness check, and a fold that decoded all 59 vectors would pass all of +# them: decoding everything reads real values, never a hole, so the answers stay +# right and the saving disappears in silence. +GROUP_VECS=59 +fplan=$(run "$SET_ON" "EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) $Q") +vskip=$(grep "Columnar Vectors Skipped" <<<"$fplan" | grep -oE '[0-9]+' | head -1) +vdec=$(grep "Columnar Vectors Decoded" <<<"$fplan" | grep -oE '[0-9]+' | head -1) +check "premise: the fold arm decodes only the vector it needs" "${vdec:-none}" "1" +check "and reports the 58 it skipped, on its own plan (#542)" "${vskip:-none}" "58" +check "so decoded plus skipped is the group's vectors, on the FOLD arm" \ + "$(( ${vdec:-0} + ${vskip:-0} ))" "$GROUP_VECS" # ---- premise two: the query actually reaches the fold ----------------------- # @@ -65,14 +91,18 @@ fold=$(run "$SET_ON" "EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) $Q" | grep -oE "Columnar Batch Fold: [a-z]+" | head -1) check "premise: the query reaches the batch fold" "$fold" "Columnar Batch Fold: yes" -# ---- the guard is silent while decode produces every vector ---------------- +# ---- and it answers correctly while decode DOES skip ----------------------- # -# Today decode never skips, so the fold must run normally and answer correctly. -# The heap arithmetic is the oracle: 101 rows, sum of 30000..30100. +# The name matters. This check was called "the fold answers correctly while +# decode skips nothing", which contradicted the premise directly above it even +# when it was written, and became plainly false when #452 phase 1b-i landed. +# Decode skips 58 of 59 vectors on this query; the fold answers over the one that +# survives. The heap arithmetic is the oracle: 101 rows, sum of 30000..30100. want_n=101 want_s=$(( (30000 + 30100) * 101 / 2 )) got=$(run "$SET_ON" "$Q" | tail -1) -check "the fold answers correctly while decode skips nothing" "$got" "$want_n|$want_s" +check "the fold answers correctly while decode skips 58 of 59 vectors" \ + "$got" "$want_n|$want_s" # And the same answer without the fold, so the fold is not quietly wrong in a way # that matches a wrong expectation.