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@anarazel.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