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

On Mon, 26 Oct 2026 14:08:29 -0400, shihao zhong <zhong950419@gmail.com> wrote:

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.

While I agree that the vast majority of users rightly default to the
BTREE index I don't find that a compelling reason to drop HASH. I see
that as a challenge, maybe HASH isn't used because it is not as well
maintained as BTREE and needs some attention [0]?

we should have just ripped them out.

I have mixed feelings on this.

I think this would invite further specialization of code outside nbtree
into shapes that only support nbtree reducing the ability to innovate
with other table and index AMs.

Sure there is GIN, GiST, BRIN, etc. but IMO hash represents a quadrant
of access patterns that we should support, and our code there today
isn't inspiring many to give it a look even for use cases that fit that
pattern well. That's a signal to innovate, not remove code.

We have this issue today with HEAP, because we only have one table AM in
core things are simpler to maintain but it is also easier for heap-isms
to leak outside the table AM boundaries in the name of performance or
other goals.

IMO this is a long-term philosophical question for Postgres; is it
extensible or is it highly opinionated and specialized?

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.

I agree that there is historical baggage, performance, and possibly simple
knowledge gaps that push the adoption toward BTREE.

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?

Expression indexes prevent HOT updates, so maybe you could but at what
cost?

Thanks,
Shihao

best.

-greg

On Tue, 27 Oct 2026 08:06:48 +0900, Michael Paquier <michael@paquier.xyz> wrote:

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.
[...] if somebody is willing to work that, be it with an LLM, I would
read the code.

That plan works for me, and I'll take the hash rework (step 2).

Step 1 (down to v17, including HEAD): the flag-level fix we discussed,
with your comment wording. I'll post it as the next version shortly.
The one line I'd keep is the pointer from verifyBackupPageConsistency()
back to XLogRecordAssemble(), since the two have to stay in agreement
and they're far apart in the file.

Step 2 (make hash stop breaking the rule): the three sites that register
a pin-only primary/bucket page purely to force a replay cleanup lock
(hashbucketcleanup, _hash_freeovflpage, _hash_squeezebucket). The
tricky part isn't locking the page, it's doing so without losing the
replay-cleanup-lock behaviour the registration exists to produce, so the
standby still serialises against scans the same way. The
XLogSaveBufferForHint() protocol Jeff pointed at in 2024 is the model
I'll start from -- it already copes with WAL-logging a page that isn't
held exclusively. I'll post that on this thread for review before
worrying about step 3; once it's in, step 3 (re-add the assert, remove
REGBUF_NO_CHANGE) falls out.

There is nothing more permanent than a temporary fix [...] but I'd like
to trust in something happening for hash.

Too true!

On 2026-10-26 shihao zhong wrote:

[btree indexing a hash of the key + recheck] users can do that today with
expression index right?

Something else occured to me on this, the gap is the interesting bit.

You can do:

CREATE INDEX ON t (hashtext(col)) and then query
WHERE hashtext(col) = hashtext('x') AND col = 'x'

but you have to write the hash and the recheck yourself in every query,
and the planner won't use that index for a plain col = 'x'.

What Andres described is the planner doing that transparently for col =
'x' against a normal-looking index, which the expression-index approach
doesn't give you, and HOT is out of the question [0]/messages/by-id/78574B24-BE0A-42C5-8075-3FA9FA63B8FC@amazon.com. So it's a manual
workaround today, not the transparent feature. (Orthogonal to this
thread; just answering.)

best,

-greg

[0]: /messages/by-id/78574B24-BE0A-42C5-8075-3FA9FA63B8FC@amazon.com

Hi Greg,

That's a signal to innovate, not remove code.

I agree with that and I would like to spend some time to modernize hash,
I will start looking after the PG19 release and happy to review any of your
hash changes.

Expression indexes prevent HOT updates, so maybe you could but at what
cost?

Haven't really tested that, just some wild thinking, but I think hash index
also
have similar limitations.

Thanks,
Shihao

On Tuesday, October 27th, 2026 at 10:05 AM, shihao zhong <zhong950419@gmail.com> wrote:

Hi Greg,

That's a signal to innovate, not remove code.

I agree with that and I would like to spend some time to modernize hash,
I will start looking after the PG19 release and happy to review any of your
hash changes.

Sounds like a plan to me.

