Hi, here are my review comments for patch v19-0002.
======
src/backend/commands/subscriptioncmds.c
CheckAlterSubOption:
nitpick - tweak some comment wording
~
On Wed, Jul 17, 2024 at 3:13 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > 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.
>
> This style was needed to preserve error condition for failover option. Not changed.
>
nitpick - Hmm. I think you might be trying to preserve the ordering of
errors when that order is of no consequence. AFAICT which error comes
first here is neither documented nor regression tested. e.g.
reordering them gives no problem for testing, but OTOH reordering them
does simplify the code. Anyway, I have modified the code in my
attached nitpicks diffs to demonstrate this suggestion in case you
change your mind.
~~~
AlterSubscription:
nitpick - let's keep all the variables called 'update_xxx' together.
nitpick - comment typo /needs/need/
nitpick - tweak some comment wording
======
src/test/subscription/t/021_twophase.pl
nitpick - didn't quite understand the "Since we are..." comment. I
reworded it according to what I thought was the intention.
======
Kind Regards,
Peter Smith.
Fujitsu Australia