On Thu, Sep 17, 2026 at 2:32 AM Masahiko Sawada <[email protected]> wrote: > > On Wed, Sep 16, 2026 at 2:07 AM shveta malik <[email protected]> wrote: > > > > On Wed, Sep 16, 2026 at 5:43 AM Masahiko Sawada <[email protected]> > > wrote: > > > > > > 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. > > > > Sawada-san, does that mean that on the back branches, even for the > > case where the concerned subtransaction is committing while the slot > > is still held (a scenario we don't know can happen), we would release > > the slot and clean up MyReplicationSlotSubId? Is my understanding > > correct? > > I don't think we should release the slot at subxact commit.
I agree. I was a bit surprised by what I understood, so I wanted to confirm. > I think > it's better to leave it to the caller as it might release the slot > afterward. Another problem is that nothing tests this case. > Right. I agree. > Please refer to the attached patch that can be applied on v16 patch > and implements my idea. It adds additional regression tests too. The changes looks good. > Regards, > > -- > Masahiko Sawada > Amazon Web Services: https://aws.amazon.com
