Re: Fix checkpoint skip logic on idle systems by tracking LSN progress

On 9/28/16 3:35 AM, Michael Paquier wrote:
> On Wed, Sep 28, 2016 at 6:12 AM, David Steele <david(at)pgmasters(dot)net> wrote:
>> I tried the attached patch set and noticed an interesting behavior. With
>> archive_timeout=5 whenever I made a change I would get a WAL segment within
>> a few seconds as expected then another one would follow a few minutes later.
>
> That's intentional. We may be able to make XLOG_SWITCH records as not
> updating the progress LSN, but I wanted to tackle that as a separate
> patch once we got the basics done correctly, which is still what I
> think this patch is doing. I should have been more precise upthread:
> this patch makes the handling of checkpoint skip logic correct for
> only standby snapshots, not segment switches, and puts the infra to
> handle other things.

OK, I've done functional testing and this patch seems to work as
specified (including the caveat noted above). Some comments:

* [PATCH 1/3] hs-checkpoints-v12-1

+++ b/src/backend/access/transam/xlog.c
+ * Taking a lock is as well necessary to prevent potential torn reads
+ * on some platforms.

How about, "Taking a lock is also necessary..."

+ LWLockAcquire(&WALInsertLocks[i].l.lock, LW_EXCLUSIVE);

That's a lot of exclusive locks and that would seem to have performance
implications. It seems to me this is going to be a hard one to
benchmark because the regression (if any) would only be seen under heavy
load on a very large system.

In general I agree with the other comments that this could end up being
a problem. On the other hand, since the additional locks are only taken
at checkpoint or archive_timeout it may not be that big a deal.

+++ b/src/backend/access/transam/xloginsert.c * Should this record
include the replication origin if one is set up?

Outdated comment from XLogIncludeOrigin().

* [PATCH 2/3] hs-checkpoints-v12-2

+++ b/src/backend/postmaster/checkpointer.c
+ /* OK, it's time to switch */
+ elog(LOG, "Request XLog Switch");

LOG level seems a bit much here, perhaps DEBUG1?

* [PATCH 3/3] hs-checkpoints-v12-3

+ * switch segment only when any substantial progress have made from
+ * reasons will cause last_xlog_switch_lsn stay behind but it doesn't

How about, "Switch segment only when substantial progress has been made
after the last segment was switched by a timeout. Segment switching for
other reasons..."

+++ b/src/backend/access/transam/xlog.c
+ elog(LOG, "Not a forced or shutdown checkpoint: progress_lsn %X/%X,
ckpt %X/%X",
+ elog(LOG, "Checkpoint is skipped");
+ elog(LOG, "snapshot taken by checkpoint %X/%X",

Same for the above, seems like it would just be noise for most users.

+++ b/src/backend/postmaster/bgwriter.c
+ elog(LOG, "snapshot taken by bgwriter %X/%X",

Ditto.

I don't see any unintended consequences in this patch but it doesn't
mean there aren't any. I'm definitely concerned by the exclusive locks
but it may turn out they do not actually represent a bottleneck.

This does seem like the kind of patch that should get committed very
early in the release cycle to allow maximum time for regression testing.

--
-David
david(at)pgmasters(dot)net