Re: WAL record CRC calculated incorrectly because of underlying buffer modification

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