Re: Slow catchup of 2PC (twophase) transactions on replica in LR

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