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