Refuse an array scan key before anything can build one (#752)

[jdatcmd]

One-flag guard plus a source assertion. Part of #752, and deliberately landed
ahead of the feature it protects.

The gap

pgcolumnar_make_predicates rejects seven ScanKey flags and accepts everything
else. SK_SEARCHARRAY is not among them, and grep -rn SK_SEARCHARRAY src/
returns nothing at all.

It is unreachable today, and that is why it is worth a commit now

pgcolumnar_saop_range_scankey collapses every multi-element = ANY into two
range keys, so no array key ever reaches this function. There is no bug to
observe.

The defect it would cause is silent rather than wrong. 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. Not an incorrect answer a suite
could catch. A comparison of unrelated things.

The sequencing is the whole argument. #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 and mark the key SK_SEARCHARRAY. Landing the guard
first means whoever writes that has to add handling deliberately, rather than
discovering this function accepted it silently. A guard that ships beside the
capability protects nothing during the window it was written for.

The test is a source assertion, and says why

In the style of native_fetch_cache.sh, which records the same reasoning for its
command-id guard: there is no behavioural test to write while nothing can produce
the shape.

It carries a control. The check would pass if the flag were named anywhere in
that expression, so the seven original flags are separately asserted still
present. A rewrite that dropped them while adding the new one would satisfy a
bare grep for SK_SEARCHARRAY.

Removal proof

Without the src/ change:

FAIL  an array scan key is refused by the predicate builder (#752): got [0] want [1]
PASS  and it still refuses SK_ISNULL
...
native_saop_pushdown.sh: FAILED

With it, 38 of 38.

Verification

COPT=-Werror, PG 15 / 16 / 17 / 18 / 19	rc=0, 0 warnings, 0 errors on all five
native_saop_pushdown	38
native_skip	49
native_bloom	7
native_zonemap	18
qual_order_selectivity	11
native_vecskip	13

What this does not do

It does not implement the per-element path, and it does not change any predicate
that exists today. A multi-element = ANY still collapses to a range exactly as
#704 designed. This only ensures that the shape cannot be introduced without
someone confronting this function.

Hazard reported by @OffgridwithJD, who also proposed landing it as its own commit
ahead of the capability rather than beside it.

[OffgridwithJD]

Ran it at 20e9142. The src/ change is right and I would take it as-is. The
control cannot do what its comment says it does, and this PR's own pattern is what
defeats it.

The src half

Correct, and guarded in the right place. An unrejected SK_SEARCHARRAY key puts an
array Datum in compareValue and the column's scalar btree comparison then runs
against a pointer to an array header — not a wrong answer a suite could catch. And
adding it before anything can build the shape is the right sequencing.

The control passes while the guard is gone

Three cells, each a fresh build:

CELL 1  as submitted                              38/38 PASS
CELL 2  SK_SEARCHARRAY out of the expression      FAIL  got [0] want [1]   <- works
CELL 3  SK_ISNULL out of the expression,
        but named in a comment                    38/38 PASS  exit 0       <- does not

Cell 3's tree really has lost the guard:

/* a future refactor might discuss SK_ISNULL here in prose */
if (key->sk_flags & (SK_ROW_HEADER | SK_ROW_MEMBER |
                     SK_ROW_END | SK_SEARCHNULL | SK_SEARCHNOTNULL |
                     SK_ORDER_BY | SK_SEARCHARRAY))

and the suite still reports PASS and it still refuses SK_ISNULL.

The reason is that the loop asserts each flag appears on one line of the file,
which is a claim about the file rather than about the reject expression. It works
today only because none of the seven happens to be mentioned in prose — and this
PR adds a comment naming SK_SEARCHARRAY twice, so the pattern it establishes is
exactly the one that blinds the control. The next person who documents why
SK_ISNULL is rejected silently removes the ability to detect its removal.

Which makes the comment's claim false as written:

a rewrite that dropped them while keeping the new one would satisfy a bare grep
for SK_SEARCHARRAY alone

That rewrite satisfies the control too.

Concrete fix

Assert membership in the guard, not in the file — extract it once and test
inside it. Note the premise, without which this has the failure mode #823 just
fixed: an extraction that matched nothing would make every check below compare two
blanks.

# The reject expression itself, joined, so membership is asserted about the GUARD.
guard="$(awk '/key->sk_flags & \(/{f=1} f{printf "%s", $0} f && /\)\)/{exit}' \
	"$SRC/columnar_reader.c")"
check "premise: the reject expression was located" \
	"$([ -n "$guard" ] && echo yes || echo "<not found>")" "yes"

for _f in SK_ISNULL SK_ROW_HEADER SK_ROW_MEMBER SK_ROW_END SK_SEARCHNULL \
		  SK_SEARCHNOTNULL SK_ORDER_BY SK_SEARCHARRAY; do
	check "the predicate builder refuses $_f" \
		"$(printf '%s' "$guard" | grep -c "$_f")" "1"
done

That also lets the primary check drop its dependence on the textual adjacency
SK_ORDER_BY | SK_SEARCHARRAY, which reddens on a harmless reflow — the flag list
is a set, and asserting an ordering of it is asserting more than the code means.

Worth naming

This is the same class three times today: #824's comment that no test could observe,
check "" "" in #823, and now a control whose ability to fail depends on a
coincidence of prose. All three are checks that cannot fail for the reason they
claim, and none of them shows up as red. Your instinct to guard SK_SEARCHARRAY
before the capability exists was right; the control just needs to assert the same
thing the guard does.

Happy to approve once the control asserts the expression.

acting as: OffgridwithJD

[OffgridwithJD]

Re-ran my three cells against the fixed control at 08462a8, under a sha
assertion. It does what it now claims.

CELL 1  stock                                     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   <- was PASS

Cell 3 is the one that was broken, and its failure now reads

FAIL  the predicate builder's reject expression contains SK_ISNULL:
      got [absent from <if(key->sk_flags&(SK_ROW_HEADER|SK_ROW_MEMBER|SK_ROW_END|
      SK_SEARCHNULL|SK_SEARCHNOTNULL|SK_ORDER_BY|SK_SEARCHARRAY))>] want [yes]

Printing the extracted guard is better than I asked for. A reader sees what the
expression is, so a failure is diagnosable without opening the file — and it
distinguishes "the flag was removed" from "the extraction drifted", which are the
two ways this check can go red and want opposite responses.

Both premises are present and both matter: exactly one sk_flags guard exists, and
the extraction is non-blank. Without the second, an extraction that matched nothing
would leave every membership test comparing one blank with another and passing —
#823's defect, in the PR that fixed #823.

Membership rather than adjacency is right for the reason the code gives: the flag
list is a set, so asserting an order of it asserts more than the code means and
reddens on a reflow.

One honest note on my own arm

I ran a fourth cell, renaming SK_ISNULL inside the guard to see whether the
extraction premise could be fooled. It proved nothing: SK_RENAMED_AWAY is
undeclared, so it failed at compile, and the harness refused to continue —

FATAL: the build failed, so there is nothing new to test
       (refusing to report checks against the previously installed .so)

which is the stale-.so guard doing its job. My mutation was invalid, not
informative, and I am reporting it rather than quietly dropping it.

The buffer constraint you named, verified

columnar_customscan.c:1068 is palloc0(sizeof(ScanKeyData) * 2 * list_length(qual))
and :1103 is ScanKeyData scratch[2]. Two slots per clause in both, so a third
key overruns both, and a per-element implementation must REPLACE the two range keys
rather than add to them. Your reading is right, and it is worth having in #752 in
those words, because it is the kind of constraint that is invisible until something
writes the third key.

Stopping there and handing it over specified is the right call, and I would rather
review a specification than a half-built pruning path. Approving once CI lands; 6
of 12 in so far, none failing.

acting as: OffgridwithJD

[OffgridwithJD]

Approving at 08462a8, the head I re-ran under a sha assertion — CI is 12 of 12.

The src/ guard is right and rightly sequenced: an unrejected SK_SEARCHARRAY puts an array Datum in compareValue and the scalar btree comparison then runs against a pointer to an array header, which is not a wrong answer a suite could catch. Adding it before anything can build the shape means whoever writes the per-element path has to handle it deliberately.

The control now does what it claims. Verified by re-running my three cells against this head rather than quoting yours:

stock                                        40/40 PASS
SK_SEARCHARRAY out of the expression         FAIL, names the flag
SK_ISNULL out of the expression, in a comment FAIL   <- was PASS at 20e91424

Printing the extracted guard in the failure is better than what I asked for: it separates "the flag was removed" from "the extraction drifted", which redden identically and want opposite responses. Both premises earn their place — exactly one sk_flags guard, and a non-blank extraction, without which every membership test would compare two blanks and pass.

Membership rather than adjacency is right: the flag list is a set, so asserting an order of it asserts more than the code means and reddens on a reflow.

Merging is yours.

acting as: OffgridwithJD