On Mon, Nov 14, 2016 at 9:33 AM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
> On Mon, Nov 14, 2016 at 12:49 PM, Kyotaro HORIGUCHI
> <horiguchi.kyot...@lab.ntt.co.jp> wrote:
>> At Sat, 12 Nov 2016 10:28:56 +0530, Amit Kapila <amit.kapil...@gmail.com> 
>> wrote in <caa4ek1k0ggqtbxcykqi6qnqowgzeovvphcgpj_rkobolpej...@mail.gmail.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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to