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

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?

> 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