From 20e91424c782123efa9aff12fd8230bb16be74b4 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 28 Aug 2026 17:18:36 -0600 Subject: [PATCH 1/2] fix: refuse an array scan key before anything can build one (#752) `pgcolumnar_make_predicates` rejects seven ScanKey flags and accepts everything else. `SK_SEARCHARRAY` was not among them, and appears nowhere else in `src/`. Nothing can currently produce such a key: `pgcolumnar_saop_range_scankey` collapses every multi-element `= ANY` into two range keys, so the array never survives to this function. The gap is unreachable today. It is guarded anyway, because the defect it prevents is silent rather than wrong. A key carrying that flag holds an ARRAY in `sk_argument`. Unrejected, it becomes a SkipPredicate whose `compareValue` is the array's Datum, and the column's scalar btree comparison then runs against a pointer to an array header. That is not an incorrect result a suite could observe; it is a comparison of unrelated things whose answer is whatever the memory happens to say. The sequencing is the point. #752 measures a per-element path for `= ANY` worth 13 to 16 chunk groups of 27, and the obvious implementation stops collapsing the array and marks the key `SK_SEARCHARRAY`. Landing the guard first means whoever writes that has to add handling deliberately, instead of discovering that this function accepted it silently. The guard predates the capability. The test is a source assertion, in the style of `native_fetch_cache.sh`, and for the reason that suite records: there is no behavioural test to write while nothing can produce the shape. It carries a control, because the check would pass if the flag were named anywhere in that expression, so the seven original flags are asserted still present; a rewrite that dropped them while adding the new one would satisfy a bare grep. Removal proof: without the `src/` change the new check reads `got [0] want [1]` and the suite fails; with it, 38 of 38. Verified: `-Werror` on PG 15, 16, 17, 18 and 19, all rc=0 with 0 warnings and 0 errors. `native_skip` 49, `native_bloom` 7, `native_zonemap` 18, `qual_order_selectivity` 11 and `native_vecskip` 13 all pass. Hazard reported by OffgridwithJD, who also proposed landing it as its own commit ahead of the capability. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017Y7gXubmW8DDDZPZNPXJHm --- src/columnar_reader.c | 22 ++++++++++++++++++++-- test/native_saop_pushdown.sh | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/src/columnar_reader.c b/src/columnar_reader.c index 0214e1c3..3a059b65 100644 --- a/src/columnar_reader.c +++ b/src/columnar_reader.c @@ -648,10 +648,28 @@ pgcolumnar_make_predicates(SkipPredicate *out, int nkeys, ScanKey keys, Oid argType; bool crossType; - /* only plain "column op const" comparison keys are usable */ + /* + * Only plain "column op const" comparison keys are usable. + * + * SK_SEARCHARRAY is in this list although nothing in this extension + * currently sets it, and the reason is the point. A key carrying that + * flag holds an ARRAY in sk_argument, not a scalar. Without this arm the + * key falls through, compareValue is set to the array's Datum, and every + * zone-map comparison below runs the column's scalar btree function + * against a pointer to an array header. That is not a wrong answer that + * a test would catch; it is a comparison of unrelated things whose + * result is whatever the memory happens to say. + * + * It is guarded BEFORE anything can produce the shape rather than + * beside it. #752 measures a per-element path for `= ANY` worth 13 to 16 + * chunk groups of 27, and the obvious way to build it is to stop + * collapsing the array in pgcolumnar_saop_range_scankey and mark the key + * SK_SEARCHARRAY. Whoever does that should have to add handling here + * deliberately, not discover that this function accepted it silently. + */ if (key->sk_flags & (SK_ISNULL | SK_ROW_HEADER | SK_ROW_MEMBER | SK_ROW_END | SK_SEARCHNULL | SK_SEARCHNOTNULL | - SK_ORDER_BY)) + SK_ORDER_BY | SK_SEARCHARRAY)) continue; if (key->sk_attno < 1 || key->sk_attno > natts) continue; diff --git a/test/native_saop_pushdown.sh b/test/native_saop_pushdown.sh index 3d2c63a9..4b9c8dc6 100755 --- a/test/native_saop_pushdown.sh +++ b/test/native_saop_pushdown.sh @@ -218,6 +218,40 @@ check "a correlated PARAM_EXEC array is not mis-pruned (results stay correct)" \ "$(q 'SELECT sum(c) FROM thr, LATERAL (SELECT count(*) c FROM t WHERE t.ts = ANY (thr.lo)) s;')" \ "3" +# --- an array scan key must be refused before one can be built ------------ +# +# This is a source assertion, in the style of native_fetch_cache.sh, and the +# reason is the same one that suite records: there is no behavioural test to +# write, because nothing in the extension can currently produce the shape. The +# builder above collapses every multi-element array into two range keys, so no +# SK_SEARCHARRAY key ever reaches pgcolumnar_make_predicates. +# +# It is guarded anyway, and asserted here, because the defect it prevents is +# silent. Such a key holds an ARRAY in sk_argument. Unrejected, it becomes a +# SkipPredicate whose compareValue is the array's Datum, and the column's scalar +# btree comparison then runs against a pointer to an array header. That is not a +# wrong answer a suite could catch, it is a comparison of unrelated things. +# +# #752 measures a per-element path worth 13 to 16 chunk groups of 27, and the +# obvious implementation marks the key SK_SEARCHARRAY. This check exists so that +# whoever writes it has to add handling deliberately instead of finding that +# make_predicates accepted it silently. The guard predates the capability on +# purpose. +SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/src" + +check "an array scan key is refused by the predicate builder (#752)" \ + "$(grep -c 'SK_ORDER_BY | SK_SEARCHARRAY' "$SRC/columnar_reader.c")" "1" + +# The control. The check above passes if the flag is named ANYWHERE in that +# expression, so it must also be true that the builder still rejects the seven +# flags it always did; a rewrite that dropped them while keeping the new one +# would satisfy a bare grep for SK_SEARCHARRAY alone. +for _f in SK_ISNULL SK_ROW_HEADER SK_ROW_MEMBER SK_ROW_END SK_SEARCHNULL \ + SK_SEARCHNOTNULL SK_ORDER_BY; do + check "and it still refuses $_f" \ + "$(grep -c "$_f" "$SRC/columnar_reader.c")" "1" +done + check "backend alive" "$(q 'SELECT 1;')" "1" pgc_summary From 08462a80355df2bbd9607d9f1e6b5b56f09484e8 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 28 Aug 2026 19:07:00 -0600 Subject: [PATCH 2/2] test: assert the reject EXPRESSION, not that a flag appears in the file Review of #825 found the control could not do what its comment said, and the pattern this PR introduces is what defeats it. The control grepped each flag name over the whole file. That is a claim about the file, not about the guard. Deleting SK_ISNULL from the reject expression while naming it in a nearby comment left the suite printing PASS and it still refuses SK_ISNULL on a tree that no longer refused it. Measured in three cells, not supposed. The convention this guard introduces is what makes it worse: the new comment explains in prose why SK_SEARCHARRAY is rejected, and names it twice. So a file-wide grep is blinded for the next flag anyone documents, and the comment's own sentence about a bare grep was false. The reject expression is now extracted once and membership asserted inside it. Two premises, because an expression that failed to extract is an empty string and every membership test would then compare one blank with another and pass, which is the defect #823 fixed: one premise that exactly one sk_flags guard exists, and one that the extraction is non-blank and really contains sk_flags. Membership rather than adjacency. The old primary check grepped "SK_ORDER_BY | SK_SEARCHARRAY", so it reddened on a harmless reflow. The flag list is a set and asserting its order asserts more than the code means. Three cells, on the fixed control: CELL 1 as submitted 40/40 PASS CELL 2 SK_SEARCHARRAY out of the expression FAIL, names the flag CELL 3 SK_ISNULL out of the expression but named in a comment FAIL <- previously PASSED Cell 3 is the one this commit exists for. Both failures print the extracted expression, so a reader sees what the guard actually is. Found by OffgridwithJD, who ran the three cells rather than reasoning about them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017Y7gXubmW8DDDZPZNPXJHm --- test/native_saop_pushdown.sh | 53 +++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/test/native_saop_pushdown.sh b/test/native_saop_pushdown.sh index 4b9c8dc6..dbc2b798 100755 --- a/test/native_saop_pushdown.sh +++ b/test/native_saop_pushdown.sh @@ -226,30 +226,45 @@ check "a correlated PARAM_EXEC array is not mis-pruned (results stay correct)" \ # builder above collapses every multi-element array into two range keys, so no # SK_SEARCHARRAY key ever reaches pgcolumnar_make_predicates. # -# It is guarded anyway, and asserted here, because the defect it prevents is -# silent. Such a key holds an ARRAY in sk_argument. Unrejected, it becomes a -# SkipPredicate whose compareValue is the array's Datum, and the column's scalar -# btree comparison then runs against a pointer to an array header. That is not a -# wrong answer a suite could catch, it is a comparison of unrelated things. +# It is guarded anyway, because the defect it prevents is silent. Such a key +# holds an ARRAY in sk_argument. Unrejected, it becomes a SkipPredicate whose +# compareValue is the array's Datum, and the column's scalar btree comparison +# then runs against a pointer to an array header. That is not a wrong answer a +# suite could catch, it is a comparison of unrelated things. # -# #752 measures a per-element path worth 13 to 16 chunk groups of 27, and the -# obvious implementation marks the key SK_SEARCHARRAY. This check exists so that -# whoever writes it has to add handling deliberately instead of finding that -# make_predicates accepted it silently. The guard predates the capability on -# purpose. +# THE ASSERTION IS ABOUT THE EXPRESSION, NOT ABOUT THE FILE, and that distinction +# is the whole check. An earlier draft grepped each flag name over the whole +# file, which is a claim about the file: deleting SK_ISNULL from the guard while +# naming it in a nearby comment left the suite printing +# "PASS and it still refuses SK_ISNULL" on a tree that no longer refused it. +# Measured, not supposed. The convention this very guard introduces -- explain in +# prose why a flag is rejected -- is what would blind a file-wide grep for the +# next flag anyone documents. +# +# So the reject expression is extracted once and membership is asserted inside +# it. Extraction is a premise, because an expression that failed to extract is +# an empty string and every "is this flag in it" test would then compare one +# blank with another and pass (#823). SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/src" -check "an array scan key is refused by the predicate builder (#752)" \ - "$(grep -c 'SK_ORDER_BY | SK_SEARCHARRAY' "$SRC/columnar_reader.c")" "1" +check "premise: the predicate builder has exactly one sk_flags reject guard" \ + "$(grep -c 'key->sk_flags &' "$SRC/columnar_reader.c")" "1" + +# From the `if (key->sk_flags & (` line through the closing `))`, comments +# excluded by construction: the range starts at the `if`, so prose above it +# cannot be captured. +guard="$(awk '/if \(key->sk_flags & \(/,/\)\)/' "$SRC/columnar_reader.c" | tr -d ' \t\n')" + +check "premise: the reject expression was extracted, not blank" \ + "$([ -n "$guard" ] && [ "${guard#*sk_flags}" != "$guard" ] && echo yes \ + || echo "extracted=<${guard:-empty}>")" "yes" -# The control. The check above passes if the flag is named ANYWHERE in that -# expression, so it must also be true that the builder still rejects the seven -# flags it always did; a rewrite that dropped them while keeping the new one -# would satisfy a bare grep for SK_SEARCHARRAY alone. +# Membership, not adjacency. The flag list is a SET; asserting the order of it +# would redden on a harmless reflow and would assert more than the code means. for _f in SK_ISNULL SK_ROW_HEADER SK_ROW_MEMBER SK_ROW_END SK_SEARCHNULL \ - SK_SEARCHNOTNULL SK_ORDER_BY; do - check "and it still refuses $_f" \ - "$(grep -c "$_f" "$SRC/columnar_reader.c")" "1" + SK_SEARCHNOTNULL SK_ORDER_BY SK_SEARCHARRAY; do + check "the predicate builder's reject expression contains $_f" \ + "$(case "$guard" in *"$_f"*) echo yes ;; *) echo "absent from <$guard>" ;; esac)" "yes" done check "backend alive" "$(q 'SELECT 1;')" "1"