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

On Thu, 2 Jan 2025 at 15:57, Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Thu, Jan 2, 2025 at 8:16 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Hi Nisha,
> >
> > Here are some minor review comments for patch v58-0002.
> >
>
> Thank you for your feedback! Please find the v59 patch set addressing
> all the comments.
> Note: There are no new changes in patch-0001.
>

Hi Nisha,
I reviewed the v59-0001 patch. I have few comments:

1.I think we should update the comment for function
'InvalidatePossiblyObsoleteSlot'
Currently the comment is like:

/*
* Helper for InvalidateObsoleteReplicationSlots
*
* Acquires the given slot and mark it invalid, if necessary and possible.
*
* Returns whether ReplicationSlotControlLock was released in the interim (and
* in that case we're not holding the lock at return, otherwise we are).
*
* Sets *invalidated true if the slot was invalidated. (Untouched otherwise.)
*
* This is inherently racy, because we release the LWLock
* for syscalls, so caller must restart if we return true.
*/

I think we should add a comment for the case 'when slot is already ours'.

2. Similarly we should update comment here:
/*
* Check if the slot needs to be invalidated. If it needs to be
* invalidated, and is not currently acquired, acquire it and mark it
* as having been invalidated. We do this with the spinlock held to
* avoid race conditions -- for example the restart_lsn could move
* forward, or the slot could be dropped.
*/
SpinLockAcquire(&s->mutex);

Before we release the lock, we are marking the slot as invalidated for
the case when the slot is already acquired by our process. So we
should update it in comment.

3. I think we should also update the following 'if condition':

if (active_pid != 0)
{
/*
* Prepare the sleep on the slot's condition variable before
* releasing the lock, to close a possible race condition if the
* slot is released before the sleep below.
*/
We should not enter the if condition for the case when the slot was
already acquired by our process.

Thanks and Regards,
Shlok Kyal