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

Hi,

On Wed, Aug 19, 2026 at 4:55 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> Thank you for updating the patch!
>
> I reviewed the v12 patch and here are some review comments:

Thanks for reviewing it.

> +static SubTransactionId acquiredInSubId = InvalidSubTransactionId;
>
> I'm not sure this variable name is ideal, since "acquired..." can be
> read as a boolean. How about something like MyReplicationSlotSubid or
> slotAcquireSubid?

MyReplicationSlotSubId looks better, so I used that.

> +/*
> + * Release the replication slot at subxact end if it was acquired here.
> + *
> + * handled. The subxact id is used rather than a nesting level because levels
> + * are reused across subxacts while ids are not.
> + */
>
> I don't think it's the right place to explain the bug in detail, and
> mentioning AtEOSubXact_LargeObject() seems unnecessary. How about
> rewriting it to something like:
>
> /*
> * At subxact end, hand off or release MyReplicationSlot if it was acquired
> * in this subxact. On commit, ownership passes to the parent subxact; on
> * abort, the slot is released (a dnthe sessions' temp slots dropped).
> */

WFM. Used the above comment.

> + /*
> + * The aborting subxact is the one that acquired the slot, so the slot is
> + * still held and must be released. acquiredInSubId is set only when a
> + * slot is held and cleared when it is released, so a matching subxact id
> + * means the slot is ours.
> + */
> + ReplicationSlotRelease();
>
> We should add an assertion that MyReplicationSlot is not NULL before this call.

The slot release function already has an assertion. We discussed this
upthread and agreed on the comment wording and not to have an
additional assertion here. Does that work for you?

> AtEOSubXact_ReplicationSlot() performs the same slot cleanup (release
> + drop temporary slots) that the error path in PostgresMain() does. It
> would be good to add a note around the
> ReplicationSlotRelease()/ReplicationSlotCleanup() calls in postgres.c
> so that any future change there is also considered for
> AtEOSubXact_ReplicationSlot() (and vice versa).

Sounds good. Reworded these comments.

Please find the attached v13 patch. I verified that the same issue
exists all the way back to PG14. I want to backport it with the
reproducers, since it is a bug that can be reproduced with simple SQL
queries and can cause crashes, slot leaks and vacuum issues. If v13
looks good, I can prepare patches for back branches and send them.
Thoughts?

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