At Fri, 11 Dec 2020 19:27:31 +0000, "Bossart, Nathan" <bossartn@amazon.com> wrote in
> On 12/10/20, 9:23 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> > 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
> > cause DecodeXLogRecord() to report an error with the end of the
> > current record but it makes more sense to point to ReadRecPtr in this
> > 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.
>
> I looked through all the calls to report_invalid_record() in
> xlogreader.c and noticed that all but a few in
> XLogReaderValidatePageHeader() already report an LSN. Of the calls in
> XLogReaderValidatePageHeader() that don't report an LSN, it looks like
> most still report a position, and the remaining ones are for "WAL file
> is from different database system...," which IIUC generally happens on
> the first page of the segment.
>
> Perhaps we could simply omit the LSN in the pg_waldump message.
Yeah, I had the same feeling. At least, the two LSNs in the message
under discussion are simply redundant. So +1 to just remove the LSN at
the caller site.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center