On Fri, 17 May 2024 07:56:23 -0700, Jeff Davis <[email protected]> wrote:
> On Fri, 2024-05-17 at 10:12 +0900, Michael Paquier wrote:
> > That's something I've done four weeks ago in the hash replay code
> > path, and having the image with XLR_CHECK_CONSISTENCY even if
> > REGBUF_NO_CHANGE was necessary because replay was setting up a LSN on
> > a REGBUF_NO_CHANGE page it should not have touched.
>
> Then the candidate fix to selectively break XLR_CHECK_CONSISTENCY is
> not acceptable.
>
> >
> > Yeah, agreed that getting rid of REGBUF_NO_CHANGE would be nice in
> > the
> > final picture. It still strikes me as a weird concept that WAL
> > replay
> > for hash indexes logs full pages just to be able to lock them at
> > replay based on what's in the records. :/
>
> I'm still not entirely clear on why hash indexes can't just follow the
> rules and exclusive lock the buffer and dirty it. Presumably
> performance would suffer, but I asked that question previously and
> didn't get an answer:
>
> https://www.postgresql.org/message-id/CA%2BTgmoY%2BdagCyrMKau7UQeQU6w4LuVEu%2ByjsmJBoXKAo6XbUUA%40mail.gmail.com
>
> And if that does affect performance, what about following the same
> protocol as XLogSaveBufferForHint()?
>
> Regards,
> Jeff Davis
Hi,
Reviving this thread because I hit the same CRC failure on current
master while testing something else.
Recap of the bug:
hashbucketcleanup(), _hash_freeovflpage() and _hash_squeezebucket()
register the primary bucket page with REGBUF_STANDARD | REGBUF_NO_IMAGE
| REGBUF_NO_CHANGE, holding only a pin on it, purely to force a cleanup
lock during replay. With wal_consistency_checking covering the hash RM,
XLogRecordAssemble() takes a full-page image of that block anyway:
include_image = needs_backup || (info & XLR_CHECK_CONSISTENCY) != 0;
The CRC is then computed over a page another backend is free to modify
(it holds only a pin), and before CopyXLogRecordToWAL() runs the page
changes, so the record lands with a bad CRC. On a standby that is a
fatal "incorrect resource manager data checksum in record".
AFAICT, 2 things kept this open last time:
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.
Proposed fix:
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.
Testing:
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.
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.
best.
-greg
From 09a645463af50abdfb3b9ea20decee42ff88d4b1 Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
Date: Thu, 22 Oct 2026 14:43:17 -0400
Subject: [PATCH] Don't 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 RM, 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, is
free to modify it before the record is copied into the WAL buffers. The
result is an intermittent "incorrect resource manager data checksum in
record" failure on a standby (or under a crash-recovery consistency
check), reproducible on the buildfarm with wal_consistency_checking and
concurrent inserts plus autovacuum on a hash index.
Fix by never taking an image for a REGBUF_NO_CHANGE buffer, even under
XLR_CHECK_CONSISTENCY. 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.
Reported-by: Alexander Lakhin
Diagnosed-by: Thomas Munro
Discussion: https://postgr.es/m/[email protected]
---
src/backend/access/transam/README | 9 +++++++++
src/backend/access/transam/xloginsert.c | 15 ++++++++++++++-
src/backend/access/transam/xlogrecovery.c | 8 +++++++-
src/include/access/xloginsert.h | 7 ++++++-
4 files changed, 36 insertions(+), 3 deletions(-)
diff --git a/src/backend/access/transam/README
b/src/backend/access/transam/README
index 231106270fd..138c4c0ada6 100644
--- a/src/backend/access/transam/README
+++ b/src/backend/access/transam/README
@@ -586,6 +586,15 @@ 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).
+ Such a buffer never has a full-page image taken, not even when WAL
+ consistency checking is enabled: its contents may change concurrently
+ while the record is assembled, which would compute the record CRC over
+ data a concurrent backend can still modify, and there is nothing to
+ consistency-check on a block the record does not change.
+
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 59b0dfe436d..c3c95f89276 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -721,8 +721,21 @@ 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.
+ *
+ * However, a buffer registered with REGBUF_NO_CHANGE is not
modified
+ * by this record and may be held with only a pin (no content
lock),
+ * so its contents could change under us while we assemble the
record.
+ * Including such a page would compute the record CRC over data
that a
+ * concurrent backend can modify before the record is copied
into the
+ * WAL buffers, producing a spurious "incorrect resource
manager data
+ * checksum" failure. These blocks are registered only to
force a
+ * cleanup lock during replay, so there is nothing to
consistency-check
+ * anyway; never take an image for them.
*/
- 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 20be788da78..52cb5cdc0a1 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -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;
if (XLogRecBlockImageApply(record, block_id))
{
diff --git a/src/include/access/xloginsert.h b/src/include/access/xloginsert.h
index 91dfbd5627f..6ae2692d5b9 100644
--- a/src/include/access/xloginsert.h
+++ b/src/include/access/xloginsert.h
@@ -38,7 +38,12 @@
* 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;
+ *
suppresses the full-page image that
+ *
XLR_CHECK_CONSISTENCY would otherwise
+ *
take, since the page is not changed by
+ * this
record and may not be
+ *
content-locked */
/* prototypes for public functions in xloginsert.c: */
extern void XLogBeginInsert(void);
--
2.54.0
