Re: Fix race in ReplicationSlotRelease for ephemeral slots

On Tue, Jun 16, 2026 at 6:32 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Tuesday, June 16, 2026 5:36 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Jun 16, 2026 at 2:24 PM Zhijie Hou (Fujitsu) <houzj(dot)fnst(at)fujitsu(dot)com>
> > wrote:
> > >
> > >
> > > I have one minor comment for the 0001 patch.
> > >
> > > + NameData slot_name = {0};
> > > ...
> > > SpinLockAcquire(&local_slot->mutex);
> > > synced_slot = local_slot->in_use &&
> > > local_slot->data.synced;
> > > + if (synced_slot)
> > > + slot_name = local_slot->data.name;
> > > SpinLockRelease(&local_slot->mutex);
> > >
> > > We can defer assigning slot_name until after we pass the existing
> > > (synced_slot) check. Since it's a synced slot, no other process can
> > > change it at that point, and we can also skip initializing slot_name.
> > > (Please refer to the attached patch for suggested changes)
> > >

This seems ok to me. The only risk is that people who don't know the
assumption might think this getter is unsafe.

> > + if (dropped)
> > + ereport(LOG,
> > + errmsg("dropped replication slot \"%s\" of database with OID %u",
> > + NameStr(slot_name),
> > + slot_database));
> >
> > Can we avoid the if (dropped) check by placing this LOG message immediately
> > after dropping the slot under synced slot check?
>
> I think we can do that.

+1. Placing the log message within the lock window looks OK to me
since the cost is small.

> I'm attaching the new patches for all supported
> branches, incorporating both my and Amit's comments. I hope this helps move the
> fix forward.
>
> I also confirmed that the fix works on all supported branches.

Thanks for your preparation for patches and confirmation of them!

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.