Skip site navigation (1) Skip section navigation (2)

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

From: Andres Freund <andres(at)anarazel(dot)de>
To: pgsql-hackers(at)postgresql(dot)org,Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Subject: Change GetLastImportantRecPtr's definition? (wasSkip checkpoints, archiving on idle systems.)
Date: 2017-05-05 01:24:47
Message-ID: 20170505012447.wsrympaxnfis6ojt@alap3.anarazel.de (view raw, whole thread or download thread mbox)
Thread:
Lists: pgsql-committerspgsql-hackers
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.

- Andres


In response to

Responses

pgsql-hackers by date

Next:From: Andres FreundDate: 2017-05-05 01:26:36
Subject: Re: PG 10 release notes
Previous:From: Peter GeogheganDate: 2017-05-05 01:23:38
Subject: Re: PG 10 release notes

pgsql-committers by date

Next:From: Stephen FrostDate: 2017-05-05 02:18:54
Subject: pgsql: Change the way pg_dump retrieves partitioning info
Previous:From: Bruce MomjianDate: 2017-05-05 00:33:15
Subject: pgsql: doc: update PG 10 release notes

Privacy Policy | About PostgreSQL
Copyright © 1996-2017 The PostgreSQL Global Development Group