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

Hello,

Following up on the torn-page problem Shihao found and Vignesh
reproduced, and on Amit's comments. I reproduced it, tested the fixes,
and have reworked the series so the torn-page fix comes first.

The case of the torn-page: The bug is on master today, independent of
the pin-only rework. I ran Vignesh's reproducer (adapted so it finds
the primary bucket page from a record that registers it, rather than
from the removed bucket_blkno field) against three trees:

- plain master: recovery fails replaying the overflow DELETE, "invalid
page in block 7", before the later full-page image of block 7.
- the pin-only series: same failure, reached through the appended
block number instead of block 0.
- master + the fix: recovery succeeds and the index returns the right
rows.

The hazard was spotted during the original hash WAL work. In 2017
Robert raised it on the write-ahead-logging thread [1]:

"the page is registered and therefore an FPI will be taken if this is
the first reference to this page since the checkpoint, but
hash_xlog_delete won't restore the FPI. I think that exposes us to a
torn-page hazard. _hash_freeovflpage/hash_xlog_squeeze_page seems to
have the same disease, as does
_hash_squeezebucket/hash_xlog_move_page_contents."

Redo was then switched to XLogReadBufferForRedoExtended() so it restores
an image when the record has one, but these records register the primary
bucket page pin-only with REGBUF_NO_IMAGE, so there is nothing to
restore and the RBM_NORMAL read still reaches the torn page. So the
hazard stayed open; Shihao and Vignesh have now hit it.

On the fix: RBM_ZERO_ON_ERROR (Vignesh) does fix the reproducer, but as
Amit noted it is documented for non-critical forks where the caller
repairs, and this is a main-fork index page. Guarding the read with
InHotStandby (Amit's suggestion) also fixes it and is the right one:

- The read exists only to take the cleanup lock, which only matters to
serialize against a scan on a hot standby. When hot standby is not
active no such scan can exist, so neither the read nor the lock is
needed.
- That is exactly the window where the torn-page hazard lives. The
repairing image always precedes the point where hot standby becomes
active, so once InHotStandby is true any earlier image has already
been replayed and the page is no longer torn.
- It matches existing practice: nbtree and gist already gate their
redo cleanup-lock work on InHotStandby.

Since the bug is pre-existing, the fix is patch 0001: it stands on its
own against master, applies with or without the rework, and (the
back-branch question aside) is separate from the refactor. The rest of
the series then builds on it; in 0002 the same guard lives in the one
helper the refactor introduces, hash_xlog_lock_bucket().

0001 Fix hash index redo reading a torn primary bucket page
0002 Don't register pin-only primary bucket pages in hash WAL records
(merges the old 0001 and the four-byte optimization, and
renumbers the registered blocks per Amit's comment)
0003 Add a recovery test for the hash WAL pin-only bucket page fix
0004 Remove REGBUF_NO_CHANGE

For 0002 I folded the append-only-when-needed change into the main patch
so there is no common-case size increase at any point, and renumbered
the registered blocks so each record starts at 0 again now that the
primary bucket page is no longer block 0.

Testing (on current master): the new 068a recovery test fails on master
and on the pin-only series, and passes with 0001; the full recovery,
isolation and regress suites pass with PG_TEST_EXTRA=
wal_consistency_checking across the series.

best.

-greg

[1] https://postgr.es/m/CAA4eK1LwZk0MaK0LncZz6hsqcHnamfJB47nA8xuwRp4CyLFKzw@mail.gmail.com