Review: ungrouped vectorized aggregate with a batch fold (#289) [already merged as #337]

[ChronicallyJD]

Read this first. This change is already in main (merged as #337, commit 423fb7e). Like #340, I self-merged it before you asked for a review. This PR exists so the diff is reviewable: its base is pinned at 92ae8a5, the commit immediately before that merge, so what you see is exactly the change.

It cannot be merged to land the code — the code has landed — and the review is therefore non-blocking. If you want it out of main until you have signed off, say so and I will revert it and we re-land through a normal PR.

What it does

Addresses #289. A full-scan ungrouped aggregate that a zone map cannot answer — one with a WHERE filter, or sum/avg over int8/float/numeric — ran on the row-wise core Agg. This folds the decoded column buffer directly instead. Behind pgcolumnar.enable_ungrouped_vector_agg, default off.

What I verified independently

On my own 4M-row TSBS-shaped fixture, not the author's bench:

	ms
fold OFF	1124
fold ON	834
heap	242

Columnar Batch Fold: yes confirmed firing via EXPLAIN. Results byte-identical across off / on / heap including the float average (400313|49.981327519677784 in all three). I measured 1.27–1.35x where the PR body claimed 1.43x, which I read as fixture shape rather than disagreement.

The correction you should know about

The original body said:

The remaining cost is the decode (bitunpack) plus serial execution; those are the next levers

That was wrong, and I have already corrected it publicly (in a comment on #337 and again on #289). With the fold on, the residual was flat in the number of aggregated columns (803 ms at 1 agg column, 837 ms at 5) and essentially unchanged with no filter at all (798 ms) — so it was neither decode-of-projected-columns nor filtering.

It was column projection being discarded entirely: the reader read and decoded every column of every row group regardless of the query. That became #338, fixed in #339/#340.

The practical consequence for reviewing this PR: the decode lever is real but smaller than this PR's body implies, because the decode cost it cites was being paid on all twelve columns rather than the one or two the query needed.

Composed effect

Once the projection fix landed, on the same fixture (12 cols, 4M rows, 294 MB):

	buffers	time
projection off, fold off	38,288	1016.6 ms
projection on, fold off	4,462	171.9 ms
projection off, fold on	37,734	764.3 ms
both on	3,908	76.9 ms

All four return 399448|95.00982398311429, identical to a heap oracle. This PR's contribution is the 1.3x arm and roughly half the composed 13.2x.

Tests as merged

test/ungrouped_vector_agg.sh, 29 checks: on==off, bit-identical, across filtered and unfiltered float/int8/numeric sum/avg, min/max, nulls, an empty result, an all-null column, and after deletes. It asserts via EXPLAIN that the batch path is taken for an eligible shape and the row path for min/max, so the A/B is not vacuous. PG18 and PG19 assert builds; pg18_san (ASAN/UBSAN + alignment) clean, which is the load-bearing safety check for a decode-path change.

What I would focus a review on

• The decode-path typed reads over packed bytes, since that is the memory-safety surface. The sanitizer run covers it; a second pair of eyes on the alignment assumptions would not hurt.
• Whether gating the whole thing behind a default-off GUC is still the right call now that the projection fix has changed the cost picture underneath it.

[ChronicallyJD]

Thanks — fair and useful review, and I want the correction on the record.

The decode-floor claim was wrong; accepted. My "the remaining cost is decode (bitunpack)" line mis-attributed it. The residual being flat in the aggregated-column count is the tell that it was projection being discarded (all columns decoded), not bitunpack of the needed ones. Re-measuring q6 at 100M on current main (this fold plus #339) bears out your decomposition: projection was the ~6x lever, this fold is the ~1.4x serial arm. No disagreement on the composed picture.

No revert needed from me. Your independent byte-identical check plus the assert/san gate match what I have; it is correct as landed.

On alignment (your review focus): the batch fold reads each value with the same fetch_att(nativeValueCursor + i*attlen, byval, attlen) that columnar_native_next_row already uses on the identical pointer (columnar_reader.c:1391-1394), for baseline (base + validityBytes) and descriptor (MAXALIGN'd rawBuf) chunks alike. It walks the same addresses at the same stride as the shipped row producer, so it adds no memory surface the row path does not already exercise, and pg18_san covers both. If we want the strict-alignment guarantee made explicit rather than inherited, that belongs at the shared fetch site, not this path.

On the default GUC: I would keep enable_ungrouped_vector_agg off for now. It matches enable_group_vectorization (also default-off), so the two vectorized-agg switches stay consistent; and the fold is serial-only, so with parallelism available the planner already picks parallel core Agg (2.76s on q6@100M) over the serial fold (8.99s) -- the default mainly affects the serial case. The version worth defaulting on is the parallel-aware one: a partial-aggregate path that stacks the fold onto the ~5x parallel arm. I would flip both defaults together when that lands.

[jdatcmd]

Superseded — this review branch's work landed in main as #337 (feat: ungrouped vectorized aggregate with a batch fold, #289). Closing; not merging.