The batch fold ignores the skip vector, so making decode honour it would answer from uninitialised memory

[ChronicallyJD]

Two facts that are harmless today and become a silent wrong answer together, the
moment anyone makes decode honour the skip vector — which is the obvious next
optimisation and is Phase 1b of #452.

Reported by jdatcmd from reading; verified here before filing.

1. The batch fold fetches the skip vector and never reads it

columnar_vector.c:3161 declares const bool *skipVec, :3167 fills it from
PgColumnarReadFoldGroupInfo, and skipVec[ appears nowhere in the file.

It is correct today for a reason that has nothing to do with the skip vector: the
fold evaluates the scan keys itself, per value, at :3234
(pgcolumnar_batch_key_pass over every key for every row). So it reads vectors
the zone maps already ruled out and re-checks each value by hand. Wasteful, and
right.

2. Decode never skips anything, and "Vectors Skipped" does not mean it does

pgcolumnar_native_decode_chunk (columnar_reader.c:768) takes no skip mask:

static char *
pgcolumnar_native_decode_chunk(MemoryContext cx, Form_pg_attribute att,
                             char *values, uint32 valuesLen,
                             const char *desc, uint32 descLen, int blockCodec,
                             uint32 **outVecRawLen, int *outVecCount)

A chunk is decoded whole at group load. vectorsSkipped — the counter behind
EXPLAIN's Columnar Vectors Skipped — is incremented at columnar_reader.c:1691,
in the branch that advances the value cursors past a vector. So the line means
"not turned into Datums", never "not decoded". #452's text says only that
decompression is not skipped, which understates it: nothing about decode is
skipped, and the EXPLAIN line invites the opposite reading.

Why they are one issue

Skipping a vector's decode leaves a hole in the raw buffer. The row producer
survives that — it steps its cursors past skipped vectors and never dereferences
the gap. The fold does not: it walks every value in the group and re-checks it
against the scan keys, so it would read uninitialised memory and answer from it.
An aggregate returning a wrong number, with no error and no crash.

And it is the common case, not an edge. 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.

What I would do

Not "make the fold read skipVec" as a follow-up to 1b. The ordering is the point:
the fold must honour the skip vector before decode is allowed to skip
anything, and whoever writes the decode change is the person least likely to
look in columnar_vector.c.

Cheapest safe interim, if 1b is not imminent: have the fold assert that no vector
in the group is skipped, so the day someone changes decode they get an assertion
rather than an arithmetic error. That is a few lines and it converts a silent
wrong answer into a loud one.

Separately, Columnar Vectors Skipped should either be renamed to say what it
counts, or gain a sibling once decode can skip, so the two are not conflated in a
plan someone is using to decide whether decode is the bottleneck.

Not measured

This is a reading of the code and a hazard analysis, not a reproduction — the
decode change does not exist, so there is nothing to reproduce yet. Filing it now
rather than after, because the trigger is a change someone is about to make and
the failure it produces is silent.

Refs #452

[ChronicallyJD]

Correcting the interim guard I suggested: as written it would fire today

I proposed "have the fold assert that no vector in the group is skipped". That is
wrong, and jdatcmd caught it. Verified before accepting:

pgcolumnar_native_build_skipvec (columnar_reader.c:1231) sets
rs->nativeSkipVec with flags marked whenever predicates exist and a
per-vector zone map proves exclusion — it returns early only on
rs->numPredicates == 0. And pgcolumnar_batch_shape_eligible requires every
qual to be convertible to a scan key, so the fold runs precisely when predicates
exist.

So skipVec is routinely non-NULL with skips set on the fold path right now, and my
assertion would trip on ordinary aggregate queries. It asserts the condition that
makes the hazard possible rather than the one that makes it real.

The guard belongs on the other side. A flag the loader sets when decode
actually skips a vector, asserted false by the fold:

• false today by construction, because decode takes no skip mask at all;
• true the moment someone makes decode honour it;
• and it fires in exactly the window where decode has learned to skip and the fold
  has not yet been taught to.

Same protection, no false positive, and it fails at the change that introduces the
hazard rather than at the code that merely coexists with it.

Worth having right on the issue, since the interim is the part someone implements
first and a guard that cries wolf on merge day gets deleted rather than fixed.

[jdatcmd]

Measured the obvious fix and it should not be taken on its own. Reporting the negative result and shipping nothing, because the guard I tried to ship with it I could not prove.

Teaching the fold to honour the skip vector costs ~2% and saves nothing today

I implemented it — track the current vector across the per-row loop and continue on a skipped one, after the gather so the value slots stay consumed, exactly as the deleted-row case does. It is correct: a vector is skipped only when a predicate proves no row in it can match, and those predicates come from the same scan keys the loop re-checks, so every row skipped would have failed pass anyway.

A/B on the bench, one row group of 2M rows, 1024-row vectors, narrow range predicate. Premises first, because otherwise the numbers mean nothing:

Vectors Skipped: 1952      Chunk Groups Read: 1      answers identical (3004501500)

arm	median
main	76.8 ms
fold honours skipVec	78.4 ms

Slower, marginally, and the mechanism explains it. The gather still runs for every row — it must, to keep each column's present-index aligned — so the only work saved is the scan-key re-check and the aggregate apply. That is less than the per-row vector tracking costs. (A's spread is 2.5 ms against a 1.6 ms difference, so "no benefit, possibly a small cost" is the honest reading rather than a firm 2%.)

