On Wed, Sep 16, 2026 at 11:25 AM Chao Li <[email protected]> wrote: > > > > > On Sep 10, 2026, at 06:21, Bharath Rupireddy > > <[email protected]> wrote: > > > > Hi, > > > > On Tue, Sep 8, 2026 at 9:24 PM shveta malik <[email protected]> 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
