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

Hi,

On Tue, Sep 15, 2026 at 5:13 PM 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.

Thanks for taking a look at it. Here is my thinking on this.

Within a transaction, subtransaction ids are not reset or reused, so
MyReplicationSlotSubId isn't stale. It is the id of the subtransaction
that owns the slot (ownership stays with the subtransaction that
acquired it until the slot is released, and the id gets reset at that
point). And if the owning subtransaction aborts, the slot is released,
which is what protects against the issue reported in this thread
(leftover slot or assertion failure).

Without the handoff on commit, the slot stops being protected at the
first subtransaction boundary. If the owning subtransaction commits
while still holding the slot, whether it holds the slot intentionally
or unknowingly forgets to release the slot, MyReplicationSlotSubId
keeps the id of an owner that is gone, so no later subtransaction in
the transaction can match it. If the parent subtransaction then
aborts, it is not the owner, so the slot is not released even though
the work it was acquired for is being rolled back, and that brings
back the issue reported in this thread.

With the handoff on commit, the parent becomes the owner. If the
parent aborts, it is now the owner, so the slot is released, and the
protection against the issue reported in this thread still holds. If
the parent commits, ownership moves up again, and it eventually
reaches TopSubTransactionId, which is the top-level transaction's own
id and is never assigned to a subtransaction, so no owner id is left
behind that could match an unrelated subtransaction later.

Therefore, handing the slot off to the parent on commit looks correct
to me on all branches, even when the slot is carried across a
subtransaction boundary, because ownership moves to a subtransaction
that is still in progress and the slot stays protected from the issue
reported in this thread for as long as it is held. And I am okay with
not emitting any warning on any branch, including HEAD. For the
legitimate usage, a function that intentionally holds the slot across
subtransaction boundaries, the warning would be reporting correct code
as a problem, and that is as wrong on HEAD as it is in the back
branches. With no warning on any branch, AtEOSubXact_ReplicationSlot()
ends up identical on all branches, so I don't think we need to split
this into a back-branch patch and a separate HEAD patch.

On adding AtEOXact_ReplicationSlot(), I don't think it is needed for
correctness, for the reason above, but I have no objection to adding
it on HEAD, similar to what AtEOXact_LargeObject() and
AtEOXact_Files() do today.

I previously tested the handoff in the commit path, by making
pg_replication_slot_advance() return while still holding the slot and
running it through nested PL/pgSQL exception blocks:
https://postgr.es/m/CALj2ACUD_K5zBgXD3ebYmhmouJx91fq+aiLeD8HSuC6xnYvj3g@mail.gmail.com.
If we want to add this test, I think I can add an injection point that
returns before the slot release (similar to the skip-log-running-xacts
test), and add the test on all the possible back branches.

Thoughts?

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com