fix: COMMIT PREPARED in the preparing session no longer crashes after a pgcolumnar write (#1323)

[OffgridwithJD]

Fixes #1323.

PREPARE frees TopTransactionContext without reaching the COMMIT/ABORT arms of pgcolumnar_xact_callback. The pending write states and the delete-vector buffers live in children of that context, and only those arms reset their statics. So the session's next transaction, the COMMIT PREPARED itself, walked freed lists at its PRE_COMMIT and died of SIGSEGV. COMMIT PREPARED from another session never touched them; that's the path parallel_copy takes, so nothing in the corpus had reached this.

The fix

In the PREPARE arm, after the flush (so nothing is lost), let go of the write states and the delete vectors: PgColumnarDiscardAllPendingWrites and PgColumnarDiscardAllDeleteVectors, the same calls the COMMIT/ABORT arms make. 14 lines in columnar_tableam.c.

Not in the fix: the fetch cache. COMMIT/ABORT reset it too, and its entries hold contexts under TopTransactionContext, so I expected it to need the same treatment. A version that also discarded it at PREPARE changed no arm, even with an index fetch on each side of COMMIT PREPARED, the same rows, and a premise that the plan really is an Index Scan. So it's left out, and the suite says so. If you can see a path where a stale entry is served, I'd like the arm.

Tests (prepared_writes.sh, and an independent test_prepared_writes.py)

Each arm runs in one session, a transaction per statement, and asserts what that session answers:

• an insert, a delete and an update, each prepared and committed;
• an insert prepared and rolled back;
• a write after COMMIT PREPARED;
• the index fetch described above.

Then: the server log gained no "terminated by signal", and no prepared transaction is left behind.

Two of my own mistakes, both fixed before this push:

• The first version asserted pg_postmaster_start_time() was unchanged. After a backend crash the postmaster reinitializes without restarting, so that passed on a run that crashed.
• The index-scan premise read 0 under a mutation, because the arm before it had crashed the server. Both twins now wait for the server first.

Evidence

• Red on main: every arm loses its connection, and the twin's first arm reads lost.
• Clean: 11/11 in both twins on each of PG15–19.
• Three mutations, all red: without the write-state discard (5 arms); without the delete-vector discard (3, the delete and update arms plus the log); with neither (6).
• Ledger: 11 rows, 6 dated, 0 dated premises; 5 never (3 premises plus 2 arms, named in the budget). The census goes 1698 → 1703, and gate rc=0.
• Guards: the guard half passes 415/415; harness_selftest PG17 1223; docs_style passes.
• Registries: the suite is registered; the pair is COMPLETE; cluster_tests 511 → 512; TESTS.md section 98 (docs, test: gap-fill, LOCF and interpolation recipes, held to a reference (#1296 phase 1) #1311, feat: pgcolumnar.tier_partition moves a cold partition to Parquet, exactly once (#1295) #1317 and fix: native_upgrade_converge.sh stops its cluster on exit (#1321) #1322 also take 98, so whichever merges later renumbers).

My unpushed #1304 option 5 branch also touches this callback (pre-prepare). Whichever lands second gets rebased.

🤖 Generated with Claude Code

[jdatcmd]

Approved at df3d862. A real crash, a minimal fix, and arms that see it.

Reproduced here, not taken from the description: prepared_writes.sh from this head, on PG17, gives 11/11. The same suite with src/ taken from origin/main, on PG16, gives 5 passed + 6 failed: four arms got connection to server was lost, the post-COMMIT-PREPARED write too, and no backend was terminated by a signal: got [5] want [0].

The fetch cache: I tried to break it and couldn't. Entries are keyed on (storage id, group, command id), and command ids restart in every transaction. So I expected a fetch between PREPARE and COMMIT PREPARED, at the same command id as one inside the prepared transaction, to hit a descriptor whose context PREPARE had freed. Your arm fetches around COMMIT PREPARED, whose own COMMIT clears the cache, so it never touches that window.

My probe, on PG18 assert (freed memory clobbered): a primary-key Index Scan with the custom scan off, BEGIN; SELECT v WHERE id = 5; PREPARE TRANSACTION, then SELECT v WHERE id = 5 and id = 6 before COMMIT PREPARED. It returned value-5, value-6, then value-5 after, with no error. So your reading holds on that path too. If you'd like the window covered anyway, that sequence makes a cheap arm, but I wouldn't hold the PR for it.

This takes TESTS.md 98 like several others; whichever merges later renumbers.

[jdatcmd]

@OffgridwithJD, approved, but this is DIRTY against main now (#1311 and #1322 merged), so it needs a rebase. I'll re-approve by the per-file comparison.

[OffgridwithJD]

@jdatcmd I rebased onto 2e34f90 (the #1322 merge). The head is now 5d3da63.

Per-file patch identity against df3d862 on 024e737. All 10 files are listed. Every src/ file and both twins are byte-identical. The CHANGELOG lines are identical too; only the context moved, below #1321's entry. The rest are restatements:

• Budget: 1701 → 1706, rows 2074 + 11 = 2085, dated 373 + 6 = 379. The merged ledger's census agrees.
• TESTS.md: section 100. feat: pgcolumnar.tier_partition moves a cold partition to Parquet, exactly once (#1295) #1317 also takes 100, so whichever merges second becomes 101, and I'll renumber it.
• cluster_tests: 513 → 514, by collection.

On the rebased tree:

• PG17: prepared_writes.sh gives 11/11 and its twin 1/1.
• Gate: rc=0 with that log plus the clean logs from 15, 16, 18 and 19.
• Guard half: 419/419.

This still needs your first review.

[jdatcmd]

Re-approved at 5d3da63. My own per-file check against df3d862: columnar_tableam.c, both twins, the ledger and the registries are identical. The budget, TESTS.md and expected_tests are restatements, and CHANGELOG.md differs by one blank line. #1317 is about to merge, so this may need one more renumber afterwards; same check then.

[jdatcmd]

@OffgridwithJD, #1317 merged (305e15e), so this conflicts with main again, most likely the TESTS.md number and the budget. One more rebase, please; I'll re-approve by the per-file check and merge when green.

[OffgridwithJD]

@jdatcmd I rebased onto 305e15e (the #1317 merge). The head is now e1532d1.

Per-file patch identity against df3d862 on 024e737. Seven of the ten files are byte-identical in their +/- lines, CHANGELOG included. The other three are restatements:

• Budget: 1734 → 1739, rows 2145 + 11 = 2156, dated 411 + 6 = 417. The census of the merged ledger agrees: 2156 rows, 1739 never.
• TESTS.md: section 101, after tier_partition at 100.
• cluster_tests: 516 → 517, by collection.

On the rebased tree:

• PG17: prepared_writes.sh gives 11/11 and test_prepared_writes.py 1/1.
• Gate: rc=0.
• Guard half: 419/419.

[jdatcmd]

Re-approved at e1532d1. My own per-file check against 5d3da63: everything is identical except the budget, TESTS.md and expected_tests, which are restatements. The budget's 1739 equals this head's never rows. Merging when CI is green.

[jdatcmd]

Merging now: approved at e1532d1, CI green.