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

Hello,

At Mon, 14 Nov 2016 16:53:35 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
<CAA4eK1KJAXA3PdxH4T1QJKBNOvyUK8UKm_GCvTuT+FC5jpjmjg@mail.gmail.com>
> On Mon, Nov 14, 2016 at 9:33 AM, Michael Paquier
> >>> Is it right to set XLOG_NO_PROGRESS flag in LogAccessExclusiveLocks?
> >>> This function is called not only in LogStandbySnapshot(), but during
> >>> DDL operations as well where lockmode >= AccessExclusiveLock.
> >>
> >> This does not remove any record from WAL. So theoretically any
> >> kind of record can be NO_PROGRESS, but practically as long as
> >> checkpoints are not unreasonably suppressed. Any explicit
> >> database operation must be accompanied with at least commit
> >> record that triggers checkpoint. NO_PROGRESSing there doesn't
> >> seem to me to harm database durability for this reason.
> >>
> 
> By this theory, you can even mark the insert record as no progress
> which is not good.

Of course. So we carefully choose the kinds of records to be
so. If we mark all xlog records to be SKIP_PROGRESS,
archive_timeout gets useless and as its result vacuum may leave
certain number of records not removed for maybe problematic time.

> >> The objective of this patch is skipping WALs on completely-idle
> >> state and the NO_PROGRESSing is necessary to do its work. Of
> >> course we can distinguish exclusive lock with PROGRESS and
> >> without PROGRESS but it is unnecessary complexity.
> >
> > The point that applies here is that logging the exclusive lock
> > information is necessary for the *standby* recovery conflicts, not the
> > primary  which is why it should not influence the checkpoint activity
> > that is happening on the primary. So marking this record with
> > NO_PROGRESS is actually fine to me.
> >
> 
> The progress parameter is used not only for checkpoint activity but by
> bgwriter as well for logging standby snapshot.  If you want to keep
> this record under no_progress category (which I don't endorse), then
> it might be better to add a comment, so that it will be easier for the
> readers of this code to understand the reason.

I rather agree to that. But how detailed it should be?

>LogAccessExclusiveLocks(int nlocks, xl_standby_lock *locks)
>{
>...
>    XLogRegisterData((char *) locks, nlocks * sizeof(xl_standby_lock));
>    /* Needs XLOG_SKIP_PROGRESS because called from LogStandbySnapshot */
>    XLogSetFlags(XLOG_SKIP_PROGRESS);

or 

>    /*
>        * Needs XLOG_SKIP_PROGRESS because called from LogStandbySnapshot.
>        * See the comment for LogCurrentRunningXact for the detail.
>        */

or more detiled?

The term "WAL activity' is used in the comment for
GetProgressRecPtr. Its meaning is not clear but not well
defined. Might need a bit detailed explanation about that or "WAL
activity tracking". What do you think about this?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center