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

On Thu, May 16, 2024 at 03:54:52PM -0700, Jeff Davis wrote:
> On Mon, 2024-05-13 at 11:15 +1200, Thomas Munro wrote:
>>>>>> Perhaps a no-image, no-change registered buffer should not be
>>>>>> including an image, even for XLR_CHECK_CONSISTENCY?  It's
>>>>>> actually
>>>>>> useless for consistency checking too I guess, this issue
>>>>>> aside,
>>>>>> because it doesn't change anything so there is nothing to
>>>>>> check.
>
> I'm not convinced by that reasoning. Can't it check that nothing has
> changed?

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.

> I'd prefer that we find a way to get rid of REGBUF_NO_CHANGE and make
> all callers follow the rules than to find new special cases that depend
> on REGBUF_NO_CHANGE. See these messages here:
>
> https://www.postgresql.org/message-id/b1f2d0f230d60fa8df33bb0b2af3236fde9d750d.camel%40j-davis.com
>
> https://www.postgresql.org/message-id/CA%2BTgmoY%2BdagCyrMKau7UQeQU6w4LuVEu%2ByjsmJBoXKAo6XbUUA%40mail.gmail.com
>
> In other words, we added REGBUF_NO_CHANGE for the call sites (only hash
> indexes) that don't follow the rules and where it wasn't easy to make
> them follow the rules. Now that we know of a concrete problem with the
> design, there's more upside to fixing it properly.

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. :/
--
Michael