Hello, At Mon, 14 Nov 2016 16:53:35 +0530, Amit Kapila <amit.kapil...@gmail.com> wrote in <caa4ek1kjaxa3pdxh4t1qjkbnovyuk8ukm_gcvtut+fc5jpj...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers