Re: Persist slot invalidations before publishing them

On Tue, Sep 22, 2026 at 1:58 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi JoongHyuk,
>
> On Sun, Sep 13, 2026 at 07:31:57PM +0900, JoongHyuk Shin wrote:
> > Hi Bertrand,
> >
> > I read v2 and have a question about the window the new ordering opens
> > between two invalidators.
>
> Thanks for looking at it!
>
> > If a second invalidator reaches the
> > same slot in that window (say a restartpoint enforcing
> > max_slot_wal_keep_size while startup is replaying a wal_level change, or
> > the other way round), it seems it would take the "slot is in use" path,
> > so startup might send a recovery conflict to active_pid, or another
> > process a SIGTERM. What do you think?
>
> You're right, v2 could treat the first invalidator as a regular slot user and
> terminate it.
>
> Rui, thanks for the patch! I've incorporated its locking approach and test coverage
> in v3, with some adjustments around error cleanup.
>
> Please find v3 attached.
>

I had a look at 002 to review slotsync path, I had one concern:

+ /*
+ * 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?

thanks
Shveta