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.
