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

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.

0001 instead stores the primary bucket page's block number in the main
data of these three records (a new bucket_blkno field) and has replay
read that block and call LockBufferForCleanup() on it directly, at the
same point and in the same order as today. The page is registered only
when the record actually changes it. For the squeeze record this also
removes the pin-only registration of the write page (the case where the
write page is the primary bucket page and nothing is moved onto it).

After 0001, hash registers only buffers it holds exclusively locked and
dirty, and there are no REGBUF_NO_CHANGE users left anywhere in the
tree.

Reading an unregistered block in redo has precedent: nbtree's "pin scan"
did exactly that (XLogReadBufferExtended + LockBufferForCleanup) until
9f83468b353 removed it as no longer needed, not as unsafe. Two
consequences, both benign:

- Recovery prefetch no longer sees the bucket page for these records,
so redo may do one synchronous read of it when it is not already in
shared buffers.
- Block-change consumers (pg_rewind, the WAL summarizer) stop counting
the bucket page as changed by these records. It never was changed,
so this is a correction, not a loss.

The three record formats change, so 0001 bumps XLOG_PAGE_MAGIC; master
only.

Why not just content-lock the primary page, as Jeff asked in [1]?

I looked for a safe placement and it deadlocks where the buffer is
registered. In _hash_freeovflpage() that point is after the metapage is
locked exclusive. A scan of a bucket still being populated by an
incomplete split 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 the deadlock detector does not see the cycle and a cancel does
not break it. I reproduced it deterministically with an injection point
(3 of 3): pg_cancel_backend() on the VACUUM does nothing and every
insert queues behind the metapage. Taking the bucket lock before the
metapage is read avoids the deadlock, but 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. 0001 as proposed
has neither cost and takes no new lock on the primary page.

0002: Remove REGBUF_NO_CHANGE

With 0001 there are no users of the flag left. 0002 removes it,
restores the unconditional assert in XLogRegisterBuffer() that every
registered buffer is content-locked and dirty, and reverts the two
defensive measures from 27caf4ca5ae5 on master: the XLogRecordAssemble()
special case and the verifyBackupPageConsistency() skip. 27caf4ca5ae5
stays in the back branches, where this hash rework is not applied.

Testing (16-vCPU Debian, cassert, master incl. 27caf4ca5ae5)

- meson test regress, isolation, and recovery (including
recovery/027_stream_regress with
PG_TEST_EXTRA=wal_consistency_checking): all pass with 0001+0002
applied.

- CRC reproduction with 0001+0002, i.e. after 0002 has removed the
step-1 escape and re-added the asserts: primary + streaming standby,
wal_consistency_checking='hash', 8 sessions inserting and deleting on
a single-bucket index with aggressive autovacuum. The standby caught
up with no "incorrect resource manager data checksum", no inconsistent
page, and no assertion failure, 3 of 3. This shows the fix comes from
the hash rework itself, not from the step-1 safety net.

(Earlier, more detailed A/B/C/D runs on the older posting are in [3],
including a hot-standby cursor control showing the cleanup lock still
serializes replay against an in-progress scan.)

Open item from review:

Amit suggested [4] registering bucket_blkno only for the overflow-page
cases, since for a primary-bucket-page record the block is already
known, saving 4 bytes on the common records. I have not made that
change yet; I can in the next version if we agree it is worth the extra
branch in each redo path.

Vignesh contributed a CRC injection-point test on the old thread that
I'd like to review and see how that might fit in and help.

best.

-greg

[1] https://postgr.es/m/055bb33c-bccc-bc1d-c2f8-8598534448ac@gmail.com
[2] https://postgr.es/m/auKvDKa3umaD7Hwi@paquier.xyz
[3] https://postgr.es/m/auHpsdSdOXspzuqW@floki
[4] https://postgr.es/m/CAA4eK1+JpYJnUWwrSO7=gs=P3qHkYTrV+gMMPKY-ERv3pHdt-Q@mail.gmail.com