Hi Ant, 

Good idea. Moving CheckPointReplicationSlots() to the end of CheckPointGuts() 
means we can see more fresher restart_lsn. It's especially valuable for spread 
checkpoints as you described. 

While reading the code for this, I noticed that the 
ReplicationSlot.last_saved_restart_lsn is completely invisible to operators 
today.

pg_replication_slots.restart_lsn exposes slot->data.restart_lsn, the current 
in-memory value, but WAL removal for persistent slots is gated on 
last_saved_restart_lsn (the value last
durably written to disk at checkpoint), not restart_lsn. See 
ReplicationSlotsComputeRequiredLSN() at slot.c:1346–1352:

Your patch makes the it more current at cleanup time, which helps. But between 
checkpoints the gap can still be wide. Should we also expose 
last_saved_restart_lsn since it's already available and can be exposed. ? 
Otherwise, the current approach LGTM.

Reply via email to