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

On Mon, Aug 31, 2026 at 1:47 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Sun, Aug 30, 2026 at 9:40 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > > Hi Amit, By restricting in the code, does that mean adding an Assert,
> > > or a WARNING, or a WARNING plus slot release (not an error), in the
> > > replication slot subxact callback on the commit path, instead of
> > > handing the slot off to the parent across the subtransaction boundary?
> >
> > Yes, I would prefer WARNING similar to existing cases for resource
> > leaks in commit paths. One example of a similar existing case is:
> > ------
> > /* Complain if any allocated files remain open at commit. */
> > if (isCommit && numAllocatedDescs > 0)
> > elog(WARNING, "%d temporary files and directories not closed at
> > end-of-transaction",
> > numAllocatedDescs);
> > -------
> >
> > Based on above, I am imagining a check/WARNING on lines of:
>
> Thanks, Amit. That works for me. Please find the attached v15 patch.
> If it looks good, I can prepare patches for the back branches.
>

Thank you for updating the patch! Here are some review comments:

+ if (isCommit)
+ {
+ Assert(MyReplicationSlot != NULL);
+ ereport(WARNING,
+ (errcode(ERRCODE_WARNING),
+ errmsg("subtransaction left replication slot \"%s\" acquired",
+ NameStr(MyReplicationSlot->data.name)),
+ errhint("Check for missing \"ReplicationSlotRelease\"
calls.")));

The hint message "Check for missing ReplicationSlotRelease call" seems
to be for us (PostgreSQL hackers) but not users. I think such messages
should be left as a comment instead of in errhint.

Also, we don't prohibit external extensions or functions to commit a
subtransaction while holding a replication slot. If there are such
extensions, users would get WARNING messages. Which seems to be
something I'd like to avoid in minor releases.

---
+-- Error raised inside a PL/pgSQL block with an EXCEPTION clause is caught in a
+-- subtransaction; the slot must still be released.
+SELECT 'init' FROM
pg_create_logical_replication_slot('regress_subxact_slot',
'test_decoding');
+DO $$
+BEGIN
+ PERFORM pg_replication_slot_advance('regress_subxact_slot', '0/1');
+EXCEPTION WHEN OTHERS THEN
+ RAISE NOTICE 'caught SQLSTATE %', SQLSTATE;
+END $$;
+SELECT count(*) >= 0 AS peek_ok
+ FROM pg_logical_slot_peek_changes('regress_subxact_slot', NULL, NULL);

The last sentence is the comment doesn't match the test well and this
test doesn't fail on non-assertion builds. I think we can check the
active column in pg_replication_slots instead or before
slot_peek_changes() call.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com