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.plnitpick - 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
