Date: Wed, 23 Sep 2026 06:06:58 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: shveta malik <shveta.malik@gmail.com>
Cc: JoongHyuk Shin <sjh910805@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Rui Zhao <zhaorui126@gmail.com>, pgsql-hackers@lists.postgresql.org
Subject: Re: Persist slot invalidations before publishing them
Message-ID: <arNsgodAHkkxbxvX@bdtpg>
References: <ao7u5I9OeIR72kGp@bdtpg>
 <CAA4eK1LH_yjAwcn87gxdawJn+VsAt5HSjXdfAA7gBB-Gb9BGaA@mail.gmail.com>
 <apGN1gY3ATFuxd0C@bdtpg>
 <CACSdjfOa0c1dqXZQhCBD+72tq6sOccfk1qs3SkYz+8Hgn=My5g@mail.gmail.com>
 <arI8AEPtq4EtgzWL@bdtpg>
 <CAJpy0uDAyMbD61iJE-2qRMCtdQ4Qtuq+hy=mqDszFapgCHXaJQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAJpy0uDAyMbD61iJE-2qRMCtdQ4Qtuq+hy=mqDszFapgCHXaJQ@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/arNsgodAHkkxbxvX%40bdtpg>
Precedence: bulk

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

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

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:

"
  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?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com