Re: Remove an unnecessary LSN calculation while validating WAL page header

On Tue, Oct 11, 2022 at 7:05 PM Bharath Rupireddy <
bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:

> On Tue, Oct 11, 2022 at 3:19 PM Richard Guo <guofenglinux(at)gmail(dot)com>
> wrote:
> > On Tue, Oct 11, 2022 at 1:44 PM Kyotaro Horiguchi <
> horikyota(dot)ntt(at)gmail(dot)com> wrote:
> >> At Mon, 10 Oct 2022 08:53:55 +0530, Bharath Rupireddy <
> bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> >> > It looks like we have an unnecessary XLogSegNoOffsetToRecPtr() in
> >> > XLogReaderValidatePageHeader(). We pass the start LSN of the WAL page
> >> > and check if it matches with the LSN that was stored in the WAL page
> >> > header (xlp_pageaddr). We find segno, offset and LSN again using
> >> > XLogSegNoOffsetToRecPtr(). This happens to be the same as the passed
> >> > in LSN 'recptr'.
> >>
> >> Yeah, that's obviously useless. It looks like a thinko in pg93 when
> >> recptr became to be directly passed from the caller instead of
> >> calculating from static variables for file, segment and in-segment
> >> offset.
> >
> >
> > +1. This should be introduced in 7fcbf6a4 as a thinko. A grep search
> > shows other callers of XLogSegNoOffsetToRecPtr have no such issue.
>
> Thanks for reviewing. It's a pretty-old code that exists in 9.5 or
> earlier [1], definitely not introduced by 7fcbf6a4.
>
> [1] see XLogReaderValidatePageHeader() in
>
> https://github.com/BRupireddy/postgres/blob/REL9_5_STABLE/src/backend/access/transam/xlogreader.c

As I can see in 7fcbf6a4 ValidXLogPageHeader() is refactored as

-static bool
-ValidXLogPageHeader(XLogPageHeader hdr, int emode, bool segmentonly)
-{
- XLogRecPtr recaddr;
-
- XLogSegNoOffsetToRecPtr(readSegNo, readOff, recaddr);

+static bool
+ValidXLogPageHeader(XLogReaderState *state, XLogRecPtr recptr,
+ XLogPageHeader hdr)
+{
+ XLogRecPtr recaddr;
+ XLogSegNo segno;
+ int32 offset;
+
+ Assert((recptr % XLOG_BLCKSZ) == 0);
+
+ XLByteToSeg(recptr, segno);
+ offset = recptr % XLogSegSize;
+
+ XLogSegNoOffsetToRecPtr(segno, offset, recaddr);

I think this is where the problem was introduced.

BTW, 7fcbf6a4 seems pretty old too as it can be found in 9.3 branch.

Thanks
Richard