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

On Wed, Feb 10, 2016 at 1:01 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

>
>
> On Wed, Feb 10, 2016 at 4:11 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>>
>>
>>
>>>
>>> >> > - last_snapshot_lsn != GetXLogInsertRecPtr())
>>> >> > +
>>> >> > GetLastCheckpointRecPtr() < GetProgressRecPtr())
>>> >> >
>>> >> > How the above check in patch suppose to work?
>>> >> > I think it could so happen once bgwriter logs snapshot, both
>>> checkpoint
>>> >> > and progressAt won't get updated any further and I think this check
>>> will
>>> >> > allow to log snapshots in such a case as well.
>>> >>
>>> >> The only purpose of this check is to do the following thing: if no WAL
>>> >> activity has happened since last checkpoint, there is no need to log a
>>> >> standby snapshot from the perspective of the bgwriter. In case the
>>> >> system is idle, we want to skip logging that and avoid unnecessary
>>> >> checkpoints because those records would have been generated. If the
>>> >> system is not idle, or to put it in other words there has been at
>>> >> least one record since the last checkpoint, we would log a standby
>>> >> snapshot, enforcing as well a checkpoint to happen the next time
>>> >> CreateCheckpoint() is gone through, and a standby snapshot is logged
>>> >> as well after the checkpoint contents are flushed. I am not sure I
>>> >> understand what you are getting at...
>>> >
>>> > Let me try to say again, suppose ControlFile->checkPoint is at
>>> > 100 and latest value of progressAt returned by GetProgressRecPtr
>>> > is 105, so the first time the above check happens, it will allow
>>> > to log standby snapshot which is okay, now assume there is no
>>> > activity, neither there is any checkpoint and again after
>>> > LOG_SNAPSHOT_INTERVAL_MS interval, when the above check
>>> > gets executed, it will pass and log the standby snapshot which is
>>> > *not* okay, because we don't want to log standby snapshot when
>>> > there is no activity. Am I missing something?
>>>
>>> Ah, right. Sorry for not getting your point. OK now I got it. So
>>> basically what the code does not is that if there is just one record after
>>> a checkpoint we would log every 15s a standby snapshot until the next
>>> checkpoint even if nothing happens, but we can save a bunch of standby
>>> records. So your point is that we need to save the result of
>>> GetProgressRecPtr() at each loop in the bgwriter when a standby snapshot is
>>> taken, say in a static XLogRecPtr called last_progress_lsn. And then at the
>>> next loop we compare it on the current result of GetProgressRecPtr(), so
>>> the condition to check if a standby snapshot should be generated or not in
>>> the bgwriter becomes that:
>>> (now >= timeout &&
>>> GetLastCheckpointRecPtr() < current_progress_ptr &&
>>> last_progress_ptr < current_progress_ptr)
>>>
>>
>> Why do you think including checkpoint related check is
>> required, how about something like below:
>>
>> (now >= timeout &&
>> last_progress_ptr < current_progress_ptr)
>>
>
> We need as well to be sure that no standby snapshots are logged just after
> a checkpoint in this code path when last_progress_ptr is referring to an
> LSN position *before* the last checkpoint LSN. There is already one
> snapshot in CreateCheckpoint() that is logged, there is no need of an extra
> one, explaining why the check on progress since last checkpoint is
> necessary to me.
>

Okay, but isn't it better that we remove the snapshot taken
at checkpoint time in the main branch or till where this code is
getting back-patched. Do you see any need of same after
having the logging of snapshot in bgwriter?

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