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

On Sat, Sep 5, 2026 at 2:45 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Thu, Sep 3, 2026 at 1:58 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > Thank you for updating the patch! Here are some review comments:
> >
> > + if (isCommit)
> > + {
> > + Assert(MyReplicationSlot != NULL);
> > + ereport(WARNING,
> > + (errcode(ERRCODE_WARNING),
> > + errmsg("subtransaction left replication slot \"%s\" acquired",
> > + NameStr(MyReplicationSlot->data.name)),
> > + errhint("Check for missing \"ReplicationSlotRelease\"
> > calls.")));
> >
> > The hint message "Check for missing ReplicationSlotRelease call" seems
> > to be for us (PostgreSQL hackers) but not users. I think such messages
> > should be left as a comment instead of in errhint.
>
> Agreed. Will fix it.
>
> > Also, we don't prohibit external extensions or functions to commit a
> > subtransaction while holding a replication slot. If there are such
> > extensions, users would get WARNING messages. Which seems to be
> > something I'd like to avoid in minor releases.
>
> Thanks Sawada-san for the off-list discussion on this point. Here is a
> summary for others' input.
>
> 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));

thanks
Shveta