Expression indexes prevent HOT updates, so maybe you could but at what
cost?

Haven't really tested that, just some wild thinking, but I think hash index also
have similar limitations.

This is a heap limitation, and update to a row where there is an expression index
that has a column reference which was modified prevents the HOT update even if
that expression didn't result in a material change to the index key.

Thanks,
Shihao

-greg

Here is step 2, the hash rework. 0002 is the step 3 end state, marked
NOT FOR MERGE, so you can see where this lands.

0001: Don't register pin-only primary bucket pages in hash WAL records

XLOG_HASH_DELETE, XLOG_HASH_MOVE_PAGE_CONTENTS and
XLOG_HASH_SQUEEZE_PAGE register the primary bucket page only so that
redo takes a cleanup lock on it. Redo never reads data for that block.

0001 puts the block number in the record's main data instead (a new
bucket_blkno field), and redo reads the block and calls
LockBufferForCleanup() on it directly, at the same point and in the same
order as today. The page is registered only when the record changes it.

For SQUEEZE_PAGE that also covers the REGBUF_NO_CHANGE registration of
the write page (the case where the primary page is the write page but
nothing moved onto it): block 1 is now registered only when it is
modified. After this, hash registers only buffers it holds exclusively
locked and dirty.

Reading an unregistered block in redo has precedent: nbtree's "pin scan"
did exactly that (XLogReadBufferExtended + LockBufferForCleanup) until
9f83468b353, which removed it because it was no longer needed, not
because it was unsafe ACAICT. The cost is that recovery prefetch no
longer sees the bucket page for these records. The record format
changes, so XLOG_PAGE_MAGIC is bumped; master only.

Why not lock the bucket page on the primary, as Jeff asked in 2024? I
asked for an exhaustive search.

It deadlocks if you take the lock where the buffer is registered. In
_hash_freeovflpage() that point is after the metapage is locked
exclusive. A scan of a bucket that an incomplete split is still
populating holds that bucket's primary page share-locked and waits for
the metapage in _hash_get_oldblock_from_newbucket(). VACUUM holds the
metapage and waits for the primary page. Both are buffer content locks,
so nothing detects the cycle and cancel doesn't break it. I reproduced
it deterministically with an injection point (3 runs out of 3):
pg_cancel_backend() on the VACUUM does nothing, and every insert into
the index queues behind the metapage. Taking the bucket lock before the
metapage is read avoids it, and that placement passed the same test (3
of 3). That variant still costs a full-page image of the bucket page per
checkpoint and holds inserts and scans on the bucket across the
bitmap/metapage work. The approach taken in 0001 has neither cost and
takes no new lock on the primary.

I read the code and that analysis seems on target to me.

0002 [NOT FOR MERGE]: Remove REGBUF_NO_CHANGE

With 0001 there are no users left, so XLogRegisterBuffer() asserts
unconditionally that every buffer is locked and dirty. On a tree with
the step 1 patch, step 3 would also restore the
Assert(XLogRecHasBlockImage()) in verifyBackupPageConsistency() and drop
the XLogRecordAssemble() special case. On master neither exists yet.

Testing (16-vCPU Debian, cassert builds):
A = master, B = master + step 1, C = master + 0001, D = C + 0002

- CRC reproduction: primary + streaming standby,
wal_consistency_checking = 'hash', 8 sessions inserting and deleting
on a single-bucket index, aggressive autovacuum.

A: the standby stopped on "incorrect resource manager data checksum"
in all runs, as expected (control case).

B, C, D: the standby caught up with no errors, 3 of 3 each.

C and D don't have step 1, and D has neither step 1 nor the
REGBUF_NO_CHANGE escape in XLogRegisterBuffer(), with the replay
assert in verifyBackupPageConsistency() still in place.

So 0001 removes the need for the flag; it doesn't rely on step 1 to
hide the problem.

- Replay correctness: a separate run per build, with and without
wal_consistency_checking, mixing insert/rollback churn, vacuum,
rewrites of a stable row set, and four hot standby readers counting
that set through the index. Each run produced thousands of
DELETE/MOVE/SQUEEZE records with the bucket page not modified.

C and D passed both ways: no bad reads, matching counts on primary and
standby, and matching live-item counts on every bucket and overflow
page.

