Horton

Thread

Commits

  1. Fix off-by-one possibly leading to skipped XLOG_RUNNING_XACTS records.

  2. Skip checkpoints, archiving on idle systems.

  1. pgsql: Skip checkpoints, archiving on idle systems.

    Andres Freund <andres@anarazel.de> — 2016-12-22T19:33:30Z

    Skip checkpoints, archiving on idle systems.

Some background activity (like checkpoints, archive timeout, standby
snapshots) is not supposed to happen on an idle system. Unfortunately
so far it was not easy to determine when a system is idle, which
defeated some of the attempts to avoid redundant activity on an idle
system.

To make that easier, allow to make individual WAL insertions as not
being "important". By checking whether any important activity happened
since the last time an activity was performed, it now is easy to check
whether some action needs to be repeated.

Use the new facility for checkpoints, archive timeout and standby
snapshots.

The lack of a facility causes some issues in older releases, but in my
opinion the consequences (superflous checkpoints / archived segments)
aren't grave enough to warrant backpatching.

Author: Michael Paquier, editorialized by Andres Freund
Reviewed-By: Andres Freund, David Steele, Amit Kapila, Kyotaro HORIGUCHI
Bug: #13685
Discussion:
    https://www.postgresql.org/message-id/20151016203031.3019.72930@wrigleys.postgresql.org
    https://www.postgresql.org/message-id/CAB7nPqQcPqxEM3S735Bd2RzApNqSNJVietAC=6kfkYv_45dKwA@mail.gmail.com
Backpatch: -

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/6ef2eba3f57f17960b7cd4958e18aa79e357de2f

Modified Files
--------------
doc/src/sgml/config.sgml                  |  10 +--
src/backend/access/heap/heapam.c          |  10 +--
src/backend/access/transam/xact.c         |   2 +-
src/backend/access/transam/xlog.c         | 118 +++++++++++++++++++++++-------
src/backend/access/transam/xlogfuncs.c    |   2 +-
src/backend/access/transam/xloginsert.c   |  24 ++++--
src/backend/postmaster/bgwriter.c         |   8 +-
src/backend/postmaster/checkpointer.c     |  45 ++++++++----
src/backend/replication/logical/message.c |   2 +-
src/backend/storage/ipc/standby.c         |  11 ++-
src/include/access/xlog.h                 |  12 ++-
src/include/access/xlog_internal.h        |   4 +-
src/include/access/xloginsert.h           |   2 +-
13 files changed, 173 insertions(+), 77 deletions(-)

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

    Andres Freund <andres@anarazel.de> — 2017-05-05T01:24:47Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2017-05-05T05:34:14Z

    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.

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

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

    Andres Freund <andres@anarazel.de> — 2017-05-05T06:13:10Z

    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?

- Andres

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

    Amit Kapila <amit.kapila16@gmail.com> — 2017-05-05T06:20:12Z

    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

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

    Andres Freund <andres@anarazel.de> — 2017-05-05T06:27:36Z

    Hi,

On 2017-05-05 11:50:12 +0530, Amit Kapila wrote:
> I see that EndPos can be updated in one of the cases after releasing
> the lock (refer below code). Won't that matter?

I can't see how it'd in the cases that'd matter for
GetLastImportantRecPtr() - but it'd probably good to note it in the
comment.

Thanks,

Andres

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

    Amit Kapila <amit.kapila16@gmail.com> — 2017-05-05T07:34:57Z

    On Fri, May 5, 2017 at 11:57 AM, Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
> On 2017-05-05 11:50:12 +0530, Amit Kapila wrote:
>> I see that EndPos can be updated in one of the cases after releasing
>> the lock (refer below code). Won't that matter?
>
> I can't see how it'd in the cases that'd matter for
> GetLastImportantRecPtr() - but it'd probably good to note it in the
> comment.
>

I think it should matter for any record which is not tagged as
XLOG_MARK_UNIMPORTANT, but maybe I am missing something in which case
comment should be fine.


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

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

    Andres Freund <andres@anarazel.de> — 2017-05-07T00:01:26Z

    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

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

    Michael Paquier <michael.paquier@gmail.com> — 2017-05-07T12:38:05Z

    (catching up here)

On Sun, May 7, 2017 at 9:01 AM, Andres Freund <andres@anarazel.de> wrote:
> 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.

Yeah, I have spent some time looking at many details for this stuff...
And using a start LSN location was way easier for the checkpoint skip
checks. e6c44ee looks good to me, except that I would complete this
comment block in xlog.c:
 * updated for all insertions, unless the XLOG_MARK_UNIMPORTANT flag
was
 * set. lastImportantAt is never cleared, only overwritten by the LSN
of newer
 * records.  Tracking the WAL activity directly in WALInsertLock has
the
 * advantage of not needing any additional locks to update the value.
 */
By just mentioning that the lastImportantAt is updated to the *start*
LSN position of an important record. This will help in avoiding future
confusions.

It turns out that fixing this issue only on HEAD has been a good choice.
-- 
Michael