Re: Introduce XID age and inactive timeout based replication slot invalidation

On Thu, Feb 6, 2025 at 10:17 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Feb 6, 2025 at 8:02 AM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
> >
> > On Wed, Feb 5, 2025 at 2:42 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > Would it address your concern if we write the actual idle duration
> > > (now - inactive_since) instead of directly using inactive_since in the
> > > above message?
> > >
> >
> > Simply using the raw timestamp difference (now - inactive_since) would
> > look odd. We should convert it into a user-friendly format. Since
> > idle_replication_slot_timeout is in minutes, we can express the
> > difference in minutes and seconds in the log.
> > For example:
> > DETAIL: The slot's idle time of 1 minute and 7 seconds exceeds the
> > configured "idle_replication_slot_timeout" duration.
> >
>
> This is better but the implementation should be done on the caller
> side mainly because we don't want to call a new GetCurrentTimestamp()
> in ReportSlotInvalidation.
>

Done.

> >
> > > 2.
> > > + * Flush all replication slots to disk. Also, invalidate obsolete slots during
> > > + * non-shutdown checkpoint.
> > > *
> > > * It is convenient to flush dirty replication slots at the time of checkpoint.
> > > * Additionally, in case of a shutdown checkpoint, we also identify the slots
> > > @@ -1924,6 +2007,45 @@ CheckPointReplicationSlots(bool is_shutdown)
> > >
> > > Can we try and see how the patch looks if we try to invalidate the
> > > slot due to idle time at the same time when we are trying to
> > > invalidate due to WAL?
> > >
> >
> > I'll consider the suggested change in the next version.
> >
>

Done the changes as suggested in v71.

> FYI, we discussed this previously (1), but the conclusion that it
> won't help much (as it will not help to remove WAL immediately) is
> incorrect, especially if we do what is suggested now.
>
> Apart from this, I have made minor changes in the comments. Please
> review and include them unless you disagree.
>

Done.
~~~~
Here are the v71 patches with the above comments incorporated.

--
Thanks,
Nisha