On 2017-05-05 11:04:14 +0530, Amit Kapila wrote: > On Fri, May 5, 2017 at 6:54 AM, Andres Freund <and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers