[OffgridwithJD]

@jdatcmd Fixes #1747. Stacked on #1746 (fix/1745-pflush-double-cancel), because it touches the same unpin code: merge #1746 first. This PR's base is that branch, so the diff is this one commit.

The leak, measured on PG17 main

A worker pinned its OUTPUT segment and then ran table_close, PopActiveSnapshot and CommitTransactionCommand before writing DONE. An ERROR in between wrote FAILED. The collect loop unpins only DONE slots, and the error cleanup doesn't run on the normal path, so the segment stayed pinned until postmaster restart.

There's no fault hook for that window, so the measurement used an ereport(ERROR) injected right after dsm_pin_segment, gated by an environment variable given only to the test postmaster. That injection is not committed, in this PR or anywhere else, so no production build carries it.

run: 300000 rows, 8 columns, 2 stripes × 4 workers	WARNINGs "worker N failed"	rows	new segments in pg_dynshmem
main + injection	8	right	8
this PR + injection after the new pin	8	right	0

Reachability: in the worker, pgcolumnar's own pre-commit callbacks find nothing pending, so the window raises only through out-of-memory or another preloaded library's transaction callback. It's rare, but each occurrence keeps a segment pinned until restart, and nothing reports it.

The fix (src/columnar_write_state.c)

1. Commit first, pin last. The record is built in TopMemoryContext. The worker commits, then creates, fills, names (handle before pin, as in fix: a second cancel during the parallel flush's error cleanup frees what the workers pinned (#1745) #1746), pins and detaches the segment, then writes DONE. Nothing that can raise follows the pin. A segment created but not yet pinned is freed when the worker exits.
2. Belt and braces. The collect loop frees any slot it didn't collect that names a segment. That's what turned the injected run above from 8 to 0.

Test: parallel_flush_optin arm 5 and its twin

A source-structure check that runs in CI with no fault build. In pgcolumnar_parallel_flush_worker, with comments blanked, only dsm_detach(outseg); may sit between dsm_pin_segment(outseg); and the DONE write. A behavioural test would need a fault-injection build: PostgreSQL's injection points aren't enabled in the PGDG packages CI uses or in my local builds, and pgcolumnar has no test-hook build, so this check is the part CI can run. The injected measurement above covers the behaviour.

run	shell	twin
#1746's head	9 + 1 failed: a flush worker runs nothing that can raise between pinning its output and reporting DONE (it read dsm_detach(outseg); table_close(rel, AccessShareLock); PopActiveSnapshot(); CommitTransactionCommand();)	the same name
this PR, PG15–19	10/10 on each	10/10 on each
commitlast (the worker commits after the pin again), PG17	that name red	that name red

nologin_workers (15/15) and sorted_pathkeys (113/113) pass on PG17, and the build is warning-free.

Bookkeeping, on #1746's numbers: one ledger row, dated by commitlast. Census: never 3886 → 3886, dated 2288 → 2289, rows 6174 → 6175. cluster_tests 2757 → 2758, plus a §262 row in TESTS.md and a CHANGELOG Fixed entry.

🤖 Generated with Claude Code

[jdatcmd]

@OffgridwithJD Requesting changes on #1748 at dbec2bc1.

The C fix works: with an ereport(ERROR) injected after the pin, #1746's head leaks 8 segments and this head leaks 0. Two things block approval:

1. This PR is red in CI for the reason in my review of #1746. suites (PG 17) and suites (PG 18) fail on premise: the cheap workers published their segments before the second cancel: got [no (new segments=[0])]. Arm 4 restarts with a bare pg_ctl, which refuses to run as root. Once #1746 restarts through pgc_pg, rebase this PR.
2. The structure check stops one statement too early. It reads from dsm_pin_segment to the DONE write. An ERROR anywhere before the end of the PG_TRY block also leaks the segment, because PG_CATCH overwrites DONE with FAILED. A plausible re-ordering slips past the check.

Runs (PG17, as root, restart through pgc_pg applied locally in both trees)

run	shell	twin (serial)
#1746's head (parent)	9 + 1 failed: a flush worker runs nothing that can raise between pinning its output and reporting DONE: got [dsm_detach(outseg); table_close(rel, AccessShareLock); PopActiveSnapshot(); CommitTransactionCommand();]	9 + 1 failed, the same name
this head	10/10	10/10
this head, PR test as pushed	9 + 1 failed (arm 4's premise, new segments=[0])	not run
commitlast (the 3 calls moved back after the detach)	9 + 1 failed, the same name	9 + 1 failed, the same name
nobelt (the collect loop's new free removed)	10/10	10/10

compare_to_bash.py: 10 literal matches, 0 missing. The pyarrow import scan prints [].

The leak, measured (300000 rows, 8 columns, 2 stripes × 4 workers)

Each injection is a measurement-only ereport(ERROR) that fires when the postmaster's environment sets PGC_INJ. I applied it in my worktree only and never committed it.

tree + injection	WARNINGs worker N failed	rows equal to the generator	segments left
#1746's head, after the pin	8	true (300000)	8
#1746's head, in the encode loop	8	true	0
#1746's head, after the handle and before the pin	8	true	0
this head, none	0 (8 of 8 columns by 4 worker(s))	true	0
this head, in the encode loop	8	true	0
this head, right after the new commit	8	true	0
this head, after the handle and before the pin	8	true	0
this head, after the pin	8	true	0
nobelt, after the pin	8	true	8

No WARNING is lost, and the serial fall-back returns the right rows in every case. A worker that fails before it pins leaves nothing behind, both at the parent and at this head.

Can the structure check fail for the right reason?

Mostly, yes. It reads the text between the pin and the DONE write, so commitlast goes red in both harnesses, and so would any raising call placed in that stretch. A harmless rename of outseg would also go red, with the message "not both in the worker". That red is not a defect, but it fails closed.

It misses one re-ordering a reviewer could plausibly write: commit after the DONE write, "commit at the end, as it used to be". I applied that edit and removed the belt-and-braces block in my worktree:

structure check reads: 'dsm_detach(outseg);' -> PASS
PGC_INJ=after_done: 8 WARNINGs, 300000 rows, content equal to the generator: true
RESULT: segments base=2 after=10 left=8

The belt is what makes that re-ordering harmless today, and no test covers it: nobelt passes 10/10 in both harnesses.

Suggested fix: end the stretch at PG_CATCH();, not at the DONE write, and expect dsm_detach(outseg); pg_atomic_write_u32(&me->state, PFLUSH_DONE);. I ran that boundary over three versions of the source:

head               pin..PG_CATCH reads: 'dsm_detach(outseg); pg_atomic_write_u32(&me->state, PFLUSH_DONE);'
commit-after-done  pin..PG_CATCH reads: 'dsm_detach(outseg); pg_atomic_write_u32(&me->state, PFLUSH_DONE); if (getenv(...)) ereport(...); table_close(rel, AccessShareLock); PopActiveSnapshot(); CommitTransactionCommand();'
parent (#1746)     pin..PG_CATCH reads: 'dsm_detach(outseg); table_close(rel, AccessShareLock); PopActiveSnapshot(); CommitTransactionCommand(); pg_atomic_write_u32(&me->state, PFLUSH_DONE);'

Error-handling rules

• This commit adds no HOLD_INTERRUPTS, no FlushErrorState and no PG_TRY.
• The new free in the collect loop runs inside PG_ENSURE_ERROR_CLEANUP, not in its callback. If its dsm_unpin_segment raised, the cleanup would run as designed.
• The cleanup callback can still raise. This is not introduced here: it reproduces on main. If anything raises in the collect loop after dsm_unpin_segment and before the loop invalidates the handle, the callback raises can't attach the same segment more than once and 3 segments are left. Setting slots[i].outHandle = DSM_HANDLE_INVALID right after the unpin fixed it in my worktree: the injected error was reported and 0 segments were left. A clean run left 0 segments with correct rows. Details are in my review of #1746. This is a follow-up, not a blocker here.

Registries add up. The ledger file has 6174 → 6175 rows, never stays 3886, and dated goes 2288 → 2289. The new row's name matches both harnesses. cluster_tests goes 2757 → 2758.

Logs: logs/1748/ (parent-*, head-*, mutant-commitlast-*, mutant-nobelt-*, mutant-commit-after-done-*, inject-*, suggested-check.log, ci-suites-pg17.log).

🤖 Generated with Claude Code

[jdatcmd]

@OffgridwithJD Approving #1748 at d1b98f38.

Both points from my last review are fixed. CI is green, because #1746 now restarts through pgc_pg. Arm 6 now reads to the end of the worker's PG_TRY block, so a commit moved after the DONE write fails it by name in both harnesses. I reviewed only this PR's two commits (c437fc5d, d1b98f38), against #1746's head e90e8ade. All runs are on PG17, against my own prefix copy, as root, one run at a time. The twin ran serially.

Runs

run	shell	twin
this head	11 passed + 0 failed	11 pass + 0 fail + 0 unrun = 11
commit-after-done (commit moved after the DONE write), this head's tests	10 passed + 1 failed	10 pass + 1 fail
commit-after-done, the old arm 6 (c437fc5d's tests)	11 passed + 0 failed	11 pass + 0 fail
commitlast (commit moved back between the detach and the DONE write)	10 passed + 1 failed	10 pass + 1 fail
#1746's code, this head's tests	10 passed + 1 failed	10 pass + 1 fail
nobelt (the collect loop's new free removed)	11 passed + 0 failed	11 pass + 0 fail

Blocking item 1 from my last review: fixed

CI at d1b98f38 has 15 check runs, all 15 success. As root, the shell passes 11/11 here, arm 4 included.

Blocking item 2: fixed, red first

This is the re-ordering I described: the three calls table_close, PopActiveSnapshot and CommitTransactionCommand moved after pg_atomic_write_u32(&me->state, PFLUSH_DONE). The src/ at c437fc5d is the same as at this head, so I applied the same patch to both trees. The old arm 6 passes it, 11/11 in both harnesses. The renamed arm 6 fails it, alone and by name:

FAIL  a flush worker runs nothing that can raise after pinning its output, to the end of its PG_TRY block: got [dsm_detach(outseg); pg_atomic_write_u32(&me->state, PFLUSH_DONE); table_close(rel, AccessShareLock); PopActiveSnapshot(); CommitTransactionCommand();] want [dsm_detach(outseg); pg_atomic_write_u32(&me->state, PFLUSH_DONE);]
TWIN FAIL: a flush worker runs nothing that can raise after pinning its output, to the end of its PG_TRY block

The row's date still holds. commitlast and #1746's code both redden the renamed check, alone, in both harnesses:

FAIL  a flush worker runs nothing that can raise after pinning its output, to the end of its PG_TRY block: got [dsm_detach(outseg); table_close(rel, AccessShareLock); PopActiveSnapshot(); CommitTransactionCommand(); pg_atomic_write_u32(&me->state, PFLUSH_DONE);] want [dsm_detach(outseg); pg_atomic_write_u32(&me->state, PFLUSH_DONE);]

Registries

• compare_to_bash.py: literal matches: 11 | template matches: 0 | missing: 0.
• The suite's 11 ledger rows match the shell's RESULT names and the twin's record names (diff is empty).
• Counted from the ledger: rows 7962 to 7963, never stays 4545, dated 3417 to 3418. 4545 + 3418 = 7963. cluster_tests goes 4226 to 4227.
• The pyarrow import scan prints [].

Not blocking

• nobelt still passes 11/11 in both harnesses, as at the old head. No test covers the collect loop's new free. Arm 6 is what keeps that free unneeded today.
• c437fc5d's message still says "green 10/10". d1b98f38's message gives the right count, 11.

Logs: logs/1748-rr2/ (head-clean-*, mutant-commit-after-done-*, mutant-commit-after-done-oldsuite-c437fc5d-*, mutant-commitlast-*, basecode1746-headtest-*, mutant-nobelt-*, with each mutant's .diff).

🤖 Generated with Claude Code