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

On Wed, Feb 10, 2016 at 4:11 PM, Amit Kapila <amit.kapila16@gmail.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.