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

On Thu, May 28, 2026 at 10:11 PM SATYANARAYANA NARLAPURAM
<satyanarlapuram(at)gmail(dot)com> wrote:
>
> Hi
>
> On Thu, May 28, 2026 at 9:17 PM Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>
>> On Thu, May 28, 2026 at 10:11 AM SATYANARAYANA NARLAPURAM
>> <satyanarlapuram(at)gmail(dot)com> wrote:
>> > Thanks for the patches, I combined these changes in my latest patch. Please find the v5.
>>
>> Thanks for updating the patch! But, v5 patch caused a compilation failure.
>>
>> slotfuncs.c:119:32: error: too few arguments to function call, single
>> argument 'try_disable' was not specified
>> 119 | ReplicationSlotDropAcquired();
>> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
>> ../../../src/include/replication/slot.h:338:13: note:
>> 'ReplicationSlotDropAcquired' declared here
>> 338 | extern void ReplicationSlotDropAcquired(bool try_disable);
>> | ^ ~~~~~~~~~~~~~~~~
>> slotfuncs.c:207:32: error: too few arguments to function call, single
>> argument 'try_disable' was not specified
>> 207 | ReplicationSlotDropAcquired();
>> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
>> ../../../src/include/replication/slot.h:338:13: note:
>> 'ReplicationSlotDropAcquired' declared here
>> 338 | extern void ReplicationSlotDropAcquired(bool try_disable);
>> | ^ ~~~~~~~~~~~~~~~~
>> slotfuncs.c:922:32: error: too few arguments to function call, single
>> argument 'try_disable' was not specified
>> 922 | ReplicationSlotDropAcquired();
>> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
>> ../../../src/include/replication/slot.h:338:13: note:
>> 'ReplicationSlotDropAcquired' declared here
>> 338 | extern void ReplicationSlotDropAcquired(bool try_disable);
>> | ^ ~~~~~~~~~~~~~~~~
>> 3 errors generated.
>
>
> Please see the v6 patch. Upstream commit 2af1dc89282 changed the ReplicationSlotDropAcquired signature since the patch generated.
>

I've reviewed the v6 patch, and here are some comments:

Maybe pg_sync_replication_slots() has the same problem if it's called
inside an exception block?

---
The patch adds PG_TRY()/PG_CATCH() to each replication slot function,
but there is no comment explaining why we need them even though we
call ReplicationSlotRelease() and ReplicationSlotCleanup() in error
paths. Also, while probably the proposed idea works for back branches,
I guess we might want to consider more comprehensive approach for HEAD
to deal with this issue as it seems to me very error-prone,
especially when adding a new replication slot function.

This problem stems from the fact that when replication slots are used
within an exception block, we don't reach the error path even if an
error occurs, and we cannot simply call ReplicationSlotRelease()
during subtransaction abort as we need to start and abort transaction
while holding a slot. But I guess that we can release the slot when
aborting the subtransaction or above (sub)transaction where we
created/acquired the slot. I've drafted the patch for this idea and
confirmed it passes the regression tests, I still need to verify its
feasibility though.

Regards,

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