Re: Change GetLastImportantRecPtr's definition? (wasSkip checkpoints, 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