Re: WAL record CRC calculated incorrectly because of underlying buffer modification

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

best.

-greg

[0] https://postgr.es/m/auHfGQ-MN1Oij_dc@floki