So this belongs inside the decode change, not before it. Once decode skips vectors the saving becomes real — the fold stops touching data that was never produced — and the 2% is repaid many times over. Landing it now buys a regression and no safety, because the fold is already correct while decode produces every vector.

The tripwire: attempted, unproven, not shipped

The interim suggested here was a flag the loader sets when decode skips, asserted by the fold, so the hazard fails loudly rather than arithmetically. I wrote it — flag on the read state, threaded through PgColumnarReadFoldGroupInfo, elog(ERROR) in the fold — and it builds clean with all the aggregate suites green.

But I could not make it fire. Simulating the hazard (setting the flag whenever a skip vector exists) produced no error, because my fixtures never reached the batch fold with a skip vector present: Aggregate -> Custom Scan (PgColumnarScan) is the scalar path, and the vectorized node needs a shape I did not reproduce before running out of road.

A guard nobody has seen fail is exactly what this repository keeps re-learning not to trust, and shipping one as protection against a silent wrong answer would be the worst place to make that mistake. So it is reverted rather than merged. The construction is sound and the next person can lift it from this comment; what it needs is a fixture that provably reaches the fold and has vectors ruled out, which ungrouped_vector_agg.sh builds (60,000 rows, stripe_row_limit => 2000, its own GUC and NOPAR settings) and my ad-hoc table did not.

The ordering constraint this issue exists for is unchanged and now has a measurement attached: the fold must learn to skip in the same change that teaches decode to skip — not before, because it costs; and not after, because that window is the silent wrong answer.

[ChronicallyJD]

Resolved by #539 and #541, both merged. Verified on main rather than inferred
from the PR descriptions.

The two facts this issue named

1. "skipVec[ appears nowhere in the file." It does now:

columnar_vector.c:3270   vecSkipped = (curVec < vcount && skipVec[curVec]);
columnar_vector.c:3284   if (!vecSkipped)          # consume the slot, do not read it
columnar_vector.c:3296   if (vecSkipped) continue; # value slots already consumed above

The fold walks the per-vector map it was already being handed. The present index
still advances across a skipped vector, exactly as it does across a deleted row,
because the stream is packed by presence.

2. "The moment anyone makes decode honour the skip vector." That happened in
#539, and this issue's guard is what caught it: teaching decode to skip fired the
tripwire on the first run, with the error text telling the author to teach the
fold in the same change. It did its job on exactly the change it was written for.

What replaced the guard

Not nothing. The tripwire was an ordering guard and the ordering no longer needs
enforcing, so what survives is narrower: a skip reported without the per-vector
map to honour it must ERROR rather than guess.

Correctness is now covered by a test rather than by a refusal to run.
test/native_vecdecode.sh exercises the fold arm with
enable_ungrouped_vector_agg=on, asserts Columnar Batch Fold: yes as a
premise, and — because a fold reading undecoded holes returns the right answer
on ordinary data — assert builds poison skipped vectors with 0xA5 and the suite
picks a predicate that accepts that value. With the fold's skip handling
removed, that check reports 32,696 rows where heap says 3,000.

That was measured, not assumed: before the poison existed, a fold reading every
hole passed all 20 of the suite's checks.

Not claimed

The accounting half is separate and still open as #542: Columnar Vectors Skipped is incremented on the row path only, so the fold arm reports 0 while
decoding 1 of 59 vectors. That is a reporting defect, not the correctness hazard
this issue is about, and #543 addresses it.

Closing as fixed.