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

On Fri, 17 May 2024 07:56:23 -0700, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Fri, 2024-05-17 at 10:12 +0900, Michael Paquier wrote:
> > That's something I've done four weeks ago in the hash replay code
> > path, and having the image with XLR_CHECK_CONSISTENCY even if
> > REGBUF_NO_CHANGE was necessary because replay was setting up a LSN on
> > a REGBUF_NO_CHANGE page it should not have touched.
>
> Then the candidate fix to selectively break XLR_CHECK_CONSISTENCY is
> not acceptable.
>
> >
> > Yeah, agreed that getting rid of REGBUF_NO_CHANGE would be nice in
> > the
> > final picture.  It still strikes me as a weird concept that WAL
> > replay
> > for hash indexes logs full pages just to be able to lock them at
> > replay based on what's in the records.  :/
>
> I'm still not entirely clear on why hash indexes can't just follow the
> rules and exclusive lock the buffer and dirty it. Presumably
> performance would suffer, but I asked that question previously and
> didn't get an answer:
>
> https://www.postgresql.org/message-id/CA%2BTgmoY%2BdagCyrMKau7UQeQU6w4LuVEu%2ByjsmJBoXKAo6XbUUA%40mail.gmail.com
>
> And if that does affect performance, what about following the same
> protocol as XLogSaveBufferForHint()?
>
> Regards,
> Jeff Davis

Hi,

Reviving this thread because I hit the same CRC failure on current
master while testing something else.

Recap of the bug:

hashbucketcleanup(), _hash_freeovflpage() and _hash_squeezebucket()
register the primary bucket page with REGBUF_STANDARD | REGBUF_NO_IMAGE
| REGBUF_NO_CHANGE, holding only a pin on it, purely to force a cleanup
lock during replay. With wal_consistency_checking covering the hash RM,
XLogRecordAssemble() takes a full-page image of that block anyway:

include_image = needs_backup || (info & XLR_CHECK_CONSISTENCY) != 0;

The CRC is then computed over a page another backend is free to modify
(it holds only a pin), and before CopyXLogRecordToWAL() runs the page
changes, so the record lands with a bad CRC. On a standby that is a
fatal "incorrect resource manager data checksum in record".

AFAICT, 2 things kept this open last time:

1. Thomas's narrow fix (skip the image for REGBUF_NO_CHANGE) was ruled
out because Michael's f56a9def seemed to rely on an image being present
for a REGBUF_NO_CHANGE page. Looking again, f56a9def is on the squeeze
*redo* path: it stops replay from setting an LSN on the write buffer
(block 1) when that buffer turns out not to be modified. That redo code
reads only the per-buffer data registered with XLogRegisterBufData() and
the xl_hash_squeeze_page fields, never a full-page image, so suppressing
the image does not affect it. The block that triggers the CRC failure
is a different one: the primary bucket page registered as block 0 only
to take a cleanup lock, which is never image-applied at replay.
Suppressing its image is safe.

2. Jeff asked why hash can't just follow the rules and exclusive-lock +
dirty the buffer. That is the better long-term answer, but it is a
non-trivial rework of the hash cleanup/squeeze locking and does not
back-patch cleanly. The attached fix is the minimal, back-patchable
change and does not foreclose the rework.

Proposed fix:

Never take a full-page image for a REGBUF_NO_CHANGE buffer, even under
XLR_CHECK_CONSISTENCY.

The reasoning is the same one Thomas gave: there is nothing to
consistency-check on a block the record does not change, and the page is
not safe to read while assembling the record.
verifyBackupPageConsistency() is taught to skip a registered block that
carries no image, rather than asserting one is present (that assertion
is what fires first on an assert build with the fix in place).

This keeps REGBUF_NO_CHANGE as the single, documented marker for "pinned
but not content-locked, not modified here" and makes its
consistency-check behaviour match its contract.

Testing:

I reproduced the failure with a primary plus a streaming standby,
wal_consistency_checking='hash', and eight sessions doing:

INSERT INTO h SELECT 1 FROM generate_series(1,400);
DELETE FROM h WHERE ctid IN (SELECT ctid FROM h LIMIT 150);

against a single-bucket hash index, with aggressive autovacuum. On
unpatched master the standby dies with the CRC error within a few
seconds, every run. With the patch, runs replayed clean and the standby
caught up each time.

This still reproduces on all supported back branches (Alexander saw it
as far back as 2018); REGBUF_NO_CHANGE exists only since 00d7fb5e (v17),
so the back-patch needs the equivalent condition expressed without the
flag. I can prepare those once the master approach has agreement.

best.

-greg