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

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. 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