Make columnar_fetch_row cancellable (floor for #212)

[ChronicallyJD]

The floor you assigned me -- the cancellability half of #212 (see the scope note below; it does not close the whole issue). Scope is deliberately just the interrupt check; the per-fetch catalog scan that makes the loop expensive is left for the caching rework, per your call to wait on the #213 ablation.

The change

One CHECK_FOR_INTERRUPTS() at the top of columnar_fetch_row. Every fetch is now a cancellation point.

columnar_fetch_row is reached once per candidate item pointer by _bt_check_unique() during a unique INSERT, and each call reads the row-group list from the catalog. The three interrupt checks already in columnar_reader.c are all on the scan/decode path (ColumnarReadRowByNumber and friends), which a unique liveness check never enters -- columnar_index_fetch_tuple settles visibility without decoding. So the checks sat one call-frame from the loop that needed one, and a fetch-heavy unique INSERT ran with no interrupt check on its path. SIGTERM (pg_ctl stop) and query cancel both set a flag that only CHECK_FOR_INTERRUPTS acts on, so before this change they were raised and never processed -- a pg_ctl stop or a cancel is almost certainly what would have ended the Jul-25 backend, and now does.

Scope, corrected from an earlier draft of this body: this closes the cancellability half of #212, not the literal "outlived its postmaster" half. ProcessInterrupts never consults PostmasterIsAlive (postmaster death is noticed only in latch waits, WL_EXIT_ON_PM_DEATH), and a CPU-bound loop with only a CHECK_FOR_INTERRUPTS still never reaches one. So a postmaster that dies without signalling its children is a separate matter -- either its own fix or a documented consequence of a loop that never waits. #212 should stay open for that half.

Behavioural before/after

A fetch-heavy unique conflict -- a few hundred dead duplicate entries for one key, each a candidate the check fetches, each fetch scanning the whole row-group list -- under a 10 ms statement_timeout, on a PG18 assert build:

WITH guard   (this PR)   ERROR: canceling statement ... (57014), backend up
WITHOUT guard            completes, NOERR -- the statement ignored the 10 ms cap

Without the guard the same statement runs to completion ignoring the timeout: the loop reaches no interrupt check, so the cancel is never raised (which is also why a plain WHEN OTHERS never sees it). That is #212 in miniature. With the guard it cancels at the cap and the backend survives.

Why the regression test is structural

test/native_fetch_interrupt.sh asserts the guard is inside columnar_fetch_row (not merely somewhere in the file -- the file already had three checks that don't cover this path) and that it precedes the per-fetch catalog read it guards. Both fail if the guard is removed; that is the regression.

The behavioural check above is not in CI on purpose. Making the loop reliably overrun a small timeout needs the per-fetch catalog scan paid enough times, and that fixture is itself O(dups^2) to build -- each set-up delete pays the same scan the bug is about, so a loop long enough to be reliably uncancellable takes minutes to set up. Worse, the loop's exact length shifts with build flags and catalog state enough to cross a 10 ms line either way (it did, between -O2 and the -O1 matrix build). A flaky timing assertion in the matrix is worse than none -- it teaches readers to discount red. The guard's presence and placement do not move, so that is what the suite pins.

That O(dups^2) set-up cost is not incidental: it is the same O(candidates x row_groups) per-fetch scan that is the second defect, the one you are holding for the caching rework. This PR does not touch it.

Gate

Full bar on a clean box: preflight 15.18 / 16.14 / 17.6 / 18.4 / 19beta2 with zero warnings, matrix ALL VERSIONS PASSED on PG18 and PG19, native_fetch_interrupt=PASS on both majors, harness_selftest green (registered next to native_fetch_cache). Warnings-clean build. The first cut carried a behavioural timing check that flaked between -O2 and the -O1 matrix build -- that is what sent me to the structural form above.

The hang is now a cancellable statement. The per-fetch catalog scan is untouched and yours to sequence after the ablation.

🤖 Generated with Claude Code

[jdatcmd]

The change itself is right and I want it in. The diagnosis matches what I
verified independently: columnar_fetch_row runs ~310 lines with no interrupt
check, the three checks in columnar_reader.c are all on the scan/decode path,
and columnar_index_fetch_tuple settles visibility without decoding, so a unique
liveness check never reaches any of them. One check per fetch is the right floor
and the placement before ColumnarReadRowGroupList is correct.

Three things before it merges. One is a must-fix, one is an accuracy correction,
one is a note.

1. Must fix: the SUITES edit corrupted a comment

The registration looks like a global replace of native_fetch_cache, and it hit
more than the array:

-# rather than a matter of taste. native_fetch_cache asserts one on the same
+# rather than a matter of taste. native_fetch_cache native_fetch_interrupt asserts one on the same

That sentence is the one #209 added to explain why native_fetch_cache is
deliberately absent from is_timing_suite: it compares one big group against
ten small ones, so contention is common-mode and cancels in the ratio. As
rewritten it now reads as though native_fetch_interrupt also asserts a
wall-clock ratio on an index-driven fetch, which is the opposite of what this PR
argues: your suite is structural precisely so it has no stopwatch in it.

That comment is load-bearing. The next person deciding whether a new timing
suite belongs in is_timing_suite reads it. Restore line 141 to its original
text and add native_fetch_interrupt to the array only.

2. Accuracy: this does not fix "survived its postmaster"

The PR body says the path was "uncancellable, and blind to postmaster death.
That is why the Jul-25 backend was still burning a core three days later instead
of dying with its cluster." The first half is right and the second does not
follow, and I checked rather than assumed:

$ sed -n '/^ProcessInterrupts(void)/,/^}/p' src/backend/tcop/postgres.c | grep -c PostmasterIsAlive
0

ProcessInterrupts never consults PostmasterIsAlive. Postmaster death is
detected in latch waits (WL_EXIT_ON_PM_DEATH), which this loop never reaches
and still will not reach after this change. So:

• Fixed by this PR: SIGTERM (pg_ctl stop, -m fast or smart) and query
  cancel. Both set a flag that only CHECK_FOR_INTERRUPTS acts on, so before
  this change they were raised and never processed. That is a real fix and it is
  almost certainly what would have killed the Jul-25 backend.
• Not fixed: a postmaster that dies without signalling its children, which is
  the literal reading of #212's title. A CPU-bound loop with only a
  CHECK_FOR_INTERRUPTS still will not notice.

The code comment you wrote is already careful about this and I would not change
it. It is the PR body that overreaches. Worth getting right because it decides
whether #212 can be closed by this PR, and my read is that it cannot: this closes
the cancellability half, and the "outlived its postmaster" half is either a
separate fix or a documented consequence of the loop never waiting.

3. Note, not a change request: the guard is per fetch, not per catalog tuple

ColumnarReadRowGroupList and ColumnarReadDeleteVectorList both have zero
interrupt checks in their scan loops (confirmed), so the cancellation interval is
one whole fetch, which is O(row_groups). That is fine now: per-tuple catalog
scanning is fast and row-group counts are not yet large enough for a single fetch
to be a noticeable hang. It stops being fine if the caching rework does not land
and row groups grow. Not worth a check in the scan loops today; worth knowing
that the interval is bounded by the same quantity the second defect is about.

On the structural test

I checked the extraction works rather than trusting it: the awk range matches
columnar_fetch_row at column 0 on line 1654 and pulls 310 lines, so it is
selecting the real function body and not silently matching nothing. Both checks
would pass vacuously if it did, so that mattered.

I accept the argument for structural over behavioural here. A timing assertion
that crosses a 10 ms line between -O2 and -O1 is exactly the flake that
teaches readers to discount red, and we have spent enough of this week removing
those. The behavioural before/after belongs in the PR, which is where you put it.

Fix 1, and correct 2, and I will merge it.

[ChronicallyJD]

Both taken, and you were right to check the second one rather than accept my framing.

1. Comment fixed. That was a global sed on native_fetch_cache that hit line 141 as collateral — exactly the "write the script, don't inline the substitution" mistake. Line 141 is back to the original #209 wording, and native_fetch_interrupt is in the SUITES array only. harness_selftest passes (registration + file checks), and the suite still passes. The C change in columnar_reader.c is byte-identical to the green gate, so this is a test-infra-only delta on top of it.

2. Body corrected, and #212 stays open. You're right and I overreached — I asserted the postmaster-death half from the symptom without checking ProcessInterrupts, which is the same shortcut I've been calling out all week. PostmasterIsAlive is only consulted in latch waits, which this loop never reaches, so this fixes SIGTERM and cancel (the flag-then-CHECK_FOR_INTERRUPTS path) and not a postmaster that dies without signalling. The body now says exactly that and scopes the PR to the cancellability half; the "outlived its postmaster" half is a separate fix or a documented consequence. Please keep #212 open for it.

3. Noted on the per-fetch interval. Agreed — the cancellation granularity is one whole fetch, O(row_groups), because ColumnarReadRowGroupList / ColumnarReadDeleteVectorList have no in-scan checks. Fine now, bounded by the same quantity the caching rework is about, and I flagged that on #212 (the caching analysis, with the snapshot landmine). If the caching does not land and row-group counts grow, a check in those scan loops becomes the cheap follow-up.

[jdatcmd]

Verified rather than taken on trust, since that is the standard you held me to on
the postmaster claim.

• Line 141 is back to the #209 wording exactly, and native_fetch_interrupt
  appears once, in the array only.
• The C change is the 13-line comment and one CHECK_FOR_INTERRUPTS(),
  unchanged from what you gated.
• The suite proves itself by removal: green as-is on PG17, and with the guard
  deleted both checks go red. The sed took exactly one of the four
  CHECK_FOR_INTERRUPTS lines in the file (4 to 3), so it removed the new guard
  and not the three pre-existing ones. That is the removal proof this needed,
  because a structural check that greps a file is otherwise easy to satisfy
  vacuously.

The corrected scope in the body is right, and thank you for stating it that
plainly. #212 stays open for the postmaster-death half.

Merging.