Re: [HACKERS] Change GetLastImportantRecPtr's definition? (wasSkipcheckpoints, archiving on idle systems.)

Hi,

On 2017-05-04 18:24:47 -0700, Andres Freund 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?  I don't quite see any upcoming user that'd need
> the beginning, and this is a bit failure prone for likely users.

Turns out this isn't the better fix, because the checkpoint code
compares with the actual record LSN (rather than the end+1 that
XLogInsert() returns).  We'd start having to do more bookkeeping or more
complicated computations (subtracting the checkpoint record's size).
Therefore I've pushed the simple fix mentioned above instead.

- Andres