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

On Thu, Aug 6, 2026 at 1:32 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Wed, Aug 5, 2026 at 8:46 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > Thanks Bharath. A few trivial comments:
> >
> > 1)
> > + * We must not get here while decoding is running. Decoding starts and
> > + * aborts an internal (sub)transaction while holding the slot, for each
> > + * decoded transaction (ReorderBufferProcessTXN()) and when executing
> > + * invalidations (ReorderBufferImmediateInvalidation()), but that always
> > + * happens below the acquiring subxact, so those aborts have a different
> > + * (deeper) id and do not match here. Decoding also runs with a historic
> > + * snapshot set up, so assert that it is not.
> >
> > It is slightly difficult to understand this comment. What does 'below'
> > mean? Shall we rephrase 'but that always...' to:
> >
> > However, those subtransactions are always nested below the subtransaction that
> > acquired the slot, so their subtransaction IDs are deeper and therefore do not
> > match here. Decoding also ....
> >
> > (I hope your comment meant this, else let me know)
>
> That's right. TXN -> SUBTXN1 (acquires the slot) -> SUBTXN2 (internal
> subxact started during decoding in ReorderBufferProcessTXN()), and
> while in SUBTXN2, the historic snapshot is held. Your wording looks
> fine to me.
>
> > 2)
> > +-- Test 3: same as Test 1 for a temporary slot. Releasing a temporary slot on
> > +-- error does not drop it, so it would keep holding back WAL removal and the
> > +-- catalog xmin. The session's temporary slots are dropped as well, so none is
> > +-- left behind.
> >
> > The comment is slightly confusing. We are intititally saying 'it does
> > not drop temp-slot' and then saying 'it is dropped'. Do we want to
> > distinguish the sentences as old and post-patch behaviour somehow?
>
> I wanted to say the difference between slot release and cleanup there.
> I simplified it as follows, and the comments in
> AtEOSubXact_ReplicationSlot() have a detailed explanation anyway.
>
> +-- Test 3: same as Test 1 for a temporary slot, which is dropped rather than
> +-- just released, so it is not left behind after the error.
>

Okay, I get it now. Thanks for the making the change.

v10 compiles without these inclusions. Can you please check?

xact.c:
+#include "replication/slot.h"

slot.c:
+#include "access/xact.h"

thanks
Shveta