On Mon, Jan 13, 2025 at 12:22 PM vignesh C <vignes...@gmail.com> wrote:
>
> On Thu, 2 Jan 2025 at 15:57, Nisha Moond <nisha.moond...@gmail.com> wrote:
> >
> > Thank you for your feedback! Please find the v59 patch set addressing
> > all the comments.
> > Note: There are no new changes in patch-0001.
>
> Few comments:
> 1) I felt we should not invalidate the slots for which has no
> effective xmin set as they will not be holding the WAL files from
> deletion. This can happen when user created slots with
> immediately_reserve as false and lsn will be actually reserved only
> after the first connection from a streaming replication client:
> +static inline bool
> +CanInvalidateIdleSlot(ReplicationSlot *s)
> +{
> + return (idle_replication_slot_timeout_mins > 0 &&
> + s->inactive_since > 0 &&
> + !(RecoveryInProgress() && s->data.synced));
> +}
>
IIUC, for both logical and physical replication slots, the effective
xmin remains NULL until the respective nodes establish their first
connection. However, logical slots always reserve WAL immediately,
whereas physical slots do not unless immediately_reserve=true is set.
To avoid unnecessary slot invalidation for slots that are not
reserving WAL when created, it might be better to check the
restart_lsn instead of effective_xmin. If restart_lsn is invalid for a
slot, it indicates that WAL is not reserved for the slot, and we can
safely skip invalidation due to idle_timeout for such slots. This
logic has been implemented in v60.
--
Thanks,
Nisha
