Re: Remove an unnecessary LSN calculation while validating WAL page header - Mailing list pgsql-hackers

From Richard Guo
Subject Re: Remove an unnecessary LSN calculation while validating WAL page header
Date
Msg-id CAMbWs4-8D1YM6oiV7wb-x5R-=fABtV5+rMCaZh1WXP8teihGXQ@mail.gmail.com
Whole thread Raw
In response to Re: Remove an unnecessary LSN calculation while validating WAL page header  (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
Responses Re: Remove an unnecessary LSN calculation while validating WAL page header  (Alvaro Herrera <alvherre@alvh.no-ip.org>)
List pgsql-hackers

On Tue, Oct 11, 2022 at 7:05 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
On Tue, Oct 11, 2022 at 3:19 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Tue, Oct 11, 2022 at 1:44 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>> At Mon, 10 Oct 2022 08:53:55 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.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

pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: Make EXPLAIN generate a generic plan for a parameterized query
Next
From: "Imseih (AWS), Sami"
Date:
Subject: Re: Add index scan progress to pg_stat_progress_vacuum