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? Do you mean that the global time to take exclusive locks gets increased? For each individual WAL insert lock, yes that's the case, but that's still far cheaper to have this evaluation logic here than in ReserveXLogInsertLocation(). > - 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... -- Michael -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers