Debug cross-check: every buffer dirtied in a WAL-emitting critical section must be registered in the WAL record#80
Draft
NikolayS wants to merge 2 commits into
Draft
Conversation
The WAL protocol requires every shared buffer modified by a WAL-logged operation to be registered with the WAL record describing the operation (XLogRegisterBuffer/XLogRegisterBlock). Replay can survive violations of this rule when the redo routine finds the page by other means, but the WAL summarizer only sees registered block references, so an unregistered modified page is silently omitted from incremental backups; it also never receives full-page images, leaving it exposed to torn writes. Commit ed62d26 fixed a long-standing violation of exactly this kind (heap records clearing visibility map bits without registering the VM page), which corrupted incremental backups. Nothing enforces the invariant mechanically. In assertion-enabled builds, cross-check it: MarkBufferDirty() records every permanent shared buffer dirtied inside a critical section (XLogRegCheckBufferDirtied in xloginsert.c); each XLogInsert() in that critical section removes tracked buffers that were registered with the inserted record; when the outermost critical section ends, any still-pending buffer is reported (WARNING by default so that a whole test run can collect all violations; define XLOG_REG_CHECK_PANIC to get a PANIC with a backtrace at the site instead). The check is deferred to critical section end because some paths emit several records per critical section and register a buffer only with a later one (e.g. XLOG_HEAP2_NEW_CID precedes the record that registers the heap page). Critical sections that insert no WAL records are not checked, which exempts temp/unlogged relations, wal_level=minimal skip-WAL relfilenodes, and index builds that WAL-log their result afterwards via log_newpage_range. FSM pages are exempted automatically (the FSM is deliberately not WAL-logged), and XLOG_ASSIGN_LSN records are whitelisted: XLogGetFakeLSN() emits them from inside index-AM critical sections operating on skip-WAL relfilenodes purely to advance the insert LSN, and wal_level=minimal cannot produce incremental backups anyway. XLogRegCheckExemptBuffer() provides a documented opt-out for any further legitimate cases; the core regression, isolation, recovery, subscription, and contrib suites currently pass without needing any buffer-level exemption. Verified by temporarily removing the VM-buffer registration that ed62d26 added to heap_insert: the check then reports the unregistered VM page on the first insert that clears an all-visible bit. Closes #73 Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01PtxA6B4d47ietk5dGsseuP
11 tasks
Replace the ad-hoc exemption list of the WAL registration cross-check with an explicit classification: every dirtied buffer and every inserted record now passes through XLogRegCheckClassifyBuffer() / XLogRegCheckClassifyRecord(), which return a XLogRegCheckClass value. Each non-CHECKED class documents the invariant that makes skipping the check safe and the conditions under which that argument would break, so future exemptions require writing down a safety argument rather than appending to a list: - NOT_WAL_LOGGED_RELATION: local (temporary) and non-permanent (unlogged) buffers; no WAL is generated for their data changes and they are outside incremental backup's guarantees. - SELF_REPAIRING_FORK: FSM pages; deliberately not WAL-logged, tolerated stale/torn, copied whole by incremental backup. - HINT_ONLY: documented for completeness; MarkBufferDirtyHint() does not feed the tracker, and checksummed hint changes are WAL-logged with a registered block via XLOG_FPI_FOR_HINT. - LSN_ONLY_RECORD: records whose entire meaning is the LSN they occupy; the only member is XLOG_ASSIGN_LSN (XLogGetFakeLSN on skip-WAL relfilenodes). Superficially similar records (XLOG_NOOP, XLOG_SWITCH) are deliberately excluded so that the check complains if they ever appear in a page-modifying critical section. Remove the per-buffer opt-out API (XLogRegCheckExemptBuffer): no caller needed it, and an escape hatch that bypasses classification could fossilize bugs. Behavior is otherwise unchanged; regress, isolation, recovery/018_wal_optimize, subscription/026_stats and recovery/027_stream_regress run clean, and the ed62d26-revert kill test still trips the check on the unregistered VM page. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01PtxA6B4d47ietk5dGsseuP
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WAL registration cross-check (assert builds)
The invariant "every page a redo routine touches is registered in the WAL record" was silently violated for years by heap VM-bit clears, fixed only recently in ed62d26: unregistered blocks are invisible to the WAL summarizer, so incremental backups silently omit them, and they never get FPIs, so they are exposed to torn writes. Nothing enforced this invariant mechanically. This PR adds a debug-build cross-check that does — with every exemption expressed as an explicit, justified classification, not a permissive list.
Design
All under
#ifdef USE_ASSERT_CHECKING:XLogRegCheckBufferDirtied, called fromMarkBufferDirtyin bufmgr.c): every buffer dirtied whileCritSectionCount > 0whose classification is CHECKED (see below) is recorded by buffer tag in a fixed 256-entry array (no allocation is possible inside a critical section; on overflow the check is skipped for that critical section).XLogRegCheckRecordInserted, called fromXLogInsert): each CHECKED-class WAL record inserted inside the critical section removes tracked buffers whose tag matches a block registered viaXLogRegisterBuffer/XLogRegisterBlockfor that record.XLogRegCheckCritSectionEnd, called fromEND_CRIT_SECTIONwhen the outermost critical section ends): if at least one CHECKED-class WAL record was inserted in the critical section, every tracked buffer still pending — dirtied beforeXLogInsertin the same critical section but never registered with any record — is reported. State also resets when a top-level critical section starts (START_CRIT_SECTION), so error-pathCritSectionCount = 0resets (elog.c, ipc.c) cannot leak stale entries.The check is deferred to critical-section end rather than raised at each
XLogInsertbecause legitimate paths emit several records per critical section and register a buffer only with a later one — e.g.XLOG_HEAP2_NEW_CID(no block references) precedes the heap record that registers the heap page. Critical sections that insert no checked record are not checked at all: there was no record the changes should have been registered with (temp/unlogged relations, wal_level=minimal skip-WAL relfilenodes, index builds that WAL-log their result afterwards vialog_newpage_range).Violations are reported at
WARNING(greppable prefixWAL registration check:) so a whole test run can collect all of them in one pass; compile with-DXLOG_REG_CHECK_PANICto turn them into PANICs and get a backtrace at the site. Bookkeeping edges handled: records with no buffers, multipleXLogInsertcalls per critical section, aborted insertions (XLogResetInsertionleaves tracking intact), buffers dirtied beforeXLogBeginInsertbut inside the critical section, nested critical sections (checked at the outermost end), bootstrap mode (phony inserts not counted).Explicit classification (no exemption list, no per-call opt-out)
Every dirtied buffer and every inserted record passes through
XLogRegCheckClassifyBuffer()/XLogRegCheckClassifyRecord(), returning anXLogRegCheckClassenum. Each non-CHECKED class documents in the source (a) the invariant that makes skipping the check safe and (b) what would make it unsafe, so future readers can re-audit. Adding an exemption requires adding a new class with its written safety argument. The earlier per-buffer opt-out API was removed: no caller needed it, and an escape hatch that bypasses classification could fossilize bugs.CHECKEDNOT_WAL_LOGGED_RELATIONBufferIsPermanentis the authority)SELF_REPAIRING_FORKFSM_FORKNUM)storage/freespace/README), best-effort, tolerated stale/torn, repaired in normal operation; having no WAL block refs anywhere, the fork is copied whole by incremental backupHINT_ONLYMarkBufferDirtyHint(documentation-only: hint dirtying never reaches the tracker)MarkBufferDirtyHintitself WAL-logs an FPI (XLOG_FPI_FOR_HINT) with the block registered, visible to the summarizerMarkBufferDirtyHint/XLogSaveBufferForHint, or a caller used it for a change replay depends onLSN_ONLY_RECORDXLOG_ASSIGN_LSNonlyXLOG_ASSIGN_LSN(emitted byXLogGetFakeLSN()inside hash/nbtree/gist critical sections): only reachable on skip-WAL relfilenodes (XLogGetFakeLSNasserts!RelationNeedsWAL), the whole relation is durably synced at commit, and wal_level=minimal is incompatible with WAL summarization (summarize_walrequireswal_level >= replica), so incremental backups cannot be affected. Superficially similar records (XLOG_NOOP,XLOG_SWITCH) are deliberately NOT members — if they ever appear in a page-modifying critical section the check should complainXLOG_ASSIGN_LSNcaller must re-justify membershipFindings — full list
recovery/018_wal_optimize(wal_level=minimal node): ~13k warnings over main-fork blocks of test relationsrmid 0 info 0xC0=XLOG_ASSIGN_LSNLSN_ONLY_RECORDclass (safety argument above); re-run cleansubscription/026_stats(subscriber node;Cluster.pminitializes non-streaming nodes with wal_level=minimal): 2 warningsCandidate incremental-backup corruption bugs found: none. After ed62d26 (in master since 2026-07-15), no unregistered-but-dirtied page in a WAL-emitting path was detected in any suite. One near-miss documented for future reviewers:
visibilitymap_prepare_truncate/FreeSpaceMapPrepareTruncateReldirty the last VM/FSM page outside the critical section that later logsXLOG_SMGR_TRUNCATE(no block reference) — outside this checker's scope by design; the WAL summarizer special-cases truncation records, but that path deserves its own audit if summarizer behavior changes.Kill test (ed62d26 revert) — committed procedure and output
Procedure (run against the final code of this PR, classification included):
heap_insert(src/backend/access/heap/heapam.c), disable the VM registration ed62d26 added: changeif (vmbuffer_modified)beforeXLogRegisterBuffer(HEAP_INSERT_BLKREF_VM, vmbuffer, 0);toif (vmbuffer_modified && false).ninja -C build, restart the assert-enabled server.CREATE TABLE vmtest(a int); INSERT INTO vmtest VALUES (1); VACUUM vmtest;(sets the all-visible bit)INSERT INTO vmtest VALUES (2);(clears it — the pre-ed62d26 unregistered VM-clear).Observed output (server log, on the first VM-clearing insert):
fork 2 = visibility map, rmid 10 = Heap, info 0x00 =
XLOG_HEAP_INSERT— precisely the ed62d26 bug class. 4. Revert the temporary change, rebuild;git statusconfirms the working tree contains only this PR's files. Executed twice: on the initial implementation and re-executed on the final classification-based code.Provenance
d374280("ci: Generate crashlogs on Windows"), v19devel; branchclaude/testplan-wal-registration-assert, commitsa59af1d(checker) +44c6fc7(classification rework).meson setup build --buildtype=debug -Dcassert=true -Dtap_tests=enabled; gcc, Linux x86-64;ninja -C build -j2.Cluster.pm:wal_level=minimalfor plain nodes,replica/logicalwithallows_streaming); manual stress instance:wal_level=logical,summarize_wal=on,autovacuum_naptime=5.regress/regress— OK, 245 subtests, 51.4 s (re-run after rework)isolation/isolation— OK, 130 subtests (re-run after rework)recovery/018_wal_optimize— OK, 38 subtests, 19.1 s (re-run after rework; pre-rework the only warning site, see Findings)subscription/026_stats— OK, 13 subtests, 8.7 s (re-run after rework)recovery/027_stream_regress(full core regression under streaming replication) — OK, 11 subtests, 83.2 s (re-run after rework)recovery+subscriptionsuites — 85 pass / 7 env-dependent skips; 27 contrib/module suites (70 tests) incl.test_decoding,pg_walsummary,pg_combinebackup,pg_verifybackup,pg_basebackup,pg_visibility,pg_surgery,amcheck, hash/gin/gist/spgist/brin-heavy suites — all pass;pg_upgrade/002(full core regression under wal_level=minimal) — pass, 64.6 s.pgbench -c4 -j2 -T60withsummarize_wal=on(~50k txns); mixed hash/gin/gist/spgist/brin/tsvector workload with vacuum/freeze cycles; logical-decoding NEW_CID exercise (user_catalog_table+ DDL +pg_logical_slot_get_changes) — all clean.build/meson-logs/testlog.txt(per-invocation summary), per-test cluster logs underbuild/testrun/<suite>/<test>/log/*.log(e.g.build/testrun/regress/regress/log/postmaster.log); violations greppable viaWAL registration check:. Checker warnings across all final runs: 0.Caveats (RFC-quality, not final API)
XLogRegisterBufferindependently asserts the buffer is dirty.Assert/PANICafter buildfarm vetting of the classification.START_CRIT_SECTIONbecomes a statement macro in assert builds; no in-tree code uses it as an expression (verified by grep; tree compiles).Closes #73
🤖 Generated with Claude Code
https://claude.ai/code/session_01PtxA6B4d47ietk5dGsseuP