Thread: Remove an unnecessary LSN calculation while validating WAL page header
Hi, 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'. Here's a tiny patch removing the unnecessary XLogSegNoOffsetToRecPtr() and using the passed in 'recptr'. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
Re: Remove an unnecessary LSN calculation while validating WAL page header
From
Kyotaro Horiguchi
Date:
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. > Here's a tiny patch removing the unnecessary XLogSegNoOffsetToRecPtr() > and using the passed in 'recptr'. Looks good to me. # Mysteriously, I didn't find a code to change readId in the pg92 tree.. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
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
Richard
shows other callers of XLogSegNoOffsetToRecPtr have no such issue.
Thanks
Richard
Re: Remove an unnecessary LSN calculation while validating WAL page header
From
Bharath Rupireddy
Date:
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 -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
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
-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
Re: Remove an unnecessary LSN calculation while validating WAL page header
From
Alvaro Herrera
Date:
On 2022-Oct-11, Richard Guo wrote: > As I can see in 7fcbf6a4 ValidXLogPageHeader() is refactored as True, but look at dfda6ebaec67 -- that changed the use of recaddr from being built from parts (which were globals back then) into something that was computed locally. Knowing how difficult that code was, and how heroic was to change it to a more maintainable form, I place no blame on failing to notice that some small thing could have been written more easily. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ Y una voz del caos me habló y me dijo "Sonríe y sé feliz, podría ser peor". Y sonreí. Y fui feliz. Y fue peor.
Re: Remove an unnecessary LSN calculation while validating WAL page header
From
Michael Paquier
Date:
On Tue, Oct 11, 2022 at 06:24:26PM +0200, Alvaro Herrera wrote: > Knowing how difficult that code was, and how heroic was to change it to > a more maintainable form, I place no blame on failing to notice that > some small thing could have been written more easily. +1. And even after such changes the code is still complex for the common eye. Anyway, this makes the code a bit simpler with the exact same maths, so applied. -- Michael
Attachment
On Wed, Oct 12, 2022 at 12:24 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
Knowing how difficult that code was, and how heroic was to change it to
a more maintainable form, I place no blame on failing to notice that
some small thing could have been written more easily.
Concur with that. The changes in 7fcbf6a4 made the code to a more
maintainable and readable state. It's a difficult but awesome work.
Thanks
Richard
maintainable and readable state. It's a difficult but awesome work.
Thanks
Richard