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

Yeah, we can do a minimal fix for back-branches on the lines you are
suggesting but OTOH, I think we are over worried about the
hypothetical cases. I feel there is no harm in keeping the HEAD and
back-branches code/behavior same, in the worst case, if we get any
report, we can address keeping the actual usage in mind.

--
With Regards,
Amit Kapila.