Re: Hash index: stop registering pin-only bucket pages in WAL (remove REGBUF_NO_CHANGE)

On Thu, 29 Oct 2026 at 21:42, Greg Burd <greg(at)burd(dot)me> wrote:
>
> Hi again,
>
> This is a follow-up to the hash-index WAL CRC thread [1]. Michael asked
> that the larger hash rework not ride on that thread, since it is a
> refactor that needs its own review attention rather than being buried
> under a bug report [2]:
>
> "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."
>
> So here it is, on its own thread, with the hash WAL-logging authors in
> CC.
>
> Background:
>
> The CRC failure in [1] was that, under wal_consistency_checking='hash',
> a standby could stop with "incorrect resource manager data checksum in
> record". The cause: hashbucketcleanup(), _hash_squeezebucket() and
> _hash_freeovflpage() register the bucket's primary page with
> REGBUF_NO_CHANGE while holding only a pin on it (no content lock),
> solely so that replay takes a cleanup lock on that page.
> XLR_CHECK_CONSISTENCY then forces a full-page image of a page a
> concurrent inserter is still free to modify, and the record CRC is
> computed over shifting data.
>
> The immediate CRC fix is committed and back-patched to v17 as
> 27caf4ca5ae5 ("Do not take a full-page image for REGBUF_NO_CHANGE
> buffers"): XLogRecordAssemble() no longer forces an image for a
> REGBUF_NO_CHANGE buffer, and verifyBackupPageConsistency() skips a
> registered block that has no image.
>
> That stops the bleeding but leaves the real problem in place: hash
> registers a buffer it does not hold locked or dirty, which breaks the
> invariant every other caller of XLogRegisterBuffer() keeps. The plan
> proposed (agreed on?) in [1] was three steps:
>
> 1. Avoid the CRC problem down to v17, including HEAD. [done: 27caf4ca5ae5]
> 2. Fix hash so it stops breaking the rule. [0001 here]
> 3. Remove REGBUF_NO_CHANGE and restore the asserts. [0002 here]
>
> This thread is steps 2 and 3.
>
> 0001: Don't register pin-only primary bucket pages in hash WAL records
>
> Replay of XLOG_HASH_DELETE, XLOG_HASH_MOVE_PAGE_CONTENTS and
> XLOG_HASH_SQUEEZE_PAGE must take a cleanup lock on the primary bucket
> page so that a hot-standby scan is not in progress in the bucket, and
> cannot start in it, while tuples are removed or moved. Today that lock
> is obtained by registering the page (pin-only) with REGBUF_NO_CHANGE.
> Redo never reads data from that block; the registration exists only for
> the lock.

The patches change the WAL records, and XLOG_PAGE_MAGIC cannot be
bumped in a minor release, so I looked at what happens if they are
backpatched. I built the base REL_18_STABLE, REL_18 + 0001, + 0002,
and + 0003 (attached, adapted for REL_18: the squeeze code there lacks
the locals present on master, so 0001 computes mod_wbuf itself) and
ran them against each other with streaming replication and crash
recovery, with wal_consistency_checking = 'hash' and VACUUM of hash
indexes in which every shape of the three records occurs.

Results (primary -> standby; all rows and index/heap scans checked):
p3 -> base works
base -> p3 works
p1 -> base fails: failed to locate backup block with ID 0
p2 -> base fails: same
base -> p1 fails: replay reads bucket_blkno that old
records do not have
base -> p2 fails: same

So 0001 and 0002 on their own are not safe in a minor release, in
either direction: a) a new primary writes records without block 0,
which an old standby cannot replay b) a new standby, or crash recovery
after upgrade, replays records written by an old server, which have
block 0 and no appended block number.

I'm not sure if we should back patch this patch. If we plan to back
patch, here is a patch for the same. The 0001 and 0002 are the same
patches as the v3 version posted in previous mail (rebased for PG18)
and 0003 has the backward compatible patch to fix them.

Thoughts?

Regards,
Vignesh