Re: Change GetLastImportantRecPtr's definition? (wasSkip checkpoints, archiving on idle systems.)

On 2017-05-05 11:04:14 +0530, Amit Kapila wrote:
> On Fri, May 5, 2017 at 6:54 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > Hi,
> >
> > On 2016-12-22 19:33:30 +0000, Andres Freund wrote:
> >> Skip checkpoints, archiving on idle systems.
> >
> > As part of an independent bugfix I noticed that Michael & I appear to
> > have introduced an off-by-one here. A few locations do comparisons like:
> > /*
> > * Only log if enough time has passed and interesting records have
> > * been inserted since the last snapshot.
> > */
> > if (now >= timeout &&
> > last_snapshot_lsn < GetLastImportantRecPtr())
> > {
> > last_snapshot_lsn = LogStandbySnapshot();
> > ...
> >
> > which looks reasonable on its face. But LogStandbySnapshot (via XLogInsert())
> > * Returns XLOG pointer to end of record (beginning of next record).
> > * This can be used as LSN for data pages affected by the logged action.
> > * (LSN is the XLOG point up to which the XLOG must be flushed to disk
> > * before the data page can be written out. This implements the basic
> > * WAL rule "write the log before the data".)
> >
> > and GetLastImportantRecPtr
> > * GetLastImportantRecPtr -- Returns the LSN of the last important record
> > * inserted. All records not explicitly marked as unimportant are considered
> > * important.
> >
> > which means that we'll e.g. not notice if there's exactly a *single* WAL
> > record since the last logged snapshot (and likely similar in the other
> > users of GetLastImportantRecPtr()), because XLogInsert() will return
> > where the next record will most of the time be inserted, and
> > GetLastImportantRecPtr() returns the beginning of said record.
> >
> > This is trivially fixable by replacing < with <=. But I wonder if the
> > better fix would be to redefine GetLastImportantRecPtr() to point to the
> > end of the record, too?
> >
>
> If you think it is straightforward to note the end of the record, then
> that sounds sensible thing to do. However, note that we remember the
> position based on lockno and lock is released before we can determine
> the EndPos.

I'm not sure I'm following:

XLogRecPtr
XLogInsertRecord(XLogRecData *rdata,
XLogRecPtr fpw_lsn,
uint8 flags)
{
...
/*
* Unless record is flagged as not important, update LSN of last
* important record in the current slot. When holding all locks, just
* update the first one.
*/
if ((flags & XLOG_MARK_UNIMPORTANT) == 0)
{
int lockno = holdingAllLocks ? 0 : MyLockNo;

WALInsertLocks[lockno].l.lastImportantAt = StartPos;
}

is the relevant bit - what prevents us from just using EndPos instead?

- Andres