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

On Wed, Aug 19, 2026 at 11:26 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> 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?

Yes, I agree.

>
> > 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?

I think we need to carefully think about whether we drop all temp
slots at subxact abort and when we do that.

+ /*
+ * Also drop this session's temporary slots, as the top-level error
+ * handler in PostgresMain() does (keep the two in sync). Otherwise a
+ * temporary slot could be left behind holding back WAL removal and the
+ * catalog xmin after an error.

If we drop temp slots here at all, I agree it should be all of them
rather than just the held one. But I'm not us re we want to drop them
here at all.

+ *
+ * Note that this only runs when the aborting subxact held a slot. A
+ * caught error that held no slot (for example an unrelated error caught
+ * by a PL/pgSQL EXCEPTION clause) does not drop the session's temporary
+ * slots, unlike a top-level error, which always does.

I'm concerned that this is confusing from the user perspective: temp
slots are cleaned up when an error happens during slot manipulation
but not when a non-slot-related error (like PERFORM 1/0) is caught.
Further, even the former is ambiguous. If
pg_replication_slot_advance() is called with a non-existent slot name,
the function raises an error but doesn't clean up temp slots because
we don't acquire any slots. So whether the session's temp slot
survives depends on which error a slot function happens to raise,
which the user cannot predict.

+ * slots, unlike a top-level error, which always does. Covering that too
+ * would mean running the cleanup on every aborting subxact, which is
+ * harder to reason about, so it is kept here after the release.

True.

+ *
+ * Note also that we could instead keep the temporary slots and treat the
+ * error as recoverable, since the subxact was caught and the session goes
+ * on. But the error may be in the slot handling itself, leaving the slot
+ * in a doubtful state, so dropping it is the safer choice.

I don't think it could be the reason, because we leave a persistent
slot alone even when an error happens while handling it.

+ */
+ ReplicationSlotCleanup(false);

Also, is calling ReplicationSlotCleanup() (and possibly
ReplicationSlotRelease()) during sub-transaction abort really safe in
the first place? That function could raise errors and we call it out
of transactions elsewhere.

I'm inclined to think AtEOSubXact_ReplicationSlot() should only
release the slot and not call ReplicationSlotCleanup() at all. That
said, I'm not fully convinced of this either as it might be
inconsistent with top-level error cases in a sense.

Regards,

--
Masahiko Sawada

Amazon Web Services: https://aws.amazon.com