- Cleanup lock on the standby: a cursor on the standby is parked inside
the bucket's overflow chain, then the primary vacuums so that the
records are 9 DELETE, 4 MOVE_PAGE_CONTENTS and 4 SQUEEZE_PAGE, all
with is_primary F.

On A, C and D, replay waits on BufferCleanup until the cursor
finishes, and the cursor sees all 2250 rows.

As a control, a build of C with the cleanup lock swapped for a plain
exclusive lock lets replay run under the scan, and the cursor only
sees 1686.

- meson test regress, isolation, pg_waldump and the hash module, plus
recovery/027_stream_regress with
PG_TEST_EXTRA=wal_consistency_checking: all pass on C and D.

Of note for reviewers because this does break WAL convention, so it
deserves a bit more scrutiny.

With 0001, redo reads a block that the record does not register. It
takes the relation from a block that is always registered (block 1 for
DELETE and MOVE_PAGE_CONTENTS, block 2 for SQUEEZE_PAGE). It reads the
page with RBM_NORMAL, so a missing page goes through the same
invalid-page tracking as when the page was registered. The page isn't
modified, so it needs no full-page image and no LSN update. Two
consumers of block references stop seeing it. Recovery prefetch is one,
so redo may do one synchronous read of the bucket page per record when
the page isn't already in shared buffers. The other is tools that
collect changed blocks from WAL, such as pg_rewind and the WAL
summarizer. They used to count the bucket page as changed for these
records even though it never was, so dropping it is a fix, not a loss.

The supporting AI-generated scripts are small, attached as
scripts.tgz.nocfbot.

This fix is referenced by another thread working on hash performance
issues as required [0]/messages/by-id/auHfGQ-MN1Oij_dc@floki.

best.

-greg

[0]: /messages/by-id/auHfGQ-MN1Oij_dc@floki

On Tue, 27 Oct 2026 at 19:25, Greg Burd <greg@burd.me> wrote:

On Tue, 27 Oct 2026 08:06:48 +0900, Michael Paquier <michael@paquier.xyz> wrote:

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.
[...] if somebody is willing to work that, be it with an LLM, I would
read the code.

That plan works for me, and I'll take the hash rework (step 2).

Step 1 (down to v17, including HEAD): the flag-level fix we discussed,
with your comment wording. I'll post it as the next version shortly.
The one line I'd keep is the pointer from verifyBackupPageConsistency()
back to XLogRecordAssemble(), since the two have to stay in agreement
and they're far apart in the file.

Thanks for working on this. These comments are on the step 1 patch
(v1-0001, "Don't take a full-page image for REGBUF_NO_CHANGE
buffers").
I could reproduce the problem consistently with a VACUUM on a table
with a hash index and a concurrent INSERT, with
wal_consistency_checking = 'hash'. The test is attached; it needs a
test-only injection point after the CRC loop in XLogRecordAssemble().
Your step 1 patch fixes it.

Few comments:
1. The README, the macro comment, the code comment and the commit
message say such a buffer "never has a full-page image taken". The
code is include_image = needs_backup, so an image is still taken when
needs_backup is true. For example, _hash_freeovflpage() registers the
write buffer with REGBUF_STANDARD |  REGBUF_NO_CHANGE, without
REGBUF_NO_IMAGE, when the primary bucket page is the write page and no
tuples are moved onto it. After a checkpoint, that record carries an
ordinary FPI of the page.
+    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

Is this ok, or should we mention like "no consistency-check image"

2. The patch does not apply to REL_17 (the README hunk fails), so a
separate patch is needed for that branch.

Regards,
Vignesh

On 2026-10-27 08:06:48 +0900, Michael Paquier wrote:

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.

Agreed.

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.

Fair enough. I guess I'd hoped that one of the folks involved in adding the
hash index WAL logging would chime in and fix this properly.

There is nothing more permanent than a temporary fix, as some say,
but I'd like to trust in something happening for hash.

I guess I don't have that trust in this case. This IIRC isn't the first
complaint about the odd locking stuff that hash has.

Ceterum censeo index access/hash esse delendum.

Greetings,

Andres Freund

On Wed, Oct 28, 2026 at 11:57:53AM -0400, Andres Freund wrote:

Fair enough. I guess I'd hoped that one of the folks involved in adding the
hash index WAL logging would chime in and fix this properly.

If input does not show up, I see nothing to do here except somebody
else popping up to take responsibility for the change. Actually, I
may not mind, this stuff is annoying..

Applied the workaround down to v17 for now.

Ceterum censeo index access/hash esse delendum.

I doubt we would be able to do that, if ever.
--
Michael

On Wed, Oct 28, 2026 at 01:29:39PM +0000, Greg Burd wrote:

- CRC reproduction: primary + streaming standby,
wal_consistency_checking = 'hash', 8 sessions inserting and deleting
on a single-bucket index, aggressive autovacuum.

A: the standby stopped on "incorrect resource manager data checksum"
in all runs, as expected (control case).

B, C, D: the standby caught up with no errors, 3 of 3 each.

That sounds encouraging, as a start.

Could you start a new thread about all that, with all the authors of
this original code in CC at least? Hiding such a large refactoring on
a thread for the CRC failure would not help in terms of attracting
the attention this requires in terms of reviews.

You could also add me in CC if you wish. As far as I recall, we
rely on the registered buffer at replay to lift a bit the lock
restrictions for concurrency purposes, but that's pretty specialized
in itself.

Thanks,
--
Michael

On Wed, Oct 28, 2026 at 6:59 PM Greg Burd <greg@burd.me> wrote:

Here is step 2, the hash rework. 0002 is the step 3 end state, marked
NOT FOR MERGE, so you can see where this lands.

0001: Don't register pin-only primary bucket pages in hash WAL records

XLOG_HASH_DELETE, XLOG_HASH_MOVE_PAGE_CONTENTS and
XLOG_HASH_SQUEEZE_PAGE register the primary bucket page only so that
redo takes a cleanup lock on it. Redo never reads data for that block.

0001 puts the block number in the record's main data instead (a new
bucket_blkno field), and redo reads the block and calls
LockBufferForCleanup() on it directly, at the same point and in the same
order as today. The page is registered only when the record changes it.

For SQUEEZE_PAGE that also covers the REGBUF_NO_CHANGE registration of
the write page (the case where the primary page is the write page but
nothing moved onto it): block 1 is now registered only when it is
modified. After this, hash registers only buffers it holds exclusively
locked and dirty.

Reading an unregistered block in redo has precedent: nbtree's "pin scan"
did exactly that (XLogReadBufferExtended + LockBufferForCleanup) until
9f83468b353, which removed it because it was no longer needed, not
because it was unsafe ACAICT. The cost is that recovery prefetch no
longer sees the bucket page for these records. The record format
changes, so XLOG_PAGE_MAGIC is bumped; master only.

This approach looks reasonable to me.
*
@@ -902,6 +902,7 @@ hashbucketcleanup(Relation rel, Bucket cur_bucket,
Buffer bucket_buf,
{
xl_hash_delete xlrec;

+ xlrec.bucket_blkno = bucket_blkno;
xlrec.clear_dead_marking = clear_dead_marking;

Though harmless, we don't need this for primary bucket pages, so one
idea could be to use XLogRegisterData to register this block number
for overflow pages only. This would save four bytes for the cases
where we don't perform the cleanup operations on overflow pages, the
same would be true for other cases.

--
With Regards,
Amit Kapila.

On Tue, 2026-10-27 at 10:04 -0400, shihao zhong wrote:

I agree with that and I would like to spend some time to
modernize hash,

There have been prior efforts in this area, such as WAL logging, under
the same assumption: that people don't use hash indexes, but would if
we put more work into them.

At this point, the bar is quite high for convincing people to use them.
Being slightly better in some cases is probably not enough to get
attention. As Andres said, the long keys problem with btrees is
incidental, and can be solved other ways. Also, btree indexes aren't
standing still, which makes it even harder to make the case.

Regards,
Jeff Davis

On Mon, 2026-10-26 at 11:15 -0400, Andres Freund wrote:

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.

In theory we could generalize this. We can take any predicate x=a,
create a new implied predicate f(x)=f(a), and then use a functional
index on f(x). (Assuming f() satisfies the right properties and
equality means the very same value.)

The planner wouldn't need any extra logic to recheck, because the x=a
predicate is still there. If there's no index on (x) but there's an
index on (f(x)), then we can see if f is a member of a hash opclass, or
if it's IMMUTABLE & LEAKPROOF, and I think that's enough.

Regards,
Jeff Davis