Re: Persist slot invalidations before publishing them

On Wed, Sep 23, 2026 at 11:37 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Tue, Sep 22, 2026 at 03:46:55PM +0530, shveta malik wrote:
> > I had a look at 002 to review slotsync path,
>
> Thanks for looking at it!
>
> > + /*
> > + * A failed invalidation can still hold the slot's I/O lock. Release it
> > + * before slot cleanup acquires ReplicationSlotAllocationLock, which
> > + * checkpoints hold while acquiring slot I/O locks.
> > + */
> > + LWLockReleaseAll();
> > +
> >
> > Could it be problematic to call LWLockReleaseAll() inside a localized
> > error cleanup callback (PG_ENSURE_ERROR_CLEANUP) rather than waiting
> > for AbortTransaction or proc_exit? Since the goal is just to avoid
> > deadlock with the Checkpointer, shouldn't we explicitly release that
> > one specific lock?
> > if (MyReplicationSlot != NULL &&
> > LWLockHeldByMe(&MyReplicationSlot->io_in_progress_lock))
> > {
> > LWLockRelease(&MyReplicationSlot->io_in_progress_lock);
> > }
> >
> > I don't have an exact scenario to worry about, but it seems like
> > overkill. Thoughts?
>
> Yeah, it's probably better to be specific here.
>
> One concern with the proposed check is that all existing uses of LWLockHeldByMe()
> appear to be for assertions or debugging (as documented on top of LWLockHeldByMe()).

Oh Okay, I missed this point earlier.

> Also, releasing an LWLock after ERROR requires restoring the interrupt holdoff
> expected by LWLockRelease().

Okay.

> Another possibility would be to make ReplicationSlotPersistInvalidation() always
> leave the caller acquired I/O lock held. Slotsync could then release that specific
> lock in a PG_CATCH() block, something like:

Yes, I agree. I find this approach much better for 2 reasons:

1) The caller has better control over the lock, which makes sense
since it is the one acquiring it.
2) It makes the code much more understandable. Earlier, it took me a
while to figure out exactly where the io_in_progress_lock was getting
released, especially looking at the slotsync patch where it was
acquired right before calling ReplicationSlotPersistInvalidation().

> "
> PG_CATCH();
> {
> HOLD_INTERRUPTS();
> LWLockRelease(&slot->io_in_progress_lock);
> PG_RE_THROW();
> }
> PG_END_TRY();
>
> LWLockRelease(&slot->io_in_progress_lock);
> "
>
> This would avoid both LWLockReleaseAll() and using LWLockHeldByMe() for normal
> control flow. Does that sound preferable?

Yes.

thanks
Shveta