On Fri, Mar 12, 2021 at 2:29 PM Peter Smith <smithpb2...@gmail.com> wrote: > > On Fri, Mar 12, 2021 at 4:07 PM vignesh C <vignes...@gmail.com> wrote: > > Hi Vignesh, > > Thanks for the review comments. > > But can you please resend it with each feedback enumerated as 1. 2. > 3., or have some other clear separation for each comment. > > (Because everything is mushed together I am not 100% sure if your > comment text applies to the code above or below it)
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); 2) I think this is not possible as we don't allow changing twophase option, should this be an assert. --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -2930,6 +2930,7 @@ maybe_reread_subscription(void) strcmp(newsub->slotname, MySubscription->slotname) != 0 || newsub->binary != MySubscription->binary || newsub->stream != MySubscription->stream || + newsub->twophase != MySubscription->twophase || !equal(newsub->publications, MySubscription->publications)) 3) We have the following check in parse_subscription_options: if (twophase && *twophase_given && *twophase) { if (streaming && *streaming_given && *streaming) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s and %s are mutually exclusive options", "two_phase = true", "streaming = true"))); } Should we have a similar check in parse_output_parameters? @@ -252,6 +254,16 @@ parse_output_parameters(List *options, uint32 *protocol_version, *enable_streaming = defGetBoolean(defel); } + else if (strcmp(defel->defname, "two_phase") == 0) + { + if (twophase_given) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("conflicting or redundant options"))); + twophase_given = true; + + *enable_twophase = defGetBoolean(defel); + } Regard, Vignesh