Skip to content

docs: align SAOP exactness comments (#904 follow-up) - #912

Merged
jdatcmd merged 1 commit into
commandprompt:mainfrom
linuxhikerpm:audit/saop-exactness-comments
Sep 10, 2026
Merged

docs: align SAOP exactness comments (#904 follow-up)#912
jdatcmd merged 1 commit into
commandprompt:mainfrom
linuxhikerpm:audit/saop-exactness-comments

Conversation

@linuxhikerpm

Copy link
Copy Markdown

Follow-up from the post-merge re-review of #904.

Two load-bearing comments still described every ScalarArrayOpExpr as a [min,max] range. #904 changed the normal path to a set-valued key, retaining the range only above the 128-element limit. The comments now state that shape while preserving the key safety conclusion: both forms are conservative and must remain excluded from the batch fold as complete row filters.

Also asserts the invariant the bloom/cross-type argument depends on: the copied array header's element type equals the scan key's sk_subtype. The only producer derives both from the same Const; a cassert build now checks that directly beside the existing kept >= 2 invariant.

Verification on this exact patch composed with merged #904:

  • native_saop_pushdown: 53/53
  • complete pytest corpus: 122/122
  • full PG18 matrix: 248 passed, 2 PG19-only skips, 0 failed/incomplete
  • git diff --check: clean

No behavior, SQL, catalog, or on-disk format change.

Co-authored-by: Cursor <cursoragent@cursor.com>

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 5ff448cf. Both comments now describe what the producer emits,
and the invariant I suggested is not only added but live.

The Assert is reached, not merely true

An assertion that is never executed is indistinguishable from one that holds, and
it is the easier of the two to ship by accident. So I inverted it and required
the crash:

as written    53 passed + 0 failed    0 crash markers
INVERTED      21 passed + 32 failed   5 crash markers   <- the line is reached
restored      53 passed + 0 failed    byte-exact

Run against /usr/local/pg17a, which I checked is genuinely a cassert build
rather than assuming it — pg_config --configure carries --enable-cassert and
pg_config --includedir-server's pg_config.h has #define USE_ASSERT_CHECKING 1.
On a non-assert build the whole thing compiles to nothing and the run proves
exactly zero. Worth stating in the PR body next time: "native_saop_pushdown 53/53"
does not distinguish those two cases, and this is a change whose entire content
on the C side is one assertion.

The comments

Both now say what the code does, and both keep the conclusion that matters:

`col IN (...)` / `col = ANY(array)` becomes one set key, with a bounded
[min,max] fallback above the element limit (#704, #752). Both forms are
conservative pruning keys, so exact remains false and the fold refuses
them as its complete row filter (#715).

That is the right shape — the reason is what a reader relies on when deciding
whether a change is safe, and the previous version handed them a mechanism that
no longer applied. columnar_vector.c gets the same substitution, which is the
one I flagged as a line #904 falsified without touching; it is touched now.

One small thing I would not change but will name: dropping "now" from
"pgcolumnar_clause_to_scankey now reports that inexactness" is right, since
that word was carrying a diff-relative meaning that stopped being true the moment
#715 merged. Comments that date themselves relative to a change go stale silently.

Scope

src/columnar_customscan.c   comment only
src/columnar_vector.c       comment only
src/columnar_reader.c       one Assert, inside the existing SEARCHARRAY block

No behaviour change outside a cassert build, and I confirmed the only executable
line is the assertion.

@jdatcmd

jdatcmd commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Verified before merging, since this touches src/ and "comment-only" is not a no-op in this tree.

The diff is what the description says: two comment corrections and one Assert. Both corrected comments now describe the set key with the bounded fallback, and both keep the conclusion that matters — the keys stay conservative, exact stays false, and the fold refuses them as a complete row filter.

The one thing worth checking was the new Assert, because an assertion that can fire on a legitimate query is a cassert-build crash rather than a comment. It cannot:

arr      = DatumGetArrayTypeP(con->constvalue);   /* customscan.c */
elemtype = ARR_ELEMTYPE(arr);
key[0].sk_subtype = elemtype;
key[0].sk_argument = con->constvalue;

arr = DatumGetArrayTypePCopy(key->sk_argument);   /* reader.c, same Const */
Assert(ARR_ELEMTYPE(arr) == key->sk_subtype);

Both sides derive from the same Const, so the invariant holds by construction rather than by convention — which is exactly what makes it worth asserting where the next person will trip over it, alongside kept >= 2.

Noting for the record that a cassert build's behaviour does change, which the description says plainly rather than hiding under "no behavior change". That is the right way to state it.

Approved by @OffgridwithJD at the head. Merging when the two running checks land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants