On Thu, Dec 14, 2023 at 7:00 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi, here are a few more review comments for the patch v47-0002
>
> (plus my review comments of v45-0002 [1] are yet to be addressed)
>
> ======
> 1. General
>
> For consistency and readability, try to use variables of the same
> names whenever they have the same purpose, even when they declared are
> in different functions. A few like this were already mentioned in the
> previous review but there are more I keep noticing.
>
> For example,
> 'slotnameChanged' in function, VERSUS 'primary_slot_changed' in the caller.
>
>
> ======
> src/backend/replication/logical/slotsync.c
>
> 2.
> +/*
> + *
> + * Validates the primary server info.
> + *
> + * Using the specified primary server connection, it verifies whether
> the master
> + * is a standby itself and returns true in that case to convey the caller that
> + * we are on the cascading standby.
> + * But if master is the primary server, it goes ahead and validates
> + * primary_slot_name. It emits error if the physical slot in primary_slot_name
> + * does not exist on the primary server.
> + */
> +static bool
> +validate_primary_info(WalReceiverConn *wrconn)
>
> 2b.
> IMO it is too tricky to have a function called "validate_xxx", when
> actually you gave that return value some special unintuitive meaning
> other than just validation. IMO it is always better for the returned
> value to properly match the function name so the expectations are very
> obvious. So, In this case, I think a better function signature would
> be like this:
>
> SUGGESTION
>
> static void
> validate_primary_info(WalReceiverConn *wrconn, bool *master_is_standby)
>
> or
>
> static void
> validate_primary_info(WalReceiverConn *wrconn, bool *am_cascading_standby)
>
The terminology master_is_standby is a bit indirect for this usage, so
I would prefer the second one. Shall we name this function as
check_primary_info()? Additionally, can we rewrite the following
comment: "Using the specified primary server connection, check whether
we are cascading standby. It also validates primary_slot_info for
non-cascading-standbys.".
--
With Regards,
Amit Kapila.