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

On Wed, Jan 15, 2025 at 11:37 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> 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'.

Done.

> 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.
>

Clarified the comments as per the mentioned case.

> 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.
>

Thank you for pointing that out. I've included the fix and also
reorganized this section of the code in patch-0001 to improve the
readability of the logic.

Attached the v60 patch set addressing above comments and all other
comments at [1].

[1] https://www.postgresql.org/message-id/CALDaNm2r969ZZPDaAZQEtxcfL-sGUW8AGdbdwC8AcMn1V8w%2Bhw%40mail.gmail.com

--
Thanks,
Nisha