Re: [PATCH] Release replication slot on error in SQL-callable slot functions - Mailing list pgsql-hackers

> On Sep 16, 2026, at 14:34, shveta malik <shveta.malik@gmail.com> wrote:
>
> On Wed, Sep 16, 2026 at 11:25 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>>
>>
>>> On Sep 10, 2026, at 06:21, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>>>
>>> Hi,
>>>
>>> On Tue, Sep 8, 2026 at 9:24 PM shveta malik <shveta.malik@gmail.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
iscaught. It remains until the session terminates, or it’s dropped explicitly. For a long-lived or pooled session, its
restart_lsncontinues 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
whenthe 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/