feat: parallel-aware ungrouped vectorized batch fold (#289 phase 5/6)

[ChronicallyJD]

What

Makes the ungrouped vectorized batch fold (#337) parallel-aware (#289 phase 5/6). The serial fold runs in one process, so it caps at ~1.4× over core Agg and leaves the ~5× parallel scan (gap 23) on the table. This adds a second upper path — a parallel-aware partial ColumnarAgg under a core Gather + Finalize Aggregate — so the fold's per-worker speedup stacks on the parallel scan.

Opt-in behind a new GUC pgcolumnar.enable_parallel_vector_agg (default off). First slice: count(*), count(col), and sum/avg over float4/float8 — the kinds whose transition state is a plain, non-internal value the fold already holds. Everything else keeps the serial node or the ordinary core Agg.

Measured — q6 @ 100M (bench, median of 5, interleaved)

SELECT count(*), avg(usage_system) FROM cpu_pgc WHERE usage_user > 90.0

arm	time	vs core parallel
parallel fold (this PR)	1.83 s	1.49× faster
core parallel Agg	2.72 s	baseline
serial fold (#337)	8.8 s	— (→ 4.8× parallel scaling)
TimescaleDB (serial)	3.51 s	—

Result matches core's own parallel aggregate to ~15 digits; count exact. (TimescaleDB's parallel path currently errors on the bench with a broken-compressed-chunk read, so only its serial number is shown.)

How it works

• Each worker claims distinct row groups through the same shared atomic the base parallel scan uses (ColumnarReadSetParallelCounter, gap 23) and folds them column-at-a-time, emitting one per-worker transition-state tuple; a core Finalize combines them — int8pl for count, float8_combine + float8_avg for avg(float8).
• Overflow parity is preserved: the partial passes through the Youngs-Cramer Sxx, and float8_combine re-derives and re-checks it — so a cross-worker overflow raises exactly as core would (a decompose-to-sum/count shortcut would silently under-raise).
• The partial target is core's own UPPERREL_PARTIAL_GROUP_AGG reltarget, so the partial and final Aggrefs stay structurally related and setrefs matches them. Uses only public planner/executor APIs (mark_partial_aggref, create_agg_path, create_gather_path, fetch_upper_rel, construct_array_builtin, the CustomScan parallel callbacks) — no core nodeAgg internals.
• When the parallel arm is added it supersedes the serial node (its Gather runs leader-only when no workers start), so the serial node — priced at the cheap Gather cost by pgcolumnar.enable_metadata_count is an orphaned GUC, and count(*) loses to a parallel scan by default #133 — is added only as the non-parallel fallback; keeping it would let the mispriced serial plan out-cost the genuinely parallel one.

Correctness guards

A parallel path returns wrong answers, not crashes, when it is wrong, so:

• the partial reader errors if opened without a shared counter (else every worker reads every group and the Finalize sums the duplicates);
• writes/deletes are flushed once in the leader's InitializeDSM before workers launch (a worker cannot see the leader's unflushed in-transaction buffers);
• the one unsafe fallback — an absent column found mid-scan after the counter advanced (ADD COLUMN on old row groups) — errors rather than under-count; a shape that is not batch-foldable from the start (a NULL test, a non-btree filter) still runs correctly on the row path with the counter shared.

Tests / gate

test/parallel_vector_agg.sh (registered in run_all_versions.sh) asserts the plan Finalize → Gather → parallel partial ColumnarAgg (batch fold: yes) is actually chosen, count is exact vs a serial oracle, avg/sum(float) match core parallel Agg within reassociation tolerance (a parallel fold is order-nondeterministic — the oracle is core parallel, never the serial fold), and null/empty/more-workers-than-groups/not-batch-foldable behave. 14/14 on PG18 assert.

Gate: preflight build on all five majors (15–19), full assert matrix on PG18 and PG19, and ASAN/UBSAN (pg18_san) on the fold read path.

🤖 Generated with Claude Code

[ChronicallyJD]

Reviewed. The design is sound and the correctness guards are the right ones -- erroring when the partial reader is opened without a shared counter, and erroring rather than under-counting on a mid-scan absent column, are both the correct direction to fail in. Preserving the Youngs-Cramer Sxx through the partial so float8_combine re-checks overflow, instead of shortcutting to sum/count, is a detail that would have been easy to get wrong quietly.

But it does not work on PG15, PG16, or PG17, and the suite cannot tell you why.

The bug

avg over float errors outright on three of five majors:

ERROR:  type 701 not supported by construct_array_builtin()

701 is FLOAT8OID. Checked on every major, same fixture, same query (SELECT avg(v) FROM t WHERE k < 700, 2M rows):

PG15	PG16	PG17	PG18	PG19
ERROR	ERROR	ERROR	71.9989425	71.9989425

sum(v) is fine everywhere -- it returns 100798519.5 on all five. Only the average is affected, because only it needs the float8[] transition array. avg(w)::float8 over float4 fails the same way.

It builds on all five, so the five-major preflight build passes. The failure is at runtime, and the gate that would have caught it (the assert matrix) was run on PG18 and PG19 only, which is exactly where it works.

The test hides the cause

CI reports this, which is not what is wrong:

FAIL  avg(v) parallel-vec ~= core parallel agg: got [ERROR:  syntax error at or near ":"

reldiff interpolates the two measured values straight into SQL:

q -c "SELECT CASE WHEN abs(($vec::float8)-($core::float8)) <= 1e-6*(abs($core::float8)+1) ..."

When the measurement returns an error string rather than a number, $vec is empty by the time it lands in that string and the query becomes abs((::float8)-(::float8)), so psql reports a syntax error. The real error is discarded before anyone sees it. I only found construct_array_builtin by running the two inner queries by hand.

Worth fixing regardless of the portability issue: a comparison helper should assert that both sides are numeric before differencing them, so a failure names itself. As written, any error in either arm reports as a syntax error, which sends the reader to the wrong file.

Suggested fix for the portability issue

Two options, and I do not have a strong preference between them:

1. Version-guarded construction. construct_array_builtin gained float8 support after PG17; on PG15-17 build the transition array with construct_array(..., FLOAT8OID, sizeof(float8), FLOAT8PASSBYVAL, TYPALIGN_DOUBLE) instead. Keeps the feature on all five majors.
2. Gate the parallel path to PG18+. Simpler, and the serial fold from feat: ungrouped vectorized aggregate with a batch fold (#289) #337 still applies below that. Costs the feature on three majors.

Whichever you pick, the assert matrix needs to run on a major where it is expected to work and one where the fallback path is taken, or the same gap reopens.

Note on the measurements

The q6 @ 100M numbers may predate #339, which landed column projection. Before this PR the scan read every column of every row group regardless of the query, so the core parallel Agg baseline of 2.72 s and the serial fold's 8.8 s were both paying I/O for eleven columns nobody asked for. If those were taken on a tree without #339, the 1.49x here is measured against an inflated baseline and the real figure is likely different -- possibly better, since the fold's per-worker win is a larger share once the I/O shrinks. Worth re-running on current main before the number goes in the changelog.

Not blocking on that; the portability bug is the blocking item.

[ChronicallyJD]

Verified the fix independently on all five majors. construct_array with explicit float8 params is the right call -- it keeps the feature everywhere rather than gating it to PG18+.

Same fixture and query as before (SELECT avg(v) FROM t WHERE k < 700, 2M rows), parallel-vec arm vs core parallel arm:

	PG15	PG16	PG17	PG18	PG19
before	ERROR	ERROR	ERROR	71.9989425	71.9989425
after	71.9989425	71.9989425	71.9989425	71.9989425	71.9989425

Both arms agree to the digit on every major. Full suite: 14/14 on all five (PG15, 16, 17, 18, 19).

One thing from the review is still open

The reldiff masking issue is unchanged (test/parallel_vector_agg.sh:65-70). It still interpolates both measured values straight into SQL:

vec="$(q -c "$PAR $UG $PP" -c "SELECT $1 FROM t WHERE k < 700")"
...
q -c "SELECT CASE WHEN abs(($vec::float8)-($core::float8)) <= ..."

So any error in either arm still reports as syntax error at or near ":" rather than naming itself. That is exactly what cost the time here: the CI red pointed at SQL syntax, and construct_array_builtin only surfaced when I ran the two inner queries by hand.

It is not blocking -- the bug it hid is fixed and the suite is green on all five. But the next failure in this suite will be just as opaque as this one was, and the fix is small: assert both sides are numeric before differencing, and fail with the raw value when they are not.

Your call whether to fold that in here or leave it. With the portability bug fixed and 14/14 across the matrix, I have no objection to this landing.

[ChronicallyJD]

Follow-up on the baseline note in my review, now measured rather than predicted.

Composed run on current main with this PR overlaid, 8M rows / 12 columns / 588 MB, PG17:

arm	buffers	time
core parallel agg, no fold	9,753	76.1 ms
+ parallel fold (this PR)	8,325	39.5 ms

That is 1.93x over core parallel agg, against the 1.49x in the PR body. Your figure was measured on a tree that predated #339, so both arms were reading all twelve columns to answer a two-column query; once projection removes that I/O, the fold's per-worker win is a larger share of what is left. So the number moved in your favour, not against it.

Caveat so this is not over-read: mine is 8M on a different box, yours is 100M on the bench. These are not like-for-like, and I am not proposing you substitute my number for yours. The point is only that the 100M figure is measured against a baseline that no longer exists and should be re-run on current main before it lands in a changelog or a README.

Full-stack context, same fixture: baseline with neither projection nor fold, serial, is 1951.6 ms at 76,793 buffers. All three levers together bring that to 39.5 ms at 8,325 buffers, and this PR is the last 1.93x of it.

Correctness held throughout: count exact against a heap oracle, average agreeing to 12 significant figures with the residual being cross-worker float reassociation -- which is exactly why comparing against core parallel rather than a serial oracle, as your suite does, is the right call.

[ChronicallyJD]

Re-verified at 97ebc5a1 with the delete coverage added: 16/16 on all five majors (PG15, 16, 17, 18, 19), run locally.

The H2 case is the one worth having. A worker is a separate backend, so an unflushed in-transaction delete buffer in the leader is invisible to it, and the failure mode is workers counting deleted rows and quietly diverging from serial -- a wrong answer, not a crash. That is not something reasoning about the code reliably catches, so pinning it with a test that runs the fold inside the same transaction as the DELETE is the right move.

No objection to this landing from my side. Summary of what I checked independently: the construct_array portability fix on the three majors it was broken on, the full suite on all five, and the composed performance number on current main (1.93x over core parallel agg rather than the 1.49x in the body, because the body's baseline predated #339).

The only outstanding item from my review is the non-blocking one: reldiff at test/parallel_vector_agg.sh:65 still interpolates both measured values into SQL unguarded, so the next failure in this suite will report as syntax error at or near ":" rather than naming itself. Entirely your call whether that is worth a follow-up.

[ChronicallyJD]

Status update since opening — all green, ready for review.

Three follow-up commits:

• 8ea5ff8 — CI caught a real cross-version bug my local gate missed: construct_array_builtin(..., FLOAT8OID) errors on PG15–17 ("type 701 not supported"; that helper learned float8 only in PG18). Switched to the general construct_array with explicit float8 params — exactly what core's float8_accum uses. parallel_vector_agg now 14→16/16 on pg15a/16a/17a/18a assert.
• 97ebc5a — added coverage for the design's H2 hazard (a worker is a separate backend and can't see the leader's unflushed in-transaction delete buffer). The partial node flushes write+delete state in the leader's InitializeDSM before workers launch; the new test deletes rows and runs the parallel fold in the same transaction, and the result (1384000|-40) matches the serial oracle exactly.

Gate:

• CI: green across build PG15–19 (both arches) + suites PG17 + PG18.
• Local: preflight builds clean on all five majors; full assert matrix on PG18 and PG19 (the only two non-green suites — analyze_stats, column_projection — fail identically on clean main in that container: a wall-clock ratio too tight for its slow ANALYZE, and a missing bc for the buffer probe; neither is code-related).
• ASAN/UBSAN (pg18_san): clean on the parallel fold + the agg suites it shares the read path with.

Measured (q6 @ 100M, median of 5, interleaved): parallel fold 1.83 s vs core parallel Agg 2.72 s (1.49×); serial fold 8.8 s; result matches core parallel to ~15 digits, count exact.