pg_walinspect: ReadNextXLogRecord's first_record argument

Hi,

I was looking at the code for pg_walinspect today and I think I may
have found a bug (or else I'm confused about how this all works, which
is also possible). ReadNextXLogRecord() takes an argument first_record
of type XLogRecPtr which is used only for error reporting purposes: if
we fail to read the next record for a reason other than end-of-WAL, we
complain that we couldn't read the WAL at the LSN specified by
first_record.

ReadNextXLogRecord() has three callers. In pg_get_wal_record_info(),
we're just reading a single record, and the LSN passed as first_record
is the LSN at which that record starts. Cool. But in the other two
callers, GetWALRecordsInfo() and GetWalStats(), we're reading multiple
records, and first_record is always passed as the LSN of the first
record. That's logical enough given the name of the argument, but the
effect of it seems to be that an error while reading any of the
records will be reported using the LSN of the first record, which does
not seem right.

By contrast, pg_rewind's extractPageMap() reports the error using
xlogreader->EndRecPtr. I think that's correct. The toplevel xlogreader
function that we're calling here is XLogReadRecord(), which in turn
calls XLogNextRecord(), which has this comment:

/*
* state->EndRecPtr is expected to have been set by the last call to
* XLogBeginRead() or XLogNextRecord(), and is the location of the
* error.
*/

Thoughts?

--
Robert Haas
EDB: http://www.enterprisedb.com