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

Hi,

On 2026-10-26 12:17:55 +0900, Michael Paquier wrote:

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].

I frankly think it's pretty much unacceptable for a niche index like hash to
violate the rules like that. If it were btree, it'd perhaps be worthwhile to
add generic infrastructure for this, but here it doesn't seem like the juice
is worth the squeeze.

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.

I'm not really convinced by that argument. It's probably the best answer in
the back branches, but I think we shouldn't do this in master.

Greetings,

Andres Freund

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

Hi,

On October 26, 2026 10:33:16 AM EDT, Greg Burd <greg@burd.me> wrote:

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.

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.

I think they're almost unused and don't offer enough advantage over btree (and perform catastrophically bad in many cases). Instead of making them WAL logged a few years back; we should have just ripped them out.

The advantage hash does have, i.e. large key handling, would have better been addressed by adding support to transparently btree indexing a hash of the key + recheck.

Greetings,

Andres
--
Sent from my phone. Please excuse my brevity.

On 2026-10-26 Andres Freund wrote:

I think they're almost unused and don't offer enough advantage over btree
(and perform catastrophically bad in many cases). Instead of making them WAL
logged a few years back; we should have just ripped them out.

Fair, whether hash earns its place is a bigger conversation than this
bug. I'll just note one data point from the other thread I have open: a
good part of the "catastrophically bad in many cases" is the metapage
tuple counter serialising every insert on one buffer, and that looks
removable. Inserts stop touching the metapage on the common path and
hash then scales like btree on a concurrent insert workload in my
measurements. That doesn't change your conclusion about large-key
handling, but it does move some of the "performs badly" into "performed
badly".

The advantage hash does have, i.e. large key handling, would have better been
addressed by adding support to transparently btree indexing a hash of the key
+ recheck.

No disagreement that that would have been the better design. It's not
where we are, though, and ripping out a shipped, WAL-logged,
user-visible AM is its own multi-release project.

Which leaves the thing this thread actually needs a decision on,
independent of hash's long-term fate: master faults a standby under
wal_consistency_checking='hash' today. The one-line fix (skip the
full-page image for a REGBUF_NO_CHANGE block, which contradicts the
flag's own contract to take) stops that and Michael's OK with it for
v17+. You'd said you'd rather master wait for the proper hash rework.

I'm fine either way, but if master isn't getting the flag-level fix now,
I'd like the rework to be the tracked item for master rather than a
someday, the crash is live on master. So: flag fix in master now plus
rework as follow-up, or rework-only for master?

I'll build whichever, I just don't want it to stall into neither.

best,

-greg

Hi Andres and Greg,

I think they're almost unused and don't offer enough advantage over btree

I largely agree with this. In my experience, B-tree accounts for 95%+ of
indexes, and all other access methods combined make up only a
small fraction. But I do see quite a few hash index users.

we should have just ripped them out.

I have mixed feelings on this. On one hand, hash is probably the least used
"general-purpose" index since its use case overlaps directly with B-tree,
whereas GiST, SP-GiST, and BRIN exist for specialized data types or access
patterns that B-tree can't handle. On the other hand, in practice I still
see
hash indexes used at least as often as GiST, BRIN, or SP-GiST.

I suspect part of the low adoption is historical baggage from being
unlogged prior to old version, which gave hash indexes a bad reputation in
the
community, it also miss some features for general OLTP workloads

I feel there is plenty of room for improvement there.

The advantage hash does have, i.e. large key handling, would have better

been

addressed by adding support to transparently btree indexing a hash of the

key + recheck.

That's an interesting point. But I think users can do that today with
expression
index right?

Thanks,
Shihao

On Mon, Oct 26, 2026 at 09:46:18AM -0400, Andres Freund wrote:

I frankly think it's pretty much unacceptable for a niche index like hash to
violate the rules like that. If it were btree, it'd perhaps be worthwhile to
add generic infrastructure for this, but here it doesn't seem like the juice
is worth the squeeze.

I'm definitely not convinced that hash is a sufficient argument for
having more infrastructure. What this is telling us here is the
contrary: we should have less infrastructure. So I'd love to be able
to put REGBUF_NO_CHANGE in the garbage bin and have an assert in the
replay code instead of silently ignoring a case.

I'm not really convinced by that argument. It's probably the best answer in
the back branches, but I think we shouldn't do this in master.

I'm not sure that I can side with your argument here. Doing nothing
on master means that we're still open to the CRC problem until we
decide to fix hash, so it still looks like a problem that needs to be
tackled in three steps:
- Avoid the CRC problem down to v17, including HEAD.
- Address hash to avoid the rule breakage.
- Lift the check in replay for wal_consistency_checking (re-add the
assert) and push REGBUF_NO_CHANGE into orbit.

There is nothing more permanent than a temporary fix, as some say,
but I'd like to trust in something happening for hash. I'm not
planning to code that myself, though, but if somebody is willing to
work that, be it with an LLM, I would read the code.

That's just to say that I don't see a downside with including HEAD in
step 1.
--
Michael