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

On Mon, Nov 14, 2016 at 9:33 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Mon, Nov 14, 2016 at 12:49 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> At Sat, 12 Nov 2016 10:28:56 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote in <CAA4eK1K0gGQTBxCyKqi6QnqOWGzEoVVPHCgPJ_RkOBoLPejCTA(at)mail(dot)gmail(dot)com>
>>> I think it is good to check the performance impact of this patch on
>>> many core m/c. Is it possible for you to once check with Alexander
>>> Korotkov to see if he can provide you access to his powerful m/c which
>>> has 70 cores (if I remember correctly)?
>
> I heard about a number like that, and there is no reason to not do
> tests to be sure. With that many cores we are more likely going to see
> the limitation of the number of XLOG insert slots popping up as a
> bottleneck, but that's just an assumption without any numbers.
> Alexander (added in CC), could it be possible to get an access to this
> machine?
>
>>> @@ -1035,6 +1038,7 @@ LogAccessExclusiveLocks(int nlocks,
>>> xl_standby_lock *locks)
>>> XLogBeginInsert();
>>> XLogRegisterData((char *) &xlrec, offsetof(xl_standby_locks, locks));
>>> XLogRegisterData((char *) locks, nlocks * sizeof(xl_standby_lock));
>>> + XLogSetFlags(XLOG_NO_PROGRESS);
>>>
>>>
>>> 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.

>> 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.

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