On Thu, Mar 11, 2021 at 7:20 AM Peter Smith <smithpb2...@gmail.com> wrote: > > On Thu, Mar 11, 2021 at 12:46 PM Peter Smith <smithpb2...@gmail.com> wrote: > > > > Please find attached the latest patch set v57* > > > > Differences from v56* are: > > > > * Rebased to HEAD @ today > > > > * Addresses the following feedback issues: > > > > (24) [vc-0305] Done. Ran pgindent for all patch 0001 source files. > > > > (49) [ak-0308] Fixed. In apply_handle_begion_prepare, don't set > > in_remote_transaction if psf spooling > > > > (50) [ak-0308] Fixed. In apply_handle_prepare, assert > > !in_remote_transaction if psf spooling. > > > > (52) [vc-0309] Done. Patch 0002. Simplify the way test 020 creates the > > publication. > > > > (53) [vc-0309] Done. Patch 0002. Simplify the way test 022 creates the > > publication. > > > > ----- > > [vc-0305] > > https://www.postgresql.org/message-id/CALDaNm1rRG2EUus%2BmFrqRzEshZwJZtxja0rn_n3qXGAygODfOA%40mail.gmail.com > > [vc-0309] > > https://www.postgresql.org/message-id/CALDaNm0QuncAis5OqtjzOxAPTZRn545JLqfjFEJwyRjUH-XvEw%40mail.gmail.com > > [ak-0308] > > https://www.postgresql.org/message-id/CAA4eK1%2BoSUU77T92FueDJWsp%3DFjTroNaNC-K45Dgdr7f18aBFA%40mail.gmail.com > > > > Kind Regards, > > Peter Smith. > > Fujitsu Australia > > Oops. I posted the wrong patch set in my previous email. > > Here are the correct ones for v57*.
Thanks for the updated patch, few comments: --- 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) 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. The corresponding changes can 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); --- 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)) I think this is not possible, should this be an assert. @@ -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); + } 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. Regards, Vignesh