On Mon, Jan 18, 2016 at 10:19 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > On Mon, Jan 18, 2016 at 10:54 AM, Michael Paquier > <michael.paqu...@gmail.com> wrote: >> >> On Sun, Jan 17, 2016 at 1:37 PM, Amit Kapila <amit.kapil...@gmail.com> >> wrote: >> > On Sat, Jan 16, 2016 at 6:37 PM, Michael Paquier >> > <michael.paqu...@gmail.com> >> > wrote: >> >> >> >> >> >> >> > >> > So here if I understand correctly the check related to the last segment >> > forcibly switched is based on the fact that if it is forcibly switched, >> > then >> > we don't need to log this record? What is the reason of such an >> > assumption? >> >> Yes, the thing is that, as mentioned at the beginning of the thread, a >> low value of archive_timeout causes a segment to be forcibly switched >> at the end of the timeout defined by this parameter. In combination >> with the standby snapshot taken in bgwriter since 9.4, this is causing >> segments to be always switched even if a system is completely idle. >> Before that, in 9.3 and older versions, we would have a segment >> forcibly switched on an idle system only once per checkpoint. >> > > Okay, so this will fix the case where the system is idle, but what I > am slightly worried is that it should not miss to log the standby snapshot > due to this check when there is actually some activity in the system. > Why is not possible to have a case such that the segment is forcibly > switched due to reason other than checkpoint (user has requested the > same) and the current insert LSN is at beginning of a new segment > due to write activity? If that is possible, which to me theoretically seems > possible, then I think bgwriter will miss to log the standby snapshot.
Yes, with segments forcibly switched by users this check would make the bgwriter not log in a snapshot if the last action done by server was XLOG_SWITCH. Based on the patch I sent, the only alternative would be to not forcedSegSwitchLSN in those code paths. Perhaps that's fine enough for back-branches.. >> The >> documentation is already clear about that actually. The whole point of >> this patch is to fix this regression, aka switch to a new segment only >> once per checkpoint. >> >> > It is not very clear by reading the comments you have >> > added in patch, may be you can expand comments slightly to explain >> > the reason of same. >> >> OK. Here are the comments that this patch is adding: >> - * only log if enough time has passed and some xlog record >> has >> - * been inserted. >> + * Only log if enough time has passed and some xlog record >> has >> + * been inserted on a new segment. On an idle system where >> + * segments can be archived in a fast pace with for example a >> + * low archive_command setting, avoid as well logging a new >> + * standby snapshot if the current insert position is still >> + * at the beginning of the segment that has just been >> switched. >> + * >> + * It is possible that GetXLogLastSwitchPtr points to the >> last >> + * position of previous segment or to the first position of >> the >> + * new segment after the switch, hence take both cases into >> + * account when deciding if a standby snapshot should be >> taken. >> + * (see comments on top of RequestXLogSwitch for more >> details). >> */ >> It makes mention of the problem that it is trying to fix, perhaps you >> mean that this should mention the fact that on an idle system standby >> snapshots should happen at worst once per checkpoint? >> > > I mean to say that in below part of comment, explanation about the > the check related to insert position is quite clear whereas why it is > okay to avoid logging standby snapshot when the segment is not > forcibly switched is not apparent. > > * avoid as well logging a new > * standby snapshot if the current insert position is still > * at the beginning of the segment that has just been switched. Perhaps: "avoid as well logging a new standby snapshot if the current insert position is at the beginning of a segment that has been *forcibly* switched at checkpoint or by archive_timeout"? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
