Re: pg_waldump error message fix

At Fri, 11 Dec 2020 17:19:33 +0900 (JST), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote in
> At Fri, 11 Dec 2020 14:21:57 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in
> > On Fri, Dec 11, 2020 at 01:30:16PM +0900, Kyotaro Horiguchi wrote:
> > > currRecPtr is a private member of the struct (see the definition of
> > > the struct XLogReaderState). We should instead use EndRecPtr outside
> > > xlog reader.
> >
> > Please note that this is documented in xlogreader.h. Hmm. I can see
> > your point here, still I think that what both of you are suggesting
> > is not completely correct. For example, switching to EndRecPtr would
>
> EndRecPtr is defined as it points to the LSN to start reading the next
> record. It donsn't move if XLogReadRecord failed to read the
> record. I think this is documented in a comment somewhere. It can
> point to the beginning of a page so "error in WAL record at <page
> start>" is a kind of bogus but that is not the point here.
>
> > cause DecodeXLogRecord() to report an error with the end of the
> > current record but it makes more sense to point to ReadRecPtr in this
>
> DecodeXLogRecord() handles a record alread successflly read. So
> ReadRecPtr is pointing to the beginning of the given record at the
> timex. pg_waldump:main() and ReadRecrod (or the context of
> DecodeXLogRecord()) are in different contexts. The place in question
> in pg_waldump seems to be a result of a thinko that it can use
> ReadRecPtr regardless of whether XLogReadRecrod successfully read a
> record or not.
>
> > case. On the other hand, it would make sense to report the beginning
> > of the position we are working on when validating the header if an
> > error happens at this point. So it seems to me that EndRecPtr or
> > ReadRecPtr are not completely correct, and that we may need an extra
> > LSN position to tell on which LSN we are working on instead that gets
> > updated before the validation part, because we update ReadRecPtr and
> > EndRecPtr only after this initial validation is done.
>
> So we cannot use the ErrorRecPtr since pg_waldump:main() shoud show
> the LSN XLogReadRecord() found a invalid record and DecodeXLogRecord()
> should show the LSN XLogReadRecord() found a valid record.

Wait! That's wrong. Yeah, we can add ErrorRecPtr to point the error
record regardless of the context.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center