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.