On Tue, Feb 27, 2024 at 12:48 PM Peter Smith <smithpb2250@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.