harden the parallel flush: cancel-safe cleanup + skip new-in-xact relations (#445 slice 4a)

[ChronicallyJD]

#445 slice 4a: harden the parallel flush (cancel-safe + new-in-xact)

The two robustness fixes from the slice-4 work, split out per review so they can land on their own. No default change — pgcolumnar.parallel_flush stays off; the default flip is a separate PR behind a size gate (small/frequent flushes must not pay per-flush worker-spawn cost — measured 3.6× on a 1000-row-flush load).

1. Cancel-safe cleanup (the leak-window flagged on #591)

The parallel flush's wait/collect is wrapped in PG_ENSURE_ERROR_CLEANUP. On an error — a statement cancel inside WaitForBackgroundWorkerShutdown is the realistic one — it terminates the workers and frees every published-but-uncollected pinned output segment. The collect loop invalidates each handle as it frees it, so the cleanup only attaches segments still pinned by a worker and never double-frees. Closes the leak-to-postmaster-restart window.

2. Skip parallel for a new-in-transaction relation

CREATE TABLE ... USING pgcolumnar; INSERT ... (or CREATE TABLE AS) in one transaction: the table's pg_class row is uncommitted, so a worker's fresh-snapshot table_open fails. Before, that path registered workers, had them all fail, logged a WARNING, and redid the flush serially. Now rel_new_in_current_xact() (via rd_createSubid) detects it and keeps the flush serial up front — silent, no wasted registration, no double work.

Verification

• Both fixes exercised with pgcolumnar.parallel_flush=on: a committed-table load uses workers and is EXCEPT-equal to heap (diff=0); a CREATE+INSERT+COMMIT load stays serial with 0 worker-failed WARNINGs and is byte-correct (diff=0).
• Byte-identical, off and on: the full write/concurrency/crash suite set passes under forced ASAN+UBSAN (0 sanitizer reports) and pg18a assert (0 crashes). -Wshadow/-Werror clean.

The default flip + size gate follows in its own PR, with a small-flush timing beside the large-flush one.

🤖 Generated with Claude Code

https://claude.ai/code/session_017N82wDmsawqSWoWkmxtHmW

[jdatcmd]

The two fixes are right — but default-on regresses small/frequent flushes 3.6x

First, the "CI failure" is a 60-minute timeout, not a test failure

The two red checks are cancelled, not failure. The run's own timing shows it: every build succeeded, and suites (PG 17)/suites (PG 18) ran 06:09:42 → 07:09:58 — exactly 60 minutes — then were cancelled. The suites didn't fail; they didn't finish. I reproduced the whole matrix locally and every suite passes (only temporal fails here, and only because my container lacks btree_gist).

Why default-on makes them time out — and why it's a real regression

Default-on spawns a worker pool on every stripe flush, and register + wait + collect + detach is a fixed cost that only amortises when the flush is large. Measured, 50 flushes in one load (stripe_row_limit=1000, 50k rows, 5 cols), pg18 non-assert:

	load
parallel_flush=off	47 ms
parallel_flush=on	170 ms (3.6x)

A single large flush is fine (5000-row/one-flush load is 71 ms either way — the win case I approved on #591 was a 500k-row load). But the suite matrix does thousands of small flushes across ~130 suites, so the per-flush worker cost balloons the run past the 60-minute budget. The same is true of any real workload with frequent small commits or a low stripe_row_limit (#440 recommends exactly that for lookup-heavy tables) — they'd all regress ~3.6x.

What I'd change

Keep both hardening fixes — the PG_ENSURE_ERROR_CLEANUP cancel path (handle invalidated as freed, no double-free) and the rel_new_in_current_xact serial guard are correct and worth having on their own.

But the default flip needs a size gate first: dispatch to workers only when the flush is large enough to amortise worker spawn — e.g., total encoded bytes, or rows × columns, above a threshold; below it, stay serial. Then:

• CI suites (small writes) run serial → no timeout;
• small / frequent-commit workloads don't regress;
• large loads still get the ~14% win #591 measured.

Without that gate, on-by-default is net-negative for the common small-flush case. I'd split this: land the two fixes now, and make the default flip its own PR behind the threshold with a small-flush timing beside the large-flush one. Happy to re-review either promptly.

[ChronicallyJD]

You're right, and I reproduced it rather than take it — same shape as your slice-2 catch, and my miss again: I measured the win case (300k–500k-row single flushes) and never the adversarial small/frequent-flush shape where the fixed per-flush worker-spawn cost dominates. 50k rows, 5 cols, pg18n, median of 3, varying stripe_row_limit:

rows/flush	flushes	off	on	on/off
1000	50	0.07 s	0.25 s	3.57x
2000	25	0.07 s	0.17 s	2.43x
5000	10	0.07 s	0.10 s	1.43x
20000	2	0.10 s	0.11 s	1.10x
50000	1	0.12 s	0.13 s	1.08x

Confirms your 3.6x, and shows the fixed spawn cost (~4 ms/flush) only amortises on a large flush. (This 5-col table never wins even at one 50k-row flush — the −14% needs many columns and size — which says the gate should key on total work, not row count alone.)

Doing exactly what you asked:

1. I've reset this PR to the two hardening fixes only — the PG_ENSURE_ERROR_CLEANUP cancel path and the rel_new_in_current_xact serial guard, both correct on their own. Retitled; ready for re-review as a standalone.
2. The default flip goes in its own PR behind a size gate: dispatch only when the flush's total buffered value bytes exceed a threshold; below it, stay serial. I'll set the threshold from a measured win-crossover (total bytes, spanning narrow→wide) and post the small-flush timing beside the large-flush one — CI's small writes run serial (no timeout), frequent-commit / low-stripe_row_limit (Record what the external validation established (#437) #440) workloads don't regress, large loads keep the win.

Thanks for catching it before it shipped. Re-review of the hardening PR whenever you have a moment.

[ChronicallyJD]

Added one commit (b5e9aae): the GUC description now documents the measured profile and that it stays opt-in (the flip is declined — full data on #445). This PR is still just the two hardening fixes plus that doc correction.

[jdatcmd]

Approve — the right resolution. Re-verified (PG18).

The revision does exactly what the review asked: keep the two hardening fixes, drop the default flip. parallel_flush stays off by default with the measured perf profile documented, so it's opt-in for the large-flush case where #591 measured the win, and it can't regress small/frequent-flush workloads or blow the CI budget.

Re-verified on the new head (b5e9aae):

• Default is OFF — proven by CI itself: suites (PG 17)/suites (PG 18) now complete green instead of running to the 60-minute cancel, which only happens if the suites' (unSET) writes take the serial path. The GUC description says so and the run confirms it.
• Both hardening fixes intact and correct — PG_ENSURE_ERROR_CLEANUP cancel path and the rel_new_in_current_xact serial guard. native_cancel, cancel_decode, native_ctas, native_backend_crash, differential, native_dml all pass.
• The opt-in ON path's per-flush cost on many small flushes is unchanged and now documented rather than shipped as a default — which is the correct place for it.

Good call splitting it this way. A future default-on behind a size threshold (dispatch only when the flush is large enough to amortise worker spawn) is the natural follow-up, but nothing here needs it. Approving; merge is jd's.