Re: Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby

On Mon, Jan 18, 2016 at 10:54 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:
>
> On Sun, Jan 17, 2016 at 1:37 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Sat, Jan 16, 2016 at 6:37 PM, Michael Paquier <
michael(dot)paquier(at)gmail(dot)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.

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

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