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

Hello,

Thanks Amit for the review, and thanks Shihao for finding the torn-page
case.

Per Amit's suggestion I merged the old 0001 and 0002 into a single
patch. The primary bucket block number is now appended to the record
only when the primary bucket page is not one of the pages the record
changes, so there is no common-case size increase at any point in the
series. The structs never carry the block number.

I also renumbered the registered blocks as Amit noted. Since the
primary bucket page is no longer registered as block 0 in DELETE,
MOVE_PAGE_CONTENTS or SQUEEZE_PAGE, I shifted the remaining blocks down
by one, so each record's registered blocks start at 0 again:

DELETE block 0: page tuples are deleted from
MOVE_PAGE_CONTENTS block 0: page tuples are moved to
block 1: page tuples are moved from
SQUEEZE_PAGE block 0: write page (if modified)
block 1: freed overflow page
block 2: page before the freed overflow page
block 3: page after the freed overflow page
block 4: bitmap page
block 5: meta page

The record descriptions in hash_xlog.h are updated to match, and the
recovery test still selects the overflow-page DELETE by its main-data
length, which does not change.

0001 Don't register pin-only primary bucket pages in hash WAL records
(now includes the append-only-when-needed change; Vignesh,
Amit's suggestion)
0002 Add a recovery test for the hash WAL pin-only bucket page fix
(co-authored with Vignesh)
0003 Remove REGBUF_NO_CHANGE

The case of the torn-page

I reproduced and traced Shihao's case and I agree it is a real,
pre-existing bug on master, not something this series introduces. The
sequence is: with data checksums on, a VACUUM writes an XLOG_HASH_DELETE
for an overflow page while holding only a pin on the primary bucket
page, so the primary page is not in that record. A later record carries
the first full-page image of the primary page.

If the primary page is torn on disk and recovery replays the DELETE
before that image, redo reads the primary page only to take the cleanup
lock, with RBM_NORMAL, and the read fails the checksum and raises
"invalid page", before the image that would repair it is reached.

On master today the same page is read through block 0 with the same
RBM_NORMAL, so the failure is identical with and without the series.

0001 just reaches the page through the appended block number instead of
block 0. So this is worth fixing on its own, and the fix is orthogonal
to whether we register the page or carry its block number.

On the approaches raised so far:

- RBM_ZERO_ON_ERROR (Vignesh): it does make the read tolerate the torn
page, and a later FPI repairs it. But as Amit noted it is documented
for non-critical forks (VM, FSM) where the caller repairs, and we'd be
pointing it at a main-fork index page, which reads wrong even if it
happens to work here. I share the hesitation.

- HotStandbyActiveInReplay() guard (Amit): the cleanup lock on the
primary page exists only to keep a hot-standby scan from running in
the bucket during replay. During crash recovery and before
consistency, no such scan can exist, so there is nothing to serialize
against and the read/lock is not needed then. That lines up with why
the lock is taken at all, and it avoids reading the page in exactly
the window where the torn copy has not yet been repaired. I think
this is the most promising direction.

- Lock the bucket page even when it is registered unchanged (Amit): I'll
look at this too, together with the deadlock I described earlier in
the thread when taking the primary-page lock in _hash_freeovflpage()
after the metapage is locked.

I'd rather not pick one and bake it into this series until we've
actually tested them against Shihao's and Vignesh's reproducers (and
confirmed the HotStandbyActiveInReplay path is safe on a standby that
reaches consistency mid-bucket). I'll do that and come back with a
separate patch for the torn-page fix, since it stands on its own against
master.

Does treating it as its own fix, rather than folding it into 0001, sound
right to you? Does anyone think this should be a separate patch against
master?

Patches 0001-0003 attached, against current master.

best.

-greg