On Fri, Oct 23, 2026 at 10:48:44AM +0000, Greg Burd wrote: > Reviving this thread because I hit the same CRC failure on current > master while testing something else.
Eh, and I thought that we were entirely done here. Nice
investigation.
> 1. Thomas's narrow fix (skip the image for REGBUF_NO_CHANGE) was ruled
> out because Michael's f56a9def seemed to rely on an image being present
> for a REGBUF_NO_CHANGE page. Looking again, f56a9def is on the squeeze
> *redo* path: it stops replay from setting an LSN on the write buffer
> (block 1) when that buffer turns out not to be modified. That redo code
> reads only the per-buffer data registered with XLogRegisterBufData() and
> the xl_hash_squeeze_page fields, never a full-page image, so suppressing
> the image does not affect it. The block that triggers the CRC failure
> is a different one: the primary bucket page registered as block 0 only
> to take a cleanup lock, which is never image-applied at replay.
> Suppressing its image is safe.
>
> 2. Jeff asked why hash can't just follow the rules and exclusive-lock +
> dirty the buffer. That is the better long-term answer, but it is a
> non-trivial rework of the hash cleanup/squeeze locking and does not
> back-patch cleanly. The attached fix is the minimal, back-patchable
> change and does not foreclose the rework.
I'm still wondering if this would not be the best thing long-term.
Perhaps not, so [shrug].
> Never take a full-page image for a REGBUF_NO_CHANGE buffer, even under
> XLR_CHECK_CONSISTENCY.
>
> The reasoning is the same one Thomas gave: there is nothing to
> consistency-check on a block the record does not change, and the page is
> not safe to read while assembling the record.
> verifyBackupPageConsistency() is taught to skip a registered block that
> carries no image, rather than asserting one is present (that assertion
> is what fires first on an assert build with the fix in place).
>
> This keeps REGBUF_NO_CHANGE as the single, documented marker for "pinned
> but not content-locked, not modified here" and makes its
> consistency-check behaviour match its contract.
Hmm, okay, I am fine with simpler for the sake of this proposition.
I'm finding the comments a bit over-complicated, so I've adjusted
these as of the attached. What do you think?
> I reproduced the failure with a primary plus a streaming standby,
> wal_consistency_checking='hash', and eight sessions doing:
>
> INSERT INTO h SELECT 1 FROM generate_series(1,400);
> DELETE FROM h WHERE ctid IN (SELECT ctid FROM h LIMIT 150);
>
> against a single-bucket hash index, with aggressive autovacuum. On
> unpatched master the standby dies with the CRC error within a few
> seconds, every run. With the patch, runs replayed clean and the standby
> caught up each time.
Ah, thanks for that. I also got a reproduction with these conditions.
> This still reproduces on all supported back branches (Alexander saw it
> as far back as 2018); REGBUF_NO_CHANGE exists only since 00d7fb5e (v17),
> so the back-patch needs the equivalent condition expressed without the
> flag. I can prepare those once the master approach has agreement.
Honestly, I am not convinced that it is worth bothering with the
v14~16 branches: REGBUF_NO_CHANGE does not exist there, and we would
need to identify a way to identify pin-only buffers, probably with an
exclusive lock check when a record is assembled, or something like
that. I really cannot get excited by that just for the sake of making
wal_consistency_checking able to work better for hash records there.
So let's leave it at v17~ where the regbuf flag exists.
@@ -2479,7 +2479,13 @@ verifyBackupPageConsistency(XLogReaderState *record)
continue;
}
- Assert(XLogRecHasBlockImage(record, block_id));
+ /*
+ * A block registered with REGBUF_NO_CHANGE is not modified by this
+ * record and carries no full-page image (see XLogRecordAssemble), so
+ * there is nothing to consistency-check for it. Skip it.
+ */
+ if (!XLogRecHasBlockImage(record, block_id))
+ continue;
This suggestion makes WAL replay weaker in the detection of a failure
mode attached to the record getting assembled. As this is linked to
only one specific condition in XLogRecordAssemble(), and you're
marking that in a comment, that would be hard to miss. I'm OK with
this suggestion. It's not like we should add a new marker in the
records for these purpose, the WAL bits are too precious for that.
What do you think about the comments I've touched?
--
Michael
From f3ead161a0814a48b4d581495e19c17a4260d0b8 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Mon, 26 Oct 2026 11:59:08 +0900 Subject: [PATCH v2] Do not take a full-page image for REGBUF_NO_CHANGE buffers A buffer registered with REGBUF_NO_CHANGE is not modified by the record and may be held with only a pin (no content lock). The hash index code registers the primary bucket page this way in hashbucketcleanup(), _hash_freeovflpage() and _hash_squeezebucket(), solely to force a cleanup lock on that block during replay. When wal_consistency_checking includes the hash resource ,anager, XLogRecordAssemble() included a full-page image for such a block anyway, because XLR_CHECK_CONSISTENCY forces an image regardless of REGBUF_NO_IMAGE. The record CRC is then computed over the image while a concurrent backend, which legitimately holds nothing stronger than a pin on that page, could modify it before the record is copied into the WAL buffers. Theis could result is an intermittent "incorrect resource manager data checksum in record" failure on a standby. As of this commit, never take an image for a REGBUF_NO_CHANGE buffer, even under XLR_CHECK_CONSISTENCY. There is no need to do a consistency check on a block not changed by a record. verifyBackupPageConsistency() is taught to skip a registered block that carries no image rather than asserting one should be present. REGBUF_NO_CHANGE exists since 00d7fb5e2e39, so backpatch down to v17. On older branches, the CRC problem can be reproduced as well, but we lack the mechanism to check how a buffer is handled by the code assembling a record. Reported-by: Alexander Lakhin <[email protected]> Author: Greg Burd <[email protected]> Diagnosed-by: Thomas Munro <[email protected]> Discussion: https://postgr.es/m/ats4pi-RM0Uom0T9@floki Backpatch-through: 17 --- src/include/access/xloginsert.h | 4 +++- src/backend/access/transam/README | 8 ++++++++ src/backend/access/transam/xloginsert.c | 9 ++++++++- src/backend/access/transam/xlogrecovery.c | 7 ++++++- 4 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/include/access/xloginsert.h b/src/include/access/xloginsert.h index 91dfbd5627f5..86264564ebb0 100644 --- a/src/include/access/xloginsert.h +++ b/src/include/access/xloginsert.h @@ -38,7 +38,9 @@ * will be skipped) */ #define REGBUF_KEEP_DATA 0x10 /* include data even if a full-page image * is taken */ -#define REGBUF_NO_CHANGE 0x20 /* intentionally register clean buffer */ +#define REGBUF_NO_CHANGE 0x20 /* intentionally register clean buffer; no + * image is forced for WAL consistency + * checking */ /* prototypes for public functions in xloginsert.c: */ extern void XLogBeginInsert(void); diff --git a/src/backend/access/transam/README b/src/backend/access/transam/README index 231106270fd4..3f4fb0a167dc 100644 --- a/src/backend/access/transam/README +++ b/src/backend/access/transam/README @@ -586,6 +586,14 @@ void XLogRegisterBuffer(uint8 block_id, Buffer buf, uint8 flags); XLogRegisterBufData() is included in the WAL record even if a full-page image is taken. + The REGBUF_NO_CHANGE flag indicates that the buffer is registered without + being modified by this record, and possibly without an exclusive content + lock (for example, to force a cleanup lock on the block during replay). + WAL consistency checking does not force a full-page image for such a + buffer, as it may be held with only a pin and be modified concurrently + while the record is assembled. Callers holding only a pin should also + pass REGBUF_NO_IMAGE, to avoid an image for a full-page write. + void XLogRegisterData(const void *data, int len); XLogRegisterData is used to include arbitrary data in the WAL record. If diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c index 59b0dfe436d1..c34d0c5ad838 100644 --- a/src/backend/access/transam/xloginsert.c +++ b/src/backend/access/transam/xloginsert.c @@ -721,8 +721,15 @@ XLogRecordAssemble(RmgrId rmid, uint8 info, /* * If needs_backup is true or WAL checking is enabled for current * resource manager, log a full-page write for the current block. + * + * Do not force an image for a REGBUF_NO_CHANGE buffer. It may be + * held with only a pin, so the page could be modified concurrently + * while the record is assembled, invalidating the record CRC. */ - include_image = needs_backup || (info & XLR_CHECK_CONSISTENCY) != 0; + if (regbuf->flags & REGBUF_NO_CHANGE) + include_image = needs_backup; + else + include_image = needs_backup || (info & XLR_CHECK_CONSISTENCY) != 0; if (include_image) { diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index 20be788da78c..13f15728ce6d 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -2479,7 +2479,12 @@ verifyBackupPageConsistency(XLogReaderState *record) continue; } - Assert(XLogRecHasBlockImage(record, block_id)); + /* + * Blocks registered with REGBUF_NO_CHANGE may have no image, see + * XLogRecordAssemble(). Nothing to compare for these. + */ + if (!XLogRecHasBlockImage(record, block_id)) + continue; if (XLogRecBlockImageApply(record, block_id)) { -- 2.56.0
signature.asc
Description: PGP signature
