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

On Sat, Aug 29, 2026 at 4:42 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> 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.

Do you mean one "cannot" 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.

My preference will be option 1 but it needs careful checking for
existing usages.

> 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