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

On Fri, May 5, 2017 at 11:43 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> 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?
>

I see that EndPos can be updated in one of the cases after releasing
the lock (refer below code). Won't that matter?

/*
* Even though we reserved the rest of the segment for us, which is
* reflected in EndPos, we return a pointer to just the end of the
* xlog-switch record.
*/

if (inserted)
{
EndPos = StartPos + SizeOfXLogRecord;
if (StartPos / XLOG_BLCKSZ != EndPos / XLOG_BLCKSZ)
{
if (EndPos % XLOG_SEG_SIZE == EndPos % XLOG_BLCKSZ)
EndPos += SizeOfXLogLongPHD;
else
EndPos += SizeOfXLogShortPHD;
}
}

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com