On Fri, Mar 12, 2021 at 8:38 PM vignesh C <vignes...@gmail.com> wrote: > ... > 1) I felt twophase_given can be a local variable, it need not be added > as a function parameter as it is not used outside the function. > --- a/src/backend/commands/subscriptioncmds.c > +++ b/src/backend/commands/subscriptioncmds.c > @@ -67,7 +67,8 @@ parse_subscription_options(List *options, > char **synchronous_commit, > bool *refresh, > bool *binary_given, > bool *binary, > - bool > *streaming_given, bool *streaming) > + bool > *streaming_given, bool *streaming, > + bool > *twophase_given, bool *twophase) > > The corresponding changes should be done here too: > @@ -358,6 +402,8 @@ CreateSubscription(CreateSubscriptionStmt *stmt, > bool isTopLevel) > bool copy_data; > bool streaming; > bool streaming_given; > + bool twophase; > + bool twophase_given; > char *synchronous_commit; > char *conninfo; > char *slotname; > @@ -382,7 +428,8 @@ CreateSubscription(CreateSubscriptionStmt *stmt, > bool isTopLevel) > > &synchronous_commit, > NULL, > /* no "refresh" */ > > &binary_given, &binary, > - > &streaming_given, &streaming); > + > &streaming_given, &streaming, > + > &twophase_given, &twophase); >
It was deliberately coded this way for consistency with the other new PG14 options - e.g. it mimics exactly binary_given, and streaming_given. I know the param is not currently used by the caller and so could be a local (as you say), but I felt the code consistency and future-proof benefits outweighed the idea of reducing the code to bare minimum required to work just "because we can". So I don't plan to change this, but if you still feel strongly that the parameter must be removed please give a convincing reason. ---- Kind Regards, Peter Smith. Fujitsu Australia.
