RE: Avoid updating inactive_since for invalid replication slots

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 ?

Best Regards,
Hou zj