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

On Mon, Jan 18, 2016 at 10:19 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> 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.

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