Re: [PATCH] Release replication slot on error in SQL-callable slot functions - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: [PATCH] Release replication slot on error in SQL-callable slot functions
Date
Msg-id CAA4eK1K2iLuJmPLV=8Wjyx5p2H0kHijt4T-u-=NOsxtD5ZAL5Q@mail.gmail.com
Whole thread
In response to Re: [PATCH] Release replication slot on error in SQL-callable slot functions  (Masahiko Sawada <sawada.mshk@gmail.com>)
List pgsql-hackers
On Wed, Sep 16, 2026 at 5:43 AM Masahiko Sawada <sawada.mshk@gmail.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.
>

Yeah, we can do a minimal fix for back-branches on the lines you are
suggesting but OTOH, I think we are over worried about the
hypothetical cases. I feel there is no harm in keeping the HEAD and
back-branches code/behavior same, in the worst case, if we get any
report, we can address keeping the actual usage in mind.

--
With Regards,
Amit Kapila.



pgsql-hackers by date:

Previous
From: Fujii Masao
Date:
Subject: Re: Stabilize recovery conflict stats checks in 031_recovery_conflict.pl
Next
From: Shlok Kyal
Date:
Subject: Re: Support EXCEPT for ALL SEQUENCES publications