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

On Tue, Sep 15, 2026 at 11:40 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
> > On Sep 16, 2026, at 14:34, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Wed, Sep 16, 2026 at 11:25 AM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> >>
> >>
> >>
> >>> On Sep 10, 2026, at 06:21, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On Tue, Sep 8, 2026 at 9:24 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >>>>
> >>>>> There can be two cases for external modules implementing logical
> >>>>> decoding functionality. A function that unknowingly forgets to call
> >>>>> ReplicationSlotRelease(), and a function that intentionally holds the
> >>>>> slot across subxact boundaries and releases it later in the top-level
> >>>>> transaction. For example
> >>>>>
> >>>>> ```
> >>>>> BeginInternalSubTransaction("xxx");
> >>>>> ReplicationSlotAcquire(name, ...);
> >>>>> <do something>
> >>>>> ReleaseCurrentSubTransaction();
> >>>>> <do more something>
> >>>>> ReplicationSlotRelease();
> >>>>> ```
> >>>>>
> >>>>> The above seems like a legitimate usage (though we don't know if there
> >>>>> is any real user of this pattern today). We can't easily distinguish
> >>>>> between the two cases in the subxact commit path. The first case is
> >>>>> more of a coding and reviewing problem. In both cases, calling the
> >>>>> function twice in a row would hit Assert(MyReplicationSlot == NULL) or
> >>>>> silently overwrite the slot, but the intentional case must already be
> >>>>> aware of this. Even if the core emits a WARNING and users report it,
> >>>>> there may not be anything we can do about it. If they release the slot
> >>>>> at the end of the function, it is not a problem. If they forget, they
> >>>>> need to fix it themselves.
> >>>>>
> >>>>> Given all this, emitting a WARNING on a subxact commit may not seem
> >>>>> right even on HEAD. Silently handing off the slot to the parent
> >>>>> transaction on subxact commit seems like the better approach.
> >>>>>
> >>>>
> >>>> I agree there could be such a scenario in the future, especially since
> >>>> we don't document or define a rule that a slot must be released in the
> >>>> same subtransaction where it was acquired. Even if no existing user
> >>>> exposed slot-function does this today, an extension could.
> >>>>
> >>>> But I feel there should be at least some way to signal that there's a
> >>>> chance of a slot leak, for the cases where it actually is one. How
> >>>> about putting in a DEBUG message noting that the slot was retained
> >>>> across a subxact boundary? Something like:
> >>>>
> >>>> elog(DEBUG1,
> >>>> "replication slot \"%s\" acquired in subtransaction retained
> >>>> across its commit; ownership transferred to parent",
> >>>> NameStr(MyReplicationSlot->data.name));
> >>>
> >>> Upon thinking more and discussing off-list with Amit and Sawada-san,
> >>> here is what I have. In the PG20+ branches, I added a WARNING and
> >>> removed the assert while handing off the slot across subtransaction
> >>> boundaries during commits. We do not know if there are any such
> >>> legitimate uses, but if there are, those users would get the WARNING
> >>> reported. On HEAD it is easier to remove the WARNING later if it feels
> >>> annoying for such users. In the backbranches,
> >>> AtEOSubXact_ReplicationSlot() is a no-op for commits because the
> >>> WARNING may not be a good idea there, and we do not have a good use
> >>> case for it on commits anyway. Hope this simplifies the fix.
> >>>
> >>> I used similar wording to the above for the WARNING.
> >>>
> >>> Please find the attached v16 patches prepared for all the supported branches.
> >>>
> >>> --
> >>> Bharath Rupireddy
> >>> Amazon Web Services: https://aws.amazon.com
> >>> <v16-0001-Fix-replication-slot-leak-on-error-caught-in-a-s.patch><nocfbot-v16-0001-PG19-Fix-replication-slot-leak-on-error-caught-in-a-s.patch><nocfbot-v16-0001-PG18-Fix-replication-slot-leak-on-error-caught-in-a-s.patch><nocfbot-v16-0001-PG17-Fix-replication-slot-leak-on-error-caught-in-a-s.patch><nocfbot-v16-0001-PG16-Fix-replication-slot-leak-on-error-caught-in-a-s.patch><nocfbot-v16-0001-PG15-Fix-replication-slot-leak-on-error-caught-in-a-s.patch><nocfbot-v16-0001-PG14-Fix-replication-slot-leak-on-error-caught-in-a-s.patch>
> >>
> >> I just reviewed v16 and have one concern.
> >>
> >> The comment explicitly says that temporary slots are left in place. For already-created temporary slots, that sounds reasonable. But what if the creation of a temporary slot fails within the subtransaction? For example:
> >> ```
> >> evantest=# DO $$
> >> evantest$# BEGIN
> >> evantest$# PERFORM pg_create_logical_replication_slot(
> >> evantest$# 'tmp_bad',
> >> evantest$# 'definitely_not_allowed',
> >> evantest$# true
> >> evantest$# );
> >> evantest$# EXCEPTION WHEN OTHERS THEN
> >> evantest$# RAISE NOTICE 'caught SQLSTATE %', SQLSTATE;
> >> evantest$# END
> >> evantest$# $$;
> >> NOTICE: caught SQLSTATE 42501
> >> DO
> >> evantest=#
> >> evantest=# SELECT slot_name,
> >> evantest-# plugin,
> >> evantest-# temporary,
> >> evantest-# active,
> >> evantest-# active_pid,
> >> evantest-# restart_lsn,
> >> evantest-# confirmed_flush_lsn,
> >> evantest-# catalog_xmin
> >> evantest-# FROM pg_replication_slots
> >> evantest-# WHERE slot_name = 'tmp_bad';
> >> slot_name | plugin | temporary | active | active_pid | restart_lsn | confirmed_flush_lsn | catalog_xmin
> >> -----------+------------------------+-----------+--------+------------+-------------+---------------------+--------------
> >> tmp_bad | definitely_not_allowed | t | t | 9668 | 0/01BFA5A8 | | 665
> >> (1 row)
> >> ```
> >>
> >> With a bad plugin, creation of the temporary slot fails, but the partially initialized slot remains after the error is caught. It remains until the session terminates, or it’s dropped explicitly. For a long-lived or pooled session, its restart_lsn continues to participate in ReplicationSlotsComputeRequiredLSN(), potentially causing unnecessary WAL retention.
> >>
> >> Therefore, should we distinguish a successfully created temporary slot from one whose creation is still in progress when the sub-transaction aborts, and drop the latter?
> >
> > We had discussed this already, please see the email at [1] and the
> > responses to it. Since this issue is not new (it exists for other
> > slots too), it was decided to consider it separately on HEAD.
> >
> > [1]: https://www.postgresql.org/message-id/CAJpy0uAwKM%3DLbnNp0rMevtCD9ub8zcADE9X1Z-PLwTmqFadgCQ%40mail.gmail.com
> >
> > Thanks
> > Shveta
>
> Thanks for the explanation. Then would it make sense to add a brief description for that in the commit message?

Yes, I'll add it to the commit message.

I stated we should deal with the issue only to master that a slot is
left behind in an incomplete state if an error happens during the slot
creation and is caught, but I think I underestimated the issue. I'd
like to summarize this remaining issue and some potential solutions in
addition to the current v19 patch.

As Amit and Chao Li reported upthread, if it happens, the slot is left
behind while holding catalog_xmin. Without the v19 patch,
only the backend who created the slot can drop it (non-assertion
builds), or the slot drop hits assertion failure (on assertion
builds), whether the logical slot is temp or persistent. With the
patch, the persistent logical slot is dropped on the slot release
whereas users would still need to drop the incomplete slot manually.
Such slots are left behind while having confirmed_flush_lsn being
NULL.

If users use such a slot for logical decoding, it can decode partial
transactions. Since confirmed_flush is invalid,
CreateDecodingContext() starts with an invalid start_decoding_at, and
as it's not in slot creation, the snapshot builder can restore a
serialized snapshot and jump to the consistent state in the middle of
a transaction that started before the slot's restart_lsn.

An ideal behavior would be that we drop such a slot, whether the slot
is temp or not, as Shveta mentioned[1]. Bharath proposed three
approaches upthread[2]:

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

Some hackers including me prefer option 1 but we agreed it should be
only master as it's impactful.

I think neither 2 nor 3 are backpatch-safe. For 2, persistence is
written to disk, and every place checking RS_TEMPORARY, including
extensions, would need to decide whether the new state counts as
temporary. Option 3 in particular would break extensions that iterate
over the ReplicationSlotCtl->replication_slots[] array.

Considering backpatch-able changes, there are two ideas:

A. At subxact abort, if the slot is a temporary logical slot and its
confirmed_flush is invalid, we drop it. The slot drop should make sure
to unset PROC_IN_LOGICAL_DECODING flag as ReplicationSlotRelease()
does.

B. Add a check to CreateDecodingContext() that raises an error if the
slot's confirmed_flush is invalid. On v17 and later, we need to
exclude slot synchronization, since update_local_synced_slot()
advances a newly created synced slot via
LogicalSlotAdvanceAndCheckSnapState() before its confirmed_flush is
set.

I prefer B as it's safer for me. Whatever approach we pick, I think we
can fix the incomplete slot problem separately from the current
discussed patch.

[1] https://www.postgresql.org/message-id/CAJpy0uAwKM%3DLbnNp0rMevtCD9ub8zcADE9X1Z-PLwTmqFadgCQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CALj2ACVjdXfSqTWUam8Db_tja_F%3DQj3J9O-bajCp0HabUts6sw%40mail.gmail.com

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com