Re: [HACKERS] logical decoding of two-phase transactions

On Thu, Mar 11, 2021 at 7:20 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Thu, Mar 11, 2021 at 12:46 PM Peter Smith <smithpb2250(at)gmail(dot)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