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

On Wed, May 27, 2026 at 5:40 PM Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> On Wed, May 27, 2026 at 8:00 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > pg_copy_physical_replication_slot() should not have it as the common
> > 'copy_replication_slot' is already fixed in the patch.
>
> copy_replication_slot() calls create_physical_replication_slot() before
> entering the PG_TRY/PG_CATCH block. So if create_physical_replication_slot()
> throws an error, wouldn't the same issue still occur?
>

You are right. Using v5, if I force create_physical_replication_slot()
to fail while executing pg_copy_physical_replication_slot() (through
debugging), I can see that the next slot-related call hits an Assert.

1)
I also noticed that for pg_copy_logical_replication_slot(), we do not
hit the CATCH block of copy_replication_slot(), but instead the one in
create_logical_replication_slot(). That behavior seems fine.

However, I noticed some inconsistencies in the implementation.

For create_physical_replication_slot(), the caller
pg_create_physical_replication_slot() contains the TRY-CATCH block,
whereas create_logical_replication_slot() contains its own TRY-CATCH
block internally.

I think it makes more sense to keep the TRY-CATCH handling inside the
internal functions, i.e. create_logical_replication_slot() and
create_physical_replication_slot(), since that would automatically
cover all callers. For example, create_logical_replication_slot() is
invoked from multiple places, so callers need not worry about cleanup
handling themselves. Similarly, for
create_physical_replication_slot(), we could move the TRY-CATCH block
inside the function instead of having it in
pg_create_physical_replication_slot(). Doing so would also resolve the
issue with pg_copy_physical_replication_slot().

2)
I also feel that ReplicationSlotCreate() should be moved inside the
TRY block in create_logical_replication_slot().

Currently, if in the future ReplicationSlotCreate() gains any
post-slot-creation implementation that could throw an error, we may
end up leaving the system in an unsafe state. Keeping it inside the
TRY block would make the code more robust against such future changes.
~~

Reveiwign further. Need to review few more things on v5/v6.

thanks
Shveta