Assert the single-backend crash blast radius (#217) - #223
Conversation
21a8d75 to
bc17aa0
Compare
|
Rebased onto |
jdatcmd
left a comment
There was a problem hiding this comment.
The audit is right, the guarantee is the correct one to state, and the suite runs
19/19 green here on PG17.10. One change first, and it is the same class of defect
we have both been finding all day.
18 of the 19 checks pass whether or not a backend crashed
I removed only the kill, left everything else, and ran it:
- kill -SEGV "$pid" 2>/dev/null || true
+ : # kill removed for the vacuity proof
FAIL a concurrent reader was dropped by the crash: got [no] want [yes]
checks run: 19
One check failed. The other eighteen passed against a run in which nothing
was ever killed.
That is not a criticism of the assertions; they are the right assertions. It is
that they are all conditional on a crash, and only the third scenario contains
anything that proves a crash happened. The mid-write and post-flush scenarios
have no such check at all:
postmaster survived, same pidpasses trivially when nothing died.committed content matches the heap mirrorpasses because it always did.the killed insert left no visible rowspasses because the insert was in an
open transaction and the backgrounded client was killed anyway, so it rolled
back without any crash.
So if the kill ever stops landing -- the pg_stat_activity marker changes, the
query text is rewritten, the pid lookup returns empty and kill -SEGV "" is a
silent no-op, which is exactly what my edit simulated -- two of the three
scenarios become no-ops and stay green forever. The suite would then be
asserting crash safety without ever crashing anything.
What I would like
Each scenario asserts that the crash actually occurred, not just that the
aftermath looks right. Cheapest form is the server log, which already says it:
LOG: server process (PID nnn) was terminated by signal 11: Segmentation fault
LOG: terminating any other active server processes
Read it incrementally from a byte offset so scenario two is not satisfied by
scenario one's crash -- that detail matters, and I got it wrong in my own fuzzer
today in the opposite direction: I re-read the log from byte zero every time and
turned one line into 257 findings. Either that, or assert the target pid is gone
and the postmaster's reinit happened, per scenario.
Then re-run the same experiment: with the kill removed, every scenario should
go red, not one.
Verified, and good
- The audit result holds.
grep -nE 'smgrwrite|FileWrite|pg_pwrite|\bwrite\('
overcolumnar_storage.c,columnar_write_state.candcolumnar_metadata.c
returns nothing, andcolumnar_storage.chas 10log_newpage*/
XLOG_SMGR_TRUNCATEsites. Nothing bypasses the buffer manager, so the
guarantee really does reduce to core's, which is the strongest form this could
take. - Holding the transaction open so the kill lands before commit is the right
design, and your note that the first cut was crashing after commit and calling
the survivors a bug is worth keeping in the file. - The
wait_upfix is right and the reasoning is right: four probes at 0.1s can
all land inside one reinit blip. A continuous window is the correct shape. - Putting the statement in
ARCHITECTURE.mdrather thanlimitations.mdis the
right call. It is a mechanism, not a limitation, and #222 has since merged.
Fix the vacuity and I will merge it. The rest of this is ready.
Alpha gate 4 of 4. recovery.sh covers whole-cluster crash and restart. This adds the other shape: one backend dies with SIGSEGV while the postmaster lives, which is the jdatcmd#210 case. The postmaster reinitializes shared memory and drops every other session, and the guarantee is that committed columnar data is intact on the far side while an in-flight write leaves nothing. test/native_backend_crash.sh holds an INSERT uncommitted in an open transaction (so the kill lands before commit regardless of timing), locates the backend by a marker in its query text, SIGSEGVs it, and after the postmaster reinitializes asserts against a heap mirror: the postmaster survived with the same pid, the cluster accepts new connections, committed content matches the mirror exactly, the killed insert left no visible rows and no partial row group, and a concurrent reader was dropped. The kill is repeated mid-write and after the stripes have flushed but before the commit. The audit the issue asked for found no exception: every columnar write goes through the buffer manager and WAL (log_newpage_buffer for data pages, log_newpage before the initial extends, XLOG_SMGR_TRUNCATE for truncation), so the guarantee follows from core. Written into docs/ARCHITECTURE.md as a Crash safety section, citing that mechanism and both crash shapes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bc17aa0 to
75f4132
Compare
|
Strengthened the suite after a self-review turned up two ways it could pass without proving anything, both my own "assert the premise" gap:
Proved the sentinel check discriminates by mutating 19 checks -> 25. Re-gated on |
jdatcmd
left a comment
There was a problem hiding this comment.
Re-ran the same experiment rather than reading the summary. Removed only the
kill -SEGV, changed nothing else:
FAIL mid-write: the crash reinitialized the cluster (a live session was dropped)
FAIL post-flush: the crash reinitialized the cluster (a live session was dropped)
FAIL a second concurrent reader was also dropped by the crash
FAIL with-reader: the crash reinitialized the cluster (a live session was dropped)
checks run: 25
One failing check became four, one in every scenario. That is the fix, and it is
the right shape: the sentinel distinguishes a crash-triggered reinit from a
backend that merely exited, which is the only thing that separates this suite
from one that asserts nothing.
The SIGSEGV to SIGTERM mutation is a better proof than the one I asked for.
Removing the kill tests "no signal"; swapping to a graceful signal tests "the
wrong kind of death", which is the failure the sentinel is actually there to
catch, and it found a real bug in your own check while you were at it.
Asserting the phase from wait_event is worth more than the vacuity fix in the
long run. The scenario names claimed mid-write and post-flush and nothing
verified either, so a timing shift would have silently collapsed two scenarios
into one while both stayed green. Polling until PgSleep instead of guessing a
delay also removes the last timing assumption, which matters on this box.
25 checks, green as-is on PG17.10 here.
Merging. #217 can close with this.
Alpha gate 4 of 4. Asserts and documents the blast radius of a single backend crash, the one you said should be a guarantee rather than an inference from the #210 repros.
The audit came first, and found no exception
The guarantee rests on there being no columnar write path that bypasses the buffer manager or WAL. I checked every write in
columnar_storage.c,columnar_write_state.c, andcolumnar_metadata.c:log_newpage_buffer,log_newpaged before thesmgrextend,XLOG_SMGR_TRUNCATErecord.Nothing writes the file directly and nothing leaves shared state a dead backend does not clean up. So a single backend death is no different from any other backend death in PostgreSQL, and the guarantee follows from core. That is the audit result the issue asked for: an exception would have been a defect, and there is none.
The suite
test/native_backend_crash.sh, registered next torecovery.shin the matrix. It holds anINSERTuncommitted in an open transaction (BEGIN; INSERT ...; pg_sleep), so the kill lands before commit no matter how fast the write runs, locates the inserting backend by a marker in its query text, andSIGSEGVs that one backend while the postmaster lives. After the postmaster reinitializes it asserts against a heap mirror written the same way:pg_sleepwas dropped, not only the backend that died.The kill is repeated at two points in the write path: mid-write, and after the stripes have flushed but before the commit that never comes. 19 checks, about 8 seconds.
The first cut of this suite was wrong in my favour and the mirror caught it: a plain
INSERTcommitted before the kill, so I was crashing after commit and calling the survivors a bug. Holding the transaction open fixed it and made the kill timing-independent, which is why correctness here does not depend on how fast the box is.The guarantee, written down
docs/ARCHITECTURE.mdgains a Crash safety section stating both crash shapes (whole cluster, and one backend while the postmaster lives), what survives and what does not, and that it rests on columnar data and its catalog being ordinary WAL-logged storage. It citesrecovery.shandnative_backend_crash.sh. I put it there rather than inlimitations.mdbecause it is a mechanism statement, and because #222 is currently editinglimitations.md; no overlap.Gate
Preflight clean on all five majors (15/16/17/18/19), 0 warnings each. Full matrix green on PG18 and PG19,
native_backend_crash=PASSon both, 19 checks per major.One thing worth calling out about the suite itself, because it bit the first gate: the crash mechanism was never in doubt, but
wait_updeclared the cluster stable on a false positive — four quickSELECT 1s at 0.1s could all land inside one transient "accepting" blip during reinit, then the assertions fired while the postmaster was still tearing down the extra reader backend and briefly refused. It only surfaced in the with-reader scenario, and only on the two liveness-dependent checks; every durability check passed throughout.wait_upnow requires a continuous 3s window (12 consecutive at 0.25s) that a reinit blip cannot fake. Verified 19/19 twice under 5x CPU contention before the gate, then green in the real 6-way matrix.A reader can now determine, from
docs/ARCHITECTURE.mdalone, what a backend crash costs and what it does not, and a suite fails if that stops being true.🤖 Generated with Claude Code