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: http://www.postgresql.org/mailpref/pgsql-hackers
