Hi! On Wed, Sep 2, 2026 at 2:39 AM Melanie Plageman <[email protected]> wrote: > On Tue, Sep 1, 2026 at 5:18 PM Noah Misch <[email protected]> wrote: > > On Tue, Apr 21, 2026 at 04:49:12PM +0300, Alexander Korotkov wrote: > > > I also think that usage of MarkBufferDirty() here is safe. If I > > > understood correctly. > > > 1) When wal_log_hints = on, should be completely safe. Even if we > > > have torn page after the crash, during recovery FPI from the primary > > > should come first. > > > > I think this change (commit c06d1a4) is incorrect. Assume checksums and > > full-page writes are enabled, both defaults. Before this change, redo would > > transition FSM pages clean->dirty only via XLogReadBufferForRedo() of an > > FPI. > > At end of recovery, the FSM passed checksum validation. After $SUBJECT, > > nothing stops the following sequence of events: finish restartpoint; dirty > > FSM > > page via heap_xlog_*; crash tears write of that FSM page; resume recovery; > > end > > recovery without curing the torn page. The FSM code's own use of > > RBM_ZERO_ON_ERROR makes FSM code accept torn pages. Other callers assume > > normal reads will succeed on FSM_FORKNUM, e.g., the > > heapam_relation_copy_data() call to RelationCopyStorage(). Claude wrote a > > test of that, attached. > > Ah, I guess this all hinged on having correctly surveyed all readers > of the FSM and ensuring they also use RBM_ZERO_ON_ERROR, which I did > not do and it turns out they don't. Oops. > > > Long-term, we do have at least these > > alternatives: > > > > - Keep the pre-2026-05 invariant that FSM is free from torn pages at end of > > recovery. (We'll likely still have the property that heap_xlog_* will > > read > > torn FSM pages during recovery. That arises, I think, because the FPI > > that > > fixes the torn page may be later in the WAL stream. FSM is unlike other > > forks this way; other forks write before they read.) > > > > - Require all readers of FSM_FORKNUM to use RBM_ZERO_ON_ERROR or the > > equivalent. > > I need to think more about it, but my initial thought was maybe > RelationCopyStorage() shouldn't be reading all forks the same with a > simple smgrread(). Though I don't know how reasonable it is to try to > prevent anyone in the future from ever reading the FSM fork without > RBM_ZERO_ON_ERROR. And I also didn't yet check if other callers are > reading the FSM without RBM_ZERO_ON_ERROR already. Hmm...I'll need to > think and dig a bit more.
I've been thinking about it for a while. While FSM code can tolerate torn pages, other code can't (and that's not only backend code). I think we have nothing to do with this (at least with back branches). 0001 is revert 0002 is patch for FSM readme to highlight this aspect I'm going to push (and backpatch) 0001. 0002 needs review. ------ Regards, Alexander Korotkov Supabase
From 2fba69295b831fa76f51d1380522b603c8964d2d Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Mon, 7 Sep 2026 14:50:56 +0300 Subject: [PATCH v1 2/2] Say in the FSM README why its hint writes still need full page images The Recovery section said we would "operate correctly without the full page images that MarkBufferDirtyHint() provides", offering them only as a way to lose less slot knowledge to RBM_ZERO_ON_ERROR. That is true of freespace.c, which passes RBM_ZERO_ON_ERROR on every read, and false of the system around it, which reads the same files and does verify checksums. Reading it as a licence to dirty FSM pages without a full page image is what led to c06d1a4ba6b, since reverted. Say instead that RBM_ZERO_ON_ERROR is a property of this directory rather than of the fork, name the readers that do not share it -- RelationCopyStorage(), RelationCopyStorageUsingBuffer(), base backups and pg_checksums -- and note that the last two cannot be relaxed, because a verification tool told to accept a tear we consider harmless can no longer report one caused by failing storage. Discussion: https://postgr.es/m/20260901211837.f6.noahmisch%40microsoft.com --- src/backend/storage/freespace/README | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/src/backend/storage/freespace/README b/src/backend/storage/freespace/README index dc2a63a137b..f272d17b050 100644 --- a/src/backend/storage/freespace/README +++ b/src/backend/storage/freespace/README @@ -189,9 +189,26 @@ to propagate the new free-space info into the upper pages of the FSM tree. As a result when we write to the FSM we treat that as a hint and thus use MarkBufferDirtyHint() rather than MarkBufferDirty(). Every read here uses RBM_ZERO_ON_ERROR to bypass checksum mismatches and other verification -failures. We'd operate correctly without the full page images that -MarkBufferDirtyHint() provides, but they do decrease the chance of losing slot -knowledge to RBM_ZERO_ON_ERROR. +failures. + +That RBM_ZERO_ON_ERROR is a property of this directory, not of the FSM fork +itself. The files themselves are ordinary relation files, and plenty of code +outside freespace.c reads them and does verify checksums: +RelationCopyStorage(), which ALTER TABLE ... SET TABLESPACE runs over every +fork; the read stream in RelationCopyStorageUsingBuffer(), used by +CREATE DATABASE ... STRATEGY = wal_log; and the checksum verification in base +backups and in pg_checksums. The last two are the reason this cannot simply be +relaxed: an external verification tool has no way to tell a tear that we +consider harmless from one caused by failing storage, so making it accept +the former blinds it to the latter. + +So the full page images MarkBufferDirtyHint() writes are not just an +optimization that reduces how often RBM_ZERO_ON_ERROR loses knowledge. +When checksums or wal_log_hints are enabled they are what keeps an FSM page +that reaches disk either checksum-valid or repairable by a later FPI. Do not +replace MarkBufferDirtyHint() with MarkBufferDirty() here on the grounds that +the FSM survives torn pages; it does, but the rest of the system is not +promised the same. Relation extension is not WAL-logged. Hence, after WAL replay, an on-disk FSM slot may indicate free space in PageIsNew() blocks that never reached disk. -- 2.55.0
From 3a5500a2545c87d9b36145d305c63b8ccb2e25cc Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Mon, 7 Sep 2026 14:50:08 +0300 Subject: [PATCH v1 1/2] Revert "Mark modified the FSM buffer as dirty during recovery" This reverts commit c06d1a4ba6b26eef27b04074683cccade6c277ee. The commit assumed that if the FSM code tolerates torn pages, everything else do so. Readers that do not tolerate that include RelationCopyStorage(), used by ALTER TABLE ... SET TABLESPACE for every fork, the read stream in RelationCopyStorageUsingBuffer() used by CREATE DATABASE ... STRATEGY = wal_log, and, most awkwardly, the checksum verification in base backups and pg_checksums. Reported-by: Noah Misch <[email protected]> Discussion: https://postgr.es/m/20260901211837.f6.noahmisch%40microsoft.com Backpatch-through: 14 --- src/backend/storage/freespace/freespace.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c index 006edab9d77..40d67a96178 100644 --- a/src/backend/storage/freespace/freespace.c +++ b/src/backend/storage/freespace/freespace.c @@ -231,18 +231,8 @@ XLogRecordPageWithFreeSpace(RelFileLocator rlocator, BlockNumber heapBlk, if (PageIsNew(page)) PageInit(page, BLCKSZ, 0); - /* - * Changes to FSM are usually marked as changed using MarkBufferDirtyHint; - * however, during recovery, it does nothing if checksums are enabled. It - * is assumed that the page should not be dirtied during recovery while - * modifying hints to prevent torn pages, since no new WAL data can be - * generated at this point to store FPI. This is not relevant to the FSM - * case, as its blocks are zeroed when a checksum mismatch occurs. So, we - * need to use regular MarkBufferDirty here to mark the FSM block as - * modified during recovery, otherwise changes to the FSM may be lost. - */ if (fsm_set_avail(page, slot, new_cat)) - MarkBufferDirty(buf); + MarkBufferDirtyHint(buf, false); UnlockReleaseBuffer(buf); } -- 2.55.0
