Hi,
> > Hi Ant, > > > > Good idea. > > +1. > > CheckPointSnapBuild() and CheckPointLogicalRewriteHeap() call > ReplicationSlotsComputeLogicalRestartLSN(), which seems to use > last_saved_restart_lsn for persistent logical slots. Since > last_saved_restart_lsn is updated by CheckPointReplicationSlots(), > if those functions run before it, their cleanup decisions are still > based on the restart_lsn saved at the previous checkpoint. > > So, isn't it better to move them after CheckPointReplicationSlots() > as well, so that logical snapshot and rewrite mapping cleanup can use > the newly saved restart_lsn? > > Of course, we'd first need to confirm that doing so is safe, though. I revisited the CheckPointSnapBuild() and CheckPointLogicalRewriteHeap(). Both functions work idempotently and are safe to move after CheckPointReplicationSlots(). They benefit from the updated last_saved_restart_lsn computed in the current checkpoint cycle, enabling more accurate cleanup decisions. I ran regression testing comparing the baseline (master) version with the patched version: - Checkpoint duration: No change. - WAL retention: More accurate, because it uses the current replication slot state. - Regressions: None detected. - Dependencies: No new dependencies introduced. Please see the v2 as attached but we can also commit the v1 since the only difference is the order of CheckPointSnapBuild and CheckPointLogicalRewriteHeap working idempotently. Regards, Demir.
v2-0001-checkpoint-replication-slots-late.patch
Description: Binary data
