On Tue, Feb 4, 2025 at 9:33 AM Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
>
> On Monday, February 3, 2025 8:03 PM Nisha Moond <nisha.moond...@gmail.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
v2-0001-Avoid-updating-inactive_since-for-invalid-replica.patch
Description: Binary data
