Skip to content

feat: gate payload decode for an unprunable qual (#452 phase 2) - #584

Merged
jdatcmd merged 2 commits into
mainfrom
feat/452-phase2-decode-gating
Aug 11, 2026
Merged

feat: gate payload decode for an unprunable qual (#452 phase 2)#584
jdatcmd merged 2 commits into
mainfrom
feat/452-phase2-decode-gating

Conversation

@jdatcmd

@jdatcmd jdatcmd commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

What

The piece the #452 comments call phase 2, not the design doc's Route B (that per-vector-compression tradeoff stays owner-rejected). A leading-wildcard LIKE '%needle%' is never a SkipPredicate, so numPredicates == 0, refine_skipvec builds no mask, and payload columns are decoded for every vector regardless of selectivity. Measured ceiling (ChronicallyJD, 2026-08-11): SELECT * under tag LIKE '%HIT%' spends 81% of its time decoding 20 payload columns for ruled-out rows, invariant to how many rows match.

Phase 1a evaluates this qual per-row after decode (saves materialization, not decode). This hoists 1a's executor-qual callback to group-load time and runs it per vector between the two decode passes — a vector no row passes is marked in nativeSkipVec, so pass 1 skips its payload decode via 1b-i's existing machinery. No format change; Route B untouched.

Two surprises the plan didn't foresee (both in the design doc)

  1. build_skipvec allocated nothing without a predicate — the mask and per-vector row spans didn't exist for an unprunable qual. Now allocates whenever a mask will be consulted (predicates or a phase-2 qual); the plain no-qual scan is unchanged.
  2. Reusing 1a's counting callback double-counted Rows Removed by Filter. The evaluator walks every row; the producer then re-tests survivors' vectors. Fix: the evaluator is the group's sole reject-counter, the producer filters through a non-counting variant on this path, and fully-skipped vectors' rows join Rows Filtered Before Materialization where they're ruled out. Both counters stay exact — native_decode_gating.sh asserts them (the first version measured only the decode saving and would have missed the double-count).

Tests

  • test/native_decode_gating.sh (new, 21 checks): RED on dde753a for the right reason (all payload vectors decoded → 192), GREEN after (32 / 37). Removal proof: disabling only the mask writer reddens exactly the two decode checks. Premises assert the unprunable shape (Usable Skip Predicates 0, Chunk Groups Removed 0, Vectors Ruled Out by Value 0) and the late-mat path, on both arms.
  • test/native_late_materialization.sh retargeted (needle in every vector) so phase 2 rules none out and it measures 1a in isolation — honoring its own "nothing pruned at the vector level" premise.
  • Regression: native_exact_selection (1b-ii), native_fold_skipguard, native_skip, native_vecskip, native_vecdecode, native_agg, native_groupagg, differential, column_projection, native_bloom — all green. harness_selftest (107) + docs_style (9) green. PG18 + PG19.

Not this PR

Route B / per-vector compression (owner-rejected). q24's leading-wildcard reaching the reader (that's #426). A selectivity gate to skip the evaluator when it can't pay — noted as a follow-up.

🤖 Generated with Claude Code

A leading-wildcard LIKE ('%needle%') is never a SkipPredicate, so numPredicates
is 0, refine_skipvec builds no mask, and the payload columns are decoded for every
vector no matter how few rows survive. That is the invariance #452 is named for:
on ChronicallyJD's 21-column fixture, SELECT * under LIKE '%HIT%' spends 81% of
its time decoding the 20 payload columns for rows the filter rules out, and it
costs the same whether 600 rows match or none.

Phase 1a already evaluates this qual, but per row in the producer, after both
decode passes, so it saves materialization and never decode. This hoists 1a's
executor-qual callback to group-load time and runs it per VECTOR between the two
passes: a vector no row can pass is marked in nativeSkipVec, so pass 1 skips its
payload columns' decode through the machinery 1b-i already built. The split that
puts the qual column in pass 0 is extended to the executor qualCols only when
numPredicates is 0, so 1b-ii's predicate path is byte-for-byte unchanged.

Two things the plan did not foresee, both in the design doc:

- build_skipvec returned early with no predicate, so neither the mask nor the
  per-vector row spans existed. It now allocates whenever a mask will be consulted
  -- predicates OR a phase-2 qual -- and the plain scan with neither is unchanged.

- reusing 1a's counting callback double-counted "Rows Removed by Filter", because
  the evaluator walks every row and the producer then re-tests a surviving vector.
  The evaluator is now the group's sole counter and the producer filters through a
  non-counting variant on this path; fully-skipped vectors' rows are added to
  "Rows Filtered Before Materialization" where they are ruled out. Both counters
  stay exact, and native_decode_gating.sh asserts them.

native_late_materialization.sh is retargeted so its needle falls in every vector,
keeping a survivor in each so phase 2 rules none out and it measures 1a in
isolation -- as its own premise requires. Green PG18 + PG19; 1b-i/1b-ii/fold-skip
and the differential suite unaffected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WmQJqcXdwyuoAiHHt2znBr
@ChronicallyJD

Copy link
Copy Markdown
Collaborator

Blocking: the phase-2 evaluator reads a freed delete mask (heap-use-after-free)

Caught by running the differential suite under ASAN+UBSAN (a decode-path change earns the sanitizer gate). It is a #584 regression, controlled against main: same forced-ASAN build, differential.sh PASSES on main (dde753a) with zero sanitizer lines, and heap-use-after-frees on this branch (3fce482).

Not LIKE-specific, and not caught by assert. The trigger is any unprunable qual (numPredicates == 0) on a table with deleted rows — the differential suite hits it with UPDATE t_col SET v = v+1 WHERE id % 17 = 0. Assert misses use-after-free; only the sanitizer does. Your own gating suites (native_decode_gating, native_late_materialization, native_vecskip) are ASAN-clean and their columnar-vs-heap differentials match, so the gating logic is correct — this is a lifecycle ordering bug, not a design flaw.

Root cause (exact). In pgcolumnar_native_load_group:

:1972   MemoryContextReset(rs->groupContext);        // frees the PREVIOUS group's nativeDeleteMask
                                                      //   (palloc0'd in groupContext at :2258 last call)
:1995   PgColumnarReadLogicalData(... nativeBuffer)  // new group's data
:2096   pgcolumnar_native_qual_skipvec(rs, ...)      // -> reads rs->nativeDeleteMask[r>>3] at :1858
:2240   rs->nativeDeleteMask = NULL;                 // per-group reset...
:2258   rs->nativeDeleteMask = palloc0(...)          // ...and reload, but AFTER the evaluator already read it

qual_skipvec (:1858) reads nativeDeleteMask before it is nulled/reloaded for the current group (:2240-:2258); the pointer still holds the previous group's address, which the reset at :1972 already freed. ASAN:

READ of size 1 ... pgcolumnar_native_qual_skipvec  columnar_reader.c:1858
                   pgcolumnar_native_load_group     columnar_reader.c:2096
freed by         MemoryContextReset via load_group  columnar_reader.c:1972
allocated by     PgColumnarReadLogicalData          columnar_storage.c:556  (via load_group:1995)

On main the only delete-mask read is at row-emit time (:2200), always after setup — this new read at group-load time is what runs early.

Fix direction: hoist the delete-mask setup (:2240-:2258) to before the qual_skipvec call (:2096). Nulling it after the reset would stop the crash but the evaluator needs the correct current-group mask (it excludes deleted rows from "does any row pass" and from the reject count), so it must be loaded, not just cleared. A minimal repro on a columnar table with deletes + an unprunable qual reproduces it deterministically under pg18_san.

Holding my approval until this is fixed; happy to re-run the ASAN differential on the fix.

pgcolumnar_native_qual_skipvec reads rs->nativeDeleteMask at group-load time to
keep deleted rows out of "does any row pass" and the reject count. The mask was
built AFTER the evaluator, so on the second group onward of a table with deleted
rows the evaluator read the PREVIOUS group's mask -- freed by the group-context
reset one load earlier. A heap-use-after-free, reachable only by an unprunable
qual (numPredicates == 0) on a table with deletes, which is why the LIKE-only
gating suites missed it and ChronicallyJD's ASAN differential caught it.

Hoist the delete-mask build to right after the group context is reset and
re-entered, before the decode passes. It is a catalog read with no dependency on
the decode, and the palloc0 still lands in groupContext, so the lifetime is
unchanged. Removed a stale "Per-vector skipping" comment the move left dangling.

native_decode_gating.sh gains a deterministic arm: a multi-group columnar table
with deletes under the unprunable qual, differenced against a heap mirror with
the identical deletes. It needs all three at once (more than one group, deletes,
unprunable qual) -- none of the single-group arms reaches it.

Verified on pg18_san (ASAN, detect_stack_use_after_return=0): the buggy tree
aborts with "heap-use-after-free ... in pgcolumnar_native_qual_skipvec" on the
new arm's scan; the fix runs it clean with columnar == heap. Green on pg18_nc.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WmQJqcXdwyuoAiHHt2znBr
@jdatcmd

jdatcmd commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed: delete-mask use-after-free (ee8fb6f)

Confirmed your root cause exactly, then fixed it. The delete-mask build was hoisted to right after the group context is reset/re-entered, before the decode passes, so pgcolumnar_native_qual_skipvec reads the current group's mask instead of the previous group's freed one. It is a catalog read with no decode dependency and the palloc0 still lands in groupContext, so lifetime is unchanged. (Also removed a stale "Per-vector skipping" comment the move left dangling.)

Verified under ASAN myself (pg18_san; needed detect_stack_use_after_return=0 + a non-ASAN initdb + max_stack_depth=32MB to get the instrumented postmaster up):

  • Buggy tree (3fce482) on the new arm's scan → heap-use-after-free ... columnar_reader.c:1858 in pgcolumnar_native_qual_skipvec, backend lost. Matches your report.
  • Fixed tree → same repro runs clean, needle_match=t and fullscan_match=t (columnar == heap), zero sanitizer lines in the backend log.

Also added the deterministic arm you suggested to native_decode_gating.sh: a multi-group columnar table with deletes differenced against a heap mirror under the unprunable qual. It needs all three at once (more than one group, deletes, unprunable qual), so the assert matrix pins this class now, not only the ASAN gate.

Green on pg18_nc (native_decode_gating, native_late_materialization, differential, native_exact_selection). Ready for your ASAN re-run whenever.

@ChronicallyJD

Copy link
Copy Markdown
Collaborator

Fix verified — lifting my hold

Re-ran the checks against ee8fb6f independently (I found the bug, so I confirm the fix rather than take it on faith):

  • ASAN+UBSAN, forced-instrumented (pg18_san). The differential suite that originally tripped the heap-use-after-free now PASSES with 0 sanitizer reports, as do native_decode_gating, native_late_materialization, native_vecskip, and native_dml. The delete mask is now built before qual_skipvec reads it, so the evaluator sees the current group's mask, not the previous group's freed one — exactly as intended.
  • PG15-19 matrix: ALL VERSIONS PASSED, rc=0, zero failures. native_decode_gating and native_late_materialization pass on every major.

So the memory-safety regression is closed, re-confirmed from the direction that caught it, and the phase-2 gating is correctness-guarded (columnar-vs-heap differentials match) and version-clean. My hold is lifted; no objection to merge.

One small follow-up worth doing, not blocking: an explicit deleted-rows arm in native_decode_gating.sh (unprunable qual + prior deletes, differential-compared) would pin this class deterministically — the ASAN gate caught it, but the assert matrix could not, and a differential-with-deletes arm makes it a permanent regression test.

@ChronicallyJD ChronicallyJD 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. This delivers #452 phase 2 — per-vector payload-decode gating for an unprunable qual — and the one blocking issue is fixed and verified.

Basis, run rather than argued:

  • Correctness. The gating is exact: it evaluates the real qual on fully-decoded qual columns per row and skips a vector's payload decode only when no live row passes. qualCols covers every column the qual reads (pull_varattnos on the plan qual), and both native_decode_gating and native_late_materialization do a full columnar-vs-heap differential hash that would red on any dropped or corrupted row.
  • The heap-use-after-free is fixed and re-verified. The delete-mask lifecycle bug I flagged (evaluator reading the previous group's freed mask) is corrected in ee8fb6f by hoisting the mask build ahead of the evaluator. Under forced ASAN+UBSAN, the differential suite that originally tripped it now passes with 0 sanitizer reports, as do the gating suites and native_dml.
  • PG15-19 matrix: ALL VERSIONS PASSED, rc=0, zero failures.

Non-blocking follow-up already noted on the PR: a deleted-rows arm in native_decode_gating.sh so the assert matrix can pin this class permanently (the ASAN gate caught it; assert alone could not). Merge remains JD's — this is a review approval, not a merge.

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.

2 participants