On Tue, Feb 27, 2024 at 12:48 PM Peter Smith <smithpb2...@gmail.com> wrote: > > Here are some review comments for v99-0001 > > ========== > 0. GENERAL. > > +#standby_slot_names = '' # streaming replication standby server slot names > that > + # logical walsender processes will wait for > > IMO the GUC name is too generic. There is nothing in this name to > suggest it has anything to do with logical slot synchronization; that > meaning is only found in the accompanying comment -- it would be > better if the GUC name itself were more self-explanatory. > > e.g. Maybe like 'wal_sender_sync_standby_slot_names' or > 'wal_sender_standby_slot_names', 'wal_sender_wait_for_standby_slots', > or ... >
It would be wrong and or misleading to append wal_sender to this GUC name as this is used during SQL APIs as well. Also, adding wait sounds more like a boolean. So, I don't see the proposed names any better than the current one. > ~~~ > > 9. > + /* Now verify if the specified slots really exist and have correct type */ > + if (!validate_standby_slots(newval)) > + return false; > > As in a prior comment, if ReplicationSlotCtl is NULL then it is not > always going to do exactly what that comment says it is doing... > It will do what the comment says when invoked as part of the SIGHUP signal. I think the current comment is okay. -- With Regards, Amit Kapila.
