Re: [PATCH] Release replication slot on error in SQL-callable slot functions

Hi,

On Fri, Aug 28, 2026 at 3:49 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Thu, Aug 27, 2026 at 9:19 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Thu, Aug 27, 2026 at 6:00 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Aug 27, 2026 at 11:58 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > > > On Mon, Aug 24, 2026 at 3:29 PM Bharath Rupireddy
> > > > <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> > > > >
> > > > >
> > > > > In short, having just the slot release in the subxact path gives the
> > > > > same error behavior, is simple to reason about, and fixes the crash
> > > > > reported in this thread.
> > > >
> > > > One thing I'm a bit concerned about is that this would be the first
> > > > caller to invoke ReplicationSlotRelease() from inside the transaction
> > > > machinery.
> > > >
> > >
> > > True, but OTOH, won't we already clean up resources not directly
> > > associated with subxact in AtEOSubXact_LargeObject() or
> > > AtEOSubXact_Files()? I don't see any problem as far as the current
> > > pattern of usage for slots.
> >
> > I agree.
> >
> > > The new restriction this patch will add is
> > > "a slot acquired in a subxact does not survive that subxact being
> > > unwound." which should be okay because of its similarity with
> > > top-level xact behavior. I feel if possible we should restrict such
> > > usage explicitly in code in some way rather than one finding out this
> > > as a surprise.
> > >
> > > *
> > > An error raised and caught in a
> > > + subtransaction, for example by a
> > > + <application>PL/pgSQL</application> exception block, does not drop
> > > + them.
> > >
> > > Based on above, something like below won't clean up temp slots and end
> > > up holding xmin.
> > > DO $$ BEGIN
> > > PERFORM pg_create_logical_replication_slot('s', 'nonexistent_plugin', true);
> > > EXCEPTION WHEN OTHERS THEN RAISE NOTICE '%', SQLERRM;
> > > END $$;
> >
> > Well, on rethinking, I feel that if we encounter an error while
> > creating a slot, whether persistent or temporary, the slot should be
> > dropped right there.
> >
> > This already works correctly for persistent slots: by the time the
> > slot reaches ReplicationSlotRelease, it is still in RS_EPHEMERAL state
> > and is therefore dropped by release. OTIOH, a temporary slot is left
> > behind. I think the temporary slot should also be dropped because the
> > caller never received a reference to it. I don't see a legitimate use
> > case where a temp slot should survive specifically because its
> > creation call failed.
>
> While I agree that it would be an ideal behavior and the analysis
> holds for logical slots, I want to note that persistent physical
> replication slots are created with RS_PERSISTENT so if an error
> happens during the slot creation the slot is left behind. Also,
> logical persistent slots actually have the same gap: if
> ReplicationSlotPersist() raises an error it leaves a persistent slot
> behind as well. Given that slot creation and drop are not
> transactional operations, and that leaving a slot behind on a failure
> is not a new behavior, I'm inclined toward only releasing the slot at
> the subxact abort. We can discuss the better behavior on HEAD
> separately.

Yes, I realised the same. The ephemeral state only applies to logical
slots, not to physical slots or temporary slots.

I agree to keep the back-branch fix simple and solve the slot leak and
crash reported in this thread. However, I think the creation failure
on temporary slots inside a subxact also needs to be fixed in the back
branches (perhaps separately), because one can hit the issue with
direct SQL. A temporary slot whose creation fails needs to be dropped,
to avoid leaking resources for a slot the caller never got a reference
to.

In the replication slot subxact callback, on the abort path, we need
to know whether the slot's creation failed. Ephemeral slots already
handle that, but only for persistent logical slots. A temporary slot
stays RS_TEMPORARY throughout. So there are a few ways to solve this:

1/ Also mark temporary slots as ephemeral initially and transition
them to RS_TEMPORARY once creation succeeds. A quick check shows this
needs changes in many places.
2/ Introduce a new state to represent a temporary slot still in
creation (RS_TEMPORARY_EPHEMERAL or such).
3/ Use a boolean in the ReplicationSlot structure
(is_create_in_progress or such), and in the subxact callback, when the
slot is temporary and is_create_in_progress is set, drop just that
temporary slot and leave the others alone.

I prefer option 3, to keep it simple without adding a new state, and
because it is back-branch friendly. The new boolean lives only in
memory and is not written to disk. To drop a single temporary slot,
I'm thinking of moving the single-slot drop code out of
ReplicationSlotCleanup() into an internal helper function.

Thoughts?

On Thu, Aug 27, 2026 at 5:30 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> True, but OTOH, won't we already clean up resources not directly
> associated with subxact in AtEOSubXact_LargeObject() or
> AtEOSubXact_Files()? I don't see any problem as far as the current
> pattern of usage for slots. The new restriction this patch will add is
> "a slot acquired in a subxact does not survive that subxact being
> unwound." which should be okay because of its similarity with
> top-level xact behavior. I feel if possible we should restrict such
> usage explicitly in code in some way rather than one finding out this
> as a surprise.

Hi Amit, By restricting in the code, does that mean adding an Assert,
or a WARNING, or a WARNING plus slot release (not an error), in the
replication slot subxact callback on the commit path, instead of
handing the slot off to the parent across the subtransaction boundary?

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com