Re: Synchronizing slots from primary to standby

On Thu, Dec 14, 2023 at 4:40 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Dec 14, 2023 at 7:00 AM Peter Smith <smithpb2250(at)gmail(dot)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.

PFA v48. Changes are:

1) Addressed comments by Peter for v45-002 and v47-002 given in [1]
and [2] respectively
2) Addressed comments by Amit for v47-002 given in [3], [4]
3) Addressed an old comment (#74 in [5]) of getting rid of header
inclusion from tablesync.c when there was no code change in that file.
Thanks Hou-san for working on this change.

TODO:
--Address the test comments in [1] for 050_standby_failover_slots_sync.pl
--Review the feasibility of addressing one pending comment (comment 13
in [5]) of 'r'->'n' conversion.

[1]: https://www.postgresql.org/message-id/CAHut%2BPtOc7J_n24HJ6f_dFWTuD3X2ApOByQzZf6jZz%2B0wb-ebQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAHut%2BPsvxs-%3Dj3aCpPVs3e4w78HndCdO-F4bLPzAX70%2BdgWUuQ%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/CAA4eK1L2ts%3DgfiF4aw7-DH8HWj29s08hVRq-Ff8%3DmjfdUXx8CA%40mail.gmail.com
[4]: https://www.postgresql.org/message-id/CAA4eK1%2Bw9yv%2B4UZXhiDHZpGDfbeRHYDBu23FwsniS8sYUZeu1w%40mail.gmail.com
[5]: https://www.postgresql.org/message-id/CAJpy0uDcOf5Hvk_CdCCAbfx9SY%2Bog%3D%3D%3DtgiuhWKzkYyqebui9g%40mail.gmail.com

thanks
Shveta