On Wed, Feb 10, 2016 at 12:41 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > On Wed, Feb 10, 2016 at 7:17 AM, Michael Paquier < michael.paqu...@gmail.com> > wrote: >> >> On Tue, Feb 9, 2016 at 10:42 PM, Amit Kapila wrote: >> > On Tue, Feb 9, 2016 at 6:08 PM, Michael Paquier wrote: >> >> Well, the idea is to improve the system responsiveness. Imagine that >> >> the call to GetProgressRecPtr() is done within the exclusive lock >> >> portion, but that while scanning the WAL insert lock entries another >> >> backend is waiting to take a lock on them, and this backend is trying >> >> to insert the first record that updates the progress LSN since the >> >> last checkpoint. In this case the checkpoint would be skipped. >> >> However, imagine that such a record is able to get through it while >> >> scanning the progress values in the WAL insert locks, in which case a >> >> checkpoint would be generated. >> > >> > Such case was not covered without your patch and I don't see the >> > need of same especially at the cost of taking locks. >> >> In this code path that's quite cheap, and it clearly improves the >> decision-making when wal_level >= hs which is now rather broken (say >> not optimized much). >> >> >> In any case, I think that we should try >> >> to get exclusive lock areas as small as possible if we have ways to do >> >> so. >> >> >> > >> > Sure, but not when you are already going to take lock for longer >> > duration. >> >> Why would an exclusive lock be taken for a longer duration in the >> checkpoint portion? > > Consider below code: > > /* > + * Get progress before acquiring insert locks to shorten the locked > + * section waiting ahead. > + */ > + progress_lsn = GetProgressRecPtr(); > + > + /* > * We must block concurrent insertions while examining insert state to > * determine the checkpoint REDO pointer. > */ > WALInsertLockAcquireExclusive(); > > In GetProgressRecPtr(), first it take WALInsertLocks one-by-one to > retrieve the latest value of progressAt and then again it will take > all the WALInsertLocks in WALInsertLockAcquireExclusive() and then > do some work. Now I think it is okay to retrieve the latest of progressAt > after WALInsertLockAcquireExclusive() as you don't need to take the > locks again.
Sure, that would be OK. Now I am not on board if this means to have the checkpoint take an exclusive locks for a bit longer duration if that's not actually necessary. >> > - 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) That's your point? -- Michael
