Re: [HACKERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server - Mailing list pgsql-committers

On 2016-04-06 10:04:42 +0100, Simon Riggs wrote:
> On 6 April 2016 at 09:45, Andres Freund <andres@anarazel.de> wrote:
>
> > On 2016-04-06 09:18:54 +0100, Simon Riggs wrote:
> > > Rather than take that option, I went to the trouble of writing a patch
> > that
> > > does the same thing but simpler, less invasive and more maintainable.
> > > Primarily, I did that for you, to avoid you having wasted your time and
> > to
> > > allow you to backpatch a solution.
> >
> > But it doesn't. It doesn't solve the longstanding problem of checkpoints
> > needlessly being repeated due to standby snapshots.

> <sigh> I can't see why you say this. I am willing to listen, but this
> appears to be wrong.

The issue there is that we continue to issue checkpoints if the only
activity since the last checkpoint was emitting a standby
snapshot. That's because:
    /*
     * If this isn't a shutdown or forced checkpoint, and we have not inserted
     * any XLOG records since the start of the last checkpoint, skip the
     * checkpoint.  The idea here is to avoid inserting duplicate checkpoints
     * when the system is idle. That wastes log space, and more importantly it
     * exposes us to possible loss of both current and previous checkpoint
     * records if the machine crashes just as we're writing the update.
     * (Perhaps it'd make even more sense to checkpoint only when the previous
     * checkpoint record is in a different xlog page?)
     *
     * If the previous checkpoint crossed a WAL segment, however, we create
     * the checkpoint anyway, to have the latest checkpoint fully contained in
     * the new segment. This is for a little bit of extra robustness: it's
     * better if you don't need to keep two WAL segments around to recover the
     * checkpoint.
     */
    if ((flags & (CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_END_OF_RECOVERY |
                  CHECKPOINT_FORCE)) == 0)
    {
        if (prevPtr == ControlFile->checkPointCopy.redo &&
            prevPtr / XLOG_SEG_SIZE == curInsert / XLOG_SEG_SIZE)
        {
            WALInsertLockRelease();
            LWLockRelease(CheckpointLock);
            END_CRIT_SECTION();
            return;
        }
    }
doesn't trigger anymore.

The proposed patch allows to fix that in a more principled manner,
because we can simply check that no "important" records have been
emitted since the last checkpoint, and skip if that's the case.


> What issue is that? Previously you said it must not skip it at all for
> logical.

It's fine to skip the records iff nothing important has happened since
the last time a snapshot has been logged. Again, the proposed approach
allowed to detect that.


> > We now log more WAL with
> > XLogArchiveTimeout > 0 than without.

> And the problem with that is what?

That an idle system unnecessarily produces WAL? Waking up disks and
everything?


> I'm not much concerned with what emotive language you choose to support
> your arguments

Err.  You're side-tracking the discussion.

Greetings,

Andres Freund