From: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | amit(dot)kapila16(at)gmail(dot)com |
Cc: | michael(dot)paquier(at)gmail(dot)com, david(at)pgmasters(dot)net, pgsql-hackers(at)postgresql(dot)org, a(dot)korotkov(at)postgrespro(dot)ru |
Subject: | Re: Fix checkpoint skip logic on idle systems by tracking LSN progress |
Date: | 2016-11-15 03:57:25 |
Message-ID: | 20161115.125725.157864063.horiguchi.kyotaro@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello,
At Mon, 14 Nov 2016 16:53:35 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote in <CAA4eK1KJAXA3PdxH4T1QJKBNOvyUK8UKm_GCvTuT+FC5jpjmjg(at)mail(dot)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Haribabu Kommi | 2016-11-15 06:58:03 | Re: Patch: Write Amplification Reduction Method (WARM) |
Previous Message | Tsunakawa, Takayuki | 2016-11-15 03:25:22 | Re: [bug fix] Cascading standby cannot catch up and get stuck emitting the same message repeatedly |