Hi On Thu, May 28, 2026 at 10:45 PM shveta malik <[email protected]> wrote:
> 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. 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 > > >
