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

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:
>>
>> On Sat, Jan 16, 2016 at 9:07 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>> > On Sat, Jan 16, 2016 at 5:08 PM, Michael Paquier
>> > <michael(dot)paquier(at)gmail(dot)com>
>> > wrote:
>> >>
>> >> On Sat, Jan 16, 2016 at 7:10 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> >> wrote:
>> >> > Won't this be a problem if the checkpoint occurs after a long time
>> >> > and
>> >> > in
>> >> > the mean time there is some activity in the server?
>> >>
>> >> Why? If there is some activity on the server, the snapshot will be
>> >> immediately taken at the next iteration without caring about the
>> >> checkpoint.
>> >>
>> >
>> > + (insert_lsn % XLOG_SEG_SIZE) != SizeOfXLogLongPHD))
>> >
>> > Do you mean to intend that it is protected by above check in the
>> > patch?
>>
>> Yep, in combination with is_switch_current to check if the last
>> completed segment was forcibly switched.
>>
>> > Isn't it possible that so much WAL is inserted between bgwriter cycles,
>> > that when it checks the location of WAL, it founds it to be at the
>> > beginning
>> > of a new segment?
>>
>> Er, no. Even if the insert LSN is at the beginning of a new segment,
>> this would take a standby snapshot if the last segment was not
>> forcibly switched.
>>
>
> 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. 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?
--
Michael