fix: the batch fold must refuse a group whose decode skipped vectors (#512)

[jdatcmd]

The batch fold does not honour the skip vector. It reads every vector and reaches the right answer by re-checking every value against the scan keys — correct only while decode produces every vector, which today it does, because pgcolumnar_native_decode_chunk takes no skip mask at all. skipVec is fetched into a local that is never indexed.

The hazard, now stated as a number rather than a code reading

@ChronicallyJD measured it on native_vecskip's fixture — 8192 rows, one row group, eight 1024-row vectors, monotonic id so each zone map is tight. Same table, same predicate, only the node differing:

node	Columnar Vectors Skipped
scalar custom scan	7
vectorized aggregate (Batch Fold: yes)	0

The scalar node steps past seven of eight vectors. The fold traverses all eight. That is why holes in the decoded buffer would be read: the fold genuinely walks vectors the zone maps ruled out.

The day decode is taught to skip them (#452 phase 1b, the obvious next optimisation), that 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 because it steps its cursors past skipped vectors. And pgcolumnar_batch_shape_eligible requires every qual to be convertible to a scan key, so the fold runs precisely when predicates exist, which is exactly when vectors get skipped. Common case, not an edge.

What that 7-against-0 does not say. It is a hazard argument, not a cost one. The work the fold does on those rows is a scan-key re-check, not materialisation. Measured on a 2M-row single-group fixture with 1,952 vectors skipped, teaching the fold to honour skipVec runs 76.8 ms → 78.4 ms — slower — with identical answers, because the gather must still run to keep each column's present-index aligned. So the saving is only the key test and the aggregate apply, which is less than the per-row vector tracking costs. Both numbers belong here, or someone reads 7-against-0 as a performance defect and optimises a 2% regression into place.

That measurement is also the argument for where the fix belongs: honouring skipVec costs today and pays only once decode skips, so it goes in that change, not before it.

The guard

The loader reports whether it skipped any vector's decode — false always, today — and the fold refuses a group it cannot read safely:

ERROR:  pgcolumnar: the vectorized aggregate cannot fold a row group whose decode
skipped vectors (#512); teach this loop to honour the skip vector in the same
change that makes decode honour it

Not the form first proposed. "Assert no vector is skipped" would trip on ordinary aggregate queries, since the fold runs precisely when predicates exist and predicates are what cause skipping. This asserts the condition that makes the hazard real, not the one that makes it possible.

Proof

test/native_fold_skipguard.sh proves the guard is reachable, which is the part that is easy to get wrong, and carries the two premises that make it worth anything:

1. the reader must skip vectors for this predicate, and
2. the query must reach the fold.

Either alone lets the suite pass forever while guarding nothing.

Fired by simulating the hazard exactly as a decode-skipping change would — the loader reporting skipped wherever a skip vector exists — with the .so fingerprinted across both arms. Guard present and hazard simulated: the error above. Guard present, no simulation: 4/4 green.

An earlier attempt at this guard could not be proven and was correctly not shipped. The cause was a wrong GUC name — enable_vectorization rather than enable_ungrouped_vector_agg — so the fixture ran the scalar path and the guard was unreachable. The guard was fine; the probe was not.

Gate

PASS PG15 (130 ran, 5 skipped) · PASS PG16 (130) · PASS PG17 (130)
PASS PG18 (133 ran, 2 skipped) · PASS PG19 (135 ran, 0 skipped)
ALL VERSIONS PASSED · exit=0

native_fold_skipguard=PASS on all five and absent from every skip list.

Refs #512, #452

[ChronicallyJD]

Reviewed. The analysis, the scoping and the placement argument are all right, and I could not find a hole in any of them. One measured gap in the test, non-blocking, and it is about what the suite protects rather than what the guard does.

What I verified rather than took

• The hazard is confined to one site. PgColumnarReadFoldGroupInfo has exactly one caller (columnar_vector.c:3170), so the guard covers the whole exposure. I went looking for a second fold path specifically because I found three separate stats-capture sites in this file earlier tonight, and a guard threaded through one of several paths is the classic miss. There is only one.
• The grouped aggregate is genuinely safe, not merely unmentioned: batchEligible/batchFolded live on the ungrouped state, and the grouped path reads through the row producer, which steps its cursors past skipped vectors.
• The oracle is independent. want_s=$(( (30000 + 30100) * 101 / 2 )) is closed-form heap arithmetic, not the engine's own answer restated, so it can disagree with the code.
• Both premises earn their place, and the second is the one your first attempt lacked.

The gap: the suite passes with the guard disabled

The suite never makes decodeSkipped true — it is false always, today — so the ereport branch does not execute during the run. Measured, on this branch, by replacing the condition with a version that can never fire while leaving the plumbing intact:

if (false && decodeSkipped)

arm	result
as submitted	4 pass / 0 fail
guard neutered	4 pass / 0 fail

So native_fold_skipguard.sh proves the fold is reachable — which is exactly what its header claims — but it does not protect the guard. Your loader-patch simulation proved the guard fires; that proof is not in the tree, so a later change that breaks the flag plumbing (drops the out-parameter, sets it on the wrong path, inverts it) reddens nothing and the guard silently stops guarding. Given the whole point is to catch a change nobody has made yet, the plumbing is the part most likely to rot.

What I am not asking for

I am not asking you to add a test seam to production code on my say-so. I declined exactly that on analyze() earlier — build the fixture, do not move the seam — and it would be inconsistent to demand one here.

Options as I see them, and this is your call:

1. Ship as is, with the header saying plainly that the ERROR path is proven by manual simulation and is not regression-protected. Honest, and the guard still does its job the day it matters.
2. A debug-only way to set the flag (pgcolumnar.debug_force_decode_skipped, or a #ifdef USE_ASSERT_CHECKING hook) so the suite can assert the ERROR text. Buys a real regression test for the plumbing, at the cost of a knob that exists only for the test.
3. Leave it to phase 1b: the change that makes decode skip is also the change that makes this guard reachable naturally, and it must add that coverage anyway.

I lean 1 or 3 — the cost of 2 is a permanent knob for a transient gap — but the header should say which, because "this suite proves the guard is REACHABLE" reads as a stronger claim than the suite makes.

One interlock

The premise reads Columnar Vectors Skipped off the scalar arm because the aggregate node does not print it. #522 fixes that. Once both land, this suite can read the counter from the arm actually under test, which removes the same-reader assumption the comment currently has to make.

[ChronicallyJD]

Re-checked against current main, since this branched six commits back (#521, #522, #524 landed underneath it) and my own #527 is open against columnar.h, which this also touches.

merges onto a0a3c41	cleanly
native_fold_skipguard	4 pass / 0 fail
ungrouped_vector_agg	45 pass / 0 fail
build warnings	0
#523 then #527 in sequence	merges cleanly — no columnar.h collision, merge order does not matter

Nothing here changes my earlier read: the analysis and scoping hold, and the only open point is that the suite still passes with the guard neutered (if (false && decodeSkipped) → 4/0 on both arms), which is about what the suite protects rather than what the guard does. Non-blocking, and your call between the three options.