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

On Fri, Mar 12, 2021 at 2:29 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Fri, Mar 12, 2021 at 4:07 PM vignesh C <vignesh21(at)gmail(dot)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