Re: Synchronizing slots from primary to standby
От | shveta malik |
---|---|
Тема | Re: Synchronizing slots from primary to standby |
Дата | |
Msg-id | CAJpy0uD+FcaakEpUp3CVAn+PhmRdkxb8gv4ONZCgPQ7Jm0rBcg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Synchronizing slots from primary to standby (shveta malik <shveta.malik@gmail.com>) |
Ответы |
Re: Synchronizing slots from primary to standby
(Amit Kapila <amit.kapila16@gmail.com>)
Re: Synchronizing slots from primary to standby (Peter Smith <smithpb2250@gmail.com>) |
Список | pgsql-hackers |
On Fri, Dec 15, 2023 at 11:02 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Thu, Dec 14, 2023 at 4:40 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > 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. > > > 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
Вложения
В списке pgsql-hackers по дате отправления: