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

On 2026-10-26 Michael Paquier wrote:
> I'm finding the comments a bit over-complicated, so I've adjusted
> these as of the attached. What do you think?

Hey Michael,

First, thanks for taking a look so fast and for offering a solid review
and revision of the patch I proposed. I'll use your wording, it is far
better than mine. The one thing I'd keep from the longer version is a
pointer from verifyBackupPageConsistency() to the exact line in
XLogRecordAssemble() that decides to omit the image, since the two have
to stay in agreement and they're far apart in the file. A half-line is
enough:

/* no image was taken (REGBUF_NO_CHANGE in XLogRecordAssemble); skip */

> So let's leave it at v17~ where the regbuf flag exists.

Agreed, drop v14-16. I'd floated the back-branch version, but the only thing
it buys there is making wal_consistency_checking='hash' not crash a standby,
and on reflection that's not worth inventing a pin-only detection path those
branches don't otherwise need.

On 2026-10-26 Andres Freund wrote:
> I frankly think it's pretty much unacceptable for a niche index like hash
> to violate the rules like that.
> [...]
> It's probably the best answer in the back branches, but I think we
> shouldn't do this in master.

Hey Andres,

Thanks for taking the time to review, I'm curious why you used the term
"niche" with hash indexes. Not related to this topic, just an aside.

You're right, the real problem is hash CRCs a page it holds only a pin on, so
another backend can change it under the record. The clean fix is to make the
three call sites (hashbucketcleanup, _hash_freeovflpage, _hash_squeezebucket)
stop reading a page they don't content-lock and either take the buffer the
normal way, or follow the XLogSaveBufferForHint() protocol Jeff pointed at.

Where I'd push back is on making that the gate for master while the standby
crash stays unfixed. Two things make me want the small fix in master too, not
only the back branches:

1. REGBUF_NO_CHANGE's contract already is "pinned, not content-locked, not
modified here". Taking a full-page image of such a block under
XLR_CHECK_CONSISTENCY contradicts that contract on its own terms:
there's nothing to consistency-check on a block the record doesn't
change, and the page isn't safe to read while assembling the record.
So skipping the image isn't only a hash workaround it makes the
consistency-check path honor the flag's definition. Any future code
that legitimately registers a pin-only block hits the same assertion
today.

2. If master keeps taking the image and the hash rework lands later, the
rework has to touch the same three sites anyway, and in the meantime
master is the one branch where wal_consistency_checking='hash' faults.
Fixing the flag's behaviour in master and doing the hash rework as a
follow-up are not in tension, the rework doesn't depend on the image
still being taken.

Concretely, here's what I propose:

- master and v17+: the minimal fix (skip the image for REGBUF_NO_CHANGE,
with Michael's comments). This stops the standby crash on every branch
that has the flag and makes the consistency check match the contract.

- master, separate patch: make hash stop modifying pin-only buffers at the
three sites, so hash follows the buffer rules and the REGBUF_NO_CHANGE |
REGBUF_NO_IMAGE dance for "just force a cleanup lock" is no longer
needed. I'm willing to write this; it's the non-trivial part, and I'd
rather it be reviewed on its own than bundled with the crash fix.

If you'd still rather master not carry the flag-level fix at all and wait for
the hash rework, I can live with that but then I'd want the rework to be the
thing we track for master now, not a someday item, because the crash is live on
master today. Which of those do you prefer?

I'll hold the back-branch patches and post a v-next of the master patch with
Michael's comments once we've settled master-only-rework vs
flag-fix-plus-rework.

best,

-greg