On Fri, Dec 15, 2023 at 11:02 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> 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:
>
Sorry, I missed attaching the patch. PFA v48.
> 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