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