On Wed, Sep 9, 2026 at 3:21 PM 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.
Thank you for updating the patch! > 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 looked at the back-branch ones and I think they have a problem that the HEAD patch doesn't have. The back branches return early on subxact commit: + if (isCommit) + return; So once the subxact that acquired the slot commits, MyReplicationSlotSubId keeps the id of a subxact that is already gone. Subxact ids restart at TopSubTransactionId in every transaction since StartTransaction() resets currentSubTransactionId, so the same id comes around again.It's not a problem for the core use cases, but if there is an external SQL function that keeps the slot when the transaction ends, that stale id can match a completely unrelated subxact in a later transaction and we release a slot that subxact never acquired. What bothers me is that this pattern works today on all branches. While I guess it's not a good programming practice, we don't restrict such use cases. So I think it's not a case of not supporting that usage, it's a behavior change we would be introducing in a minor release. That makes me want to reconsider how we split the patches. IIUC the handoff mechanism that the master patch implements is to (1) keep MyReplicationSlotSubId from going stale and (2) give the slot a new guarantee, that the slot is released if an ancestor subxact aborts, which nothing does today. (2) is the part that broadens what an extension can do whereas (1) is just cleaning up after the variable we added. I think we can fix the reported problem only with (1) even without (2). So I guess it would be cleaner to do (1) for all branches, and do (2) only for master. As for (1), we can have a function like AtEOXact_ReplicationSlot() just clearing MyReplicationSlotSubId. For (2), we can prepare a separate patch that implements the handoff mechanism (possibly with a WARNING or DEBUG message) with the regression tests, if we want to support these cases. It seems confusing and I might be too pessimistic as this is all about hypothetical cases that might not exist, but I'd like to keep the back-branch fix to the smallest thing that fixes only the reported problem while not changing other current behaviors. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
