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

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

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/