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

On Fri, 30 Oct 2026 17:18:34 +0530, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> 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.

Hi Vignesh,

Thanks for picking this up and for the two things you added: splitting
the four-byte saving out as its own patch, and the back-branch analysis.

On the structure: I've taken your v3-0001 (same as my v2-0001) and your
v3-0002 (append the block number only when the primary page is not
registered) as the base, and built the rest of the series on top. The
attached v3 is:

0001 Don't register pin-only primary bucket pages in hash WAL records
(unchanged from your v3-0001 / my v2-0001)
0002 Append the hash primary bucket block number to WAL records only
when needed (your v3-0002, Amit's suggestion)
0003 Add a recovery test for the hash WAL pin-only bucket page fix
0004 Remove REGBUF_NO_CHANGE

0002 keeps the records the same size as today in the common case, so I
think it belongs in the series rather than as a follow-up; thanks for
writing it.

0003 is the recovery test. It turns the CRC window into a deterministic
one with a general, test-only injection point in XLogRecordAssemble(),
right after the record CRC is computed and before the record is copied
into the WAL buffers. The injection point's argument names the record
as "rmid/info/mainlen", so the test waits on exactly the overflow-page
XLOG_HASH_DELETE (whose main data carries the appended bucket block
number, so it is distinguishable from the primary-page delete), lets a
concurrent INSERT change the pin-only primary bucket page, then crashes
and checks that pg_waldump and crash recovery both accept the WAL. This
is your test from the CRC thread, carried forward: the record selector
moved from the REGBUF_NO_CHANGE buffer count (which the rework zeroes)
to the main-data length so it still picks the right record after the
rework. I kept you as a co-author.

0004 removes REGBUF_NO_CHANGE and restores the unconditional assert in
XLogRegisterBuffer(), and reverts the two defensive measures from
27caf4ca5ae5 on master.

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

On your back-branch question: I don't think we should back-patch this.
27caf4ca5ae5 already fixes the actual CRC failure down to v17, so the
back branches are correct as they stand. The rework here is a cleanup
that changes three WAL record formats and bumps XLOG_PAGE_MAGIC, which,
as your p1/p2-vs-base runs show, is not compatible across a minor
release in either direction. That is exactly the kind of change we keep
to master. So I'd leave the back branches on 27caf4ca5ae5 and not carry
your v3_REL18-0003 compatibility shim; it is good to have proven the
incompatibility, but the conclusion is "master only", which 0001 already
says in its XLOG_PAGE_MAGIC bump.

One fix went into 0002 after a first CI round: pulling the primary
bucket block number out of the record struct into its own data segment
needs one more slot than XLogEnsureRecordSpace() reserved in
_hash_freeovflpage() and the move-page-contents path, so a 32-bit build
hit "too much WAL data" during a hash VACUUM that packs more tuples per
WAL record than a 64-bit build does. 0002 now reserves the extra
segment.

Patches 0001-0004 attached, against current master.

best.

-greg