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

On Thu, 16 Jan 2025 at 12:35, Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> On Wed, Jan 15, 2025 at 11:37 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Thu, 2 Jan 2025 at 15:57, Nisha Moond <nisha.moond412@gmail.com> wrote:
> > >
> > > On Thu, Jan 2, 2025 at 8:16 AM Peter Smith <smithpb2250@gmail.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
>
Hi Nisha,

Thanks for providing an updated patch. I have tested the patch and ran
some tests. The patch works fine. I have few comments:

v60-0001 patch:
1) There is extra line before 'SpinLockRelease(&s->mutex)' :

+       else                    /* Some other process owns the slot */
        {
+
+           SpinLockRelease(&s->mutex);

v60-0002 patch:
1) In the comment:

/*
 * Invalidate slots that require resources about to be removed.
 *
 * Returns true when any slot have got invalidated.
 *
 * Whether a slot needs to be invalidated depends on the cause. A slot is
 * removed if it:
 * - RS_INVAL_WAL_REMOVED: requires a LSN older than the given segment
 * - RS_INVAL_HORIZON: requires a snapshot <= the given horizon in the given
 *   db; dboid may be InvalidOid for shared relations
 * - RS_INVAL_WAL_LEVEL: is logical
 * - RS_INVAL_IDLE_TIMEOUT: idle slot timeout has occurred
 *
 * NB - this runs as part of checkpoint, so avoid raising errors if possible.
 */

It is mentioned that 'A slot is removed if it:'. I think instead of
'removed' it should be 'invalidated'.

Thanks and regards,
Shlok Kyal