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

On Thu, Sep 17, 2026 at 2:32 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Sep 16, 2026 at 2:07 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Wed, Sep 16, 2026 at 5:43 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Sep 9, 2026 at 3:21 PM 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.
> > >
> > > 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