On Fri, Jul 19, 2024 at 8:06 AM Peter Smith <smithpb2...@gmail.com> wrote: > > ====== > You wrote "tried to make the two_phase change before failover option > wherever it makes sense to keep the code consistent". But, still > failover is coded first in lots of places: > - libpqrcv_alter_slot > - ReplicationSlotAlter > - AlterReplicationSlot > etc. >
In ReplicationSlotAlter(), there are error conditions related to standby and failover slots which are better checked before setting two_phase property. The main reason for keeping two_phase before the failover option in subscriptioncmds.c is that SUBOPT_TWOPHASE_COMMIT was introduced before the equivalent failover option. We can do at other places as you pointed but I didn't see any compelling reason to not do what we normally do which is to add the new options at the end. > ====== > src/include/replication/slot.h > > 1. > -extern void ReplicationSlotAlter(const char *name, bool failover); > +extern void ReplicationSlotAlter(const char *name, bool *failover, > + bool *two_phase); > > Use const? > If so, we need to use const both for failover and two_phase but not sure if that is required here. We can evaluate that separately if required by comparing it with similar instances. > ====== > 99. > Please see attached diffs implementing the nitpicks mentioned above > These look good to me, so will incorporate them in the next patch. -- With Regards, Amit Kapila.
