Re: Slow catchup of 2PC (twophase) transactions on replica in LR - Mailing list pgsql-hackers

From Peter Smith
Subject Re: Slow catchup of 2PC (twophase) transactions on replica in LR
Date
Msg-id CAHut+PsqMRS3dcijo5jsTSbgV1-9So-YBC7PH7xg0+Z8hA7fDQ@mail.gmail.com
Whole thread Raw
In response to RE: Slow catchup of 2PC (twophase) transactions on replica in LR  ("Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>)
List 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

pgsql-hackers by date:

Previous
From: Andrei Lepikhov
Date:
Subject: Re: Expand applicability of aggregate's sortop optimization
Next
From: Michael Paquier
Date:
Subject: Re: Flush pgstats file during checkpoints