On Wed, Jan 15, 2025 at 11:37 AM Shlok Kyal <shlok.kyal....@gmail.com> wrote: > > On Thu, 2 Jan 2025 at 15:57, Nisha Moond <nisha.moond...@gmail.com> wrote: > > > > On Thu, Jan 2, 2025 at 8:16 AM Peter Smith <smithpb2...@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
--
Thanks,
Nisha
v60-0001-Enhance-replication-slot-error-handling-slot-inv.patch
Description: Binary data
v60-0002-Introduce-inactive_timeout-based-replication-slo.patch
Description: Binary data
