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

Hi

On Thu, May 28, 2026 at 10:45 PM shveta malik <shveta(dot)malik(at)gmail(dot)com>
wrote:

> 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.

Got stuck with something. Will send a revised patch over the weekend, in
the meantime if you want to take it forward please feel free to.

Thanks,
Satya

>
>
>