Re: Avoid updating inactive_since for invalid replication slots

On Tue, Feb 4, 2025 at 9:33 AM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Monday, February 3, 2025 8:03 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
> >
> > Hi Hackers,
> > (CC people involved in the earlier discussion)
> >
> > Right now, it is possible for the 'inactive_since' value of an invalid replication
> > slot to be updated multiple times, which is unexpected behavior.
> > As suggested in the ongoing thread [1], this patch introduces a new dedicated
> > function to update the inactive_since for a given replication slot, and ensures
> > that inactive_since is not updated for an invalid replication slot.
> >
> >
> > The v1 patch is attached. Any feedback would be appreciated.
>
> Thanks for sharing the patch, and I agree we should avoid updating
> inactive_since for invalid slots.
>
> But I have one question for the code:
>
> +/*
> + * Set slot's inactive_since property unless it was previously invalidated.
> + */
> +static inline void
> +ReplicationSlotSetInactiveSince(ReplicationSlot *s, TimestampTz now,
> + bool acquire_lock)
> +{
> + if (s->data.invalidated != RS_INVAL_NONE)
> + return;
>
> I am wondering is it safe to access the 'invalidated' flag without taking spinlock ?
> Strictly speaking, I think it's only safe to access slot property without lock
> when the slot is acquiring by the current process. So, if it's safe here,
> could you please add some comments to clarify the same ?

Agree, the invalidated flag check should be under the spinlock when
the process doesn't own the slot.
Here is the v2 patch with above change and other comments from [1] and
[2] incorporated.

--
Thanks,
Nisha