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