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

On Fri, May 5, 2017 at 11:43 AM, Andres Freund <andres@anarazel.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@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?
>

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