On Wed, May 27, 2026 at 5:40 PM Fujii Masao <[email protected]> wrote: > > On Wed, May 27, 2026 at 8:00 PM shveta malik <[email protected]> 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
