On Thu, Dec 14, 2023 at 7:00 AM Peter Smith <smithpb2...@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.
