From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Vitaly Davydov <v(dot)davydov(at)postgrespro(dot)ru>, Ajin Cherian <itsajin(at)gmail(dot)com> |
Subject: | Re: Slow catchup of 2PC (twophase) transactions on replica in LR |
Date: | 2024-07-17 03:31:14 |
Message-ID: | CAHut+PsqMRS3dcijo5jsTSbgV1-9So-YBC7PH7xg0+Z8hA7fDQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Here are some review comments for patch v18-0002.
======
src/backend/commands/subscriptioncmds.c
1. CheckAlterSubOption
1a.
It's not obvious why we are only checking the 'slot name' when
needs_update==true, but OTOH is always checking the 'enabled' state.
~
1b.
Param 'needs_update' is a vague name. It needs more explanatory comments or
a better name. e.g. First impression was "Why are we calling 'Alter'
function if needs_update is false?". I know it encapsulates some common
code, but if special cases cause the logic to be more confusing then that
cost may outweigh the benefit of this function.
~
1c.
If the error checks can be moved to be done up-front, then all the
'needs_update' can be combined. Avoiding multiple checks to 'needs_update'
will make this function simpler.
~~~
AlterSubscription:
nitpick - typo /needs/need/
nitpick - typo /wo_phase/two_phase/
nitpick - The comment wording "the later part...", was confusing. I've
reworded the whole comment. But this belongs in patch 0001.
======
src/test/subscription/t/021_twophase.pl
nitpick - Use the same "###############################" comment style as
in patch 0001 to indicate each main TEST scenario.
======
99.
Please refer to the diffs attachment patch, which implements any nitpicks
mentioned above.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachment | Content-Type | Size |
---|---|---|
PS_NITPICKS_20240717_2PC_V180002.txt | text/plain | 2.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2024-07-17 03:52:12 | Re: Flush pgstats file during checkpoints |
Previous Message | Andrei Lepikhov | 2024-07-17 03:28:58 | Re: Expand applicability of aggregate's sortop optimization |