On Wed, Feb 10, 2016 at 1:01 PM, Michael Paquier <michael.paqu...@gmail.com> wrote:
> > > On Wed, Feb 10, 2016 at 4:11 PM, Amit Kapila <amit.kapil...@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. > 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