Remove an unnecessary LSN calculation while validating WAL page header

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

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]see XLogReaderValidatePageHeader() in https://github.com/BRupireddy/postgres/blob/REL9_5_STABLE/src/backend/access/transam/xlogreader.c, definitely not introduced by 7fcbf6a4.

[1]: see XLogReaderValidatePageHeader() in https://github.com/BRupireddy/postgres/blob/REL9_5_STABLE/src/backend/access/transam/xlogreader.c
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

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.

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

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