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

On Fri, Feb 12, 2021 at 5:59 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
> (2)
>
> File : v39-0006-Support-2PC-txn-Subscription-option.patch
>
> @@ -213,6 +219,15 @@ parse_subscription_options(List *options,
>                         *streaming_given = true;
>                         *streaming = defGetBoolean(defel);
>                 }
> +               else if (strcmp(defel->defname, "two_phase") == 0 && twophase)
> +               {
> +                       if (*twophase_given)
> +                               ereport(ERROR,
> +                                               (errcode(ERRCODE_SYNTAX_ERROR),
> +                                                errmsg("conflicting or redundant options")));
> +                       *twophase_given = true;
> +                       *twophase = defGetBoolean(defel);
> +               }
>
> You can add this test in subscription.sql easily with double twophase options.

Thanks for the feedback. You are right.

But in the pgoutput.c there are several other potential syntax errors
"conflicting or redundant options" which are just like this
"two_phase" one.
e.g. there is the same error for options "proto_version",
"publication_names", "binary", "streaming".

AFAIK none of those other syntax errors had any regression tests. That
is the reason why I did not include any new test for the "two_phase"
option.

So:
a) should I add a new test per your feedback comment, or
b) should I be consistent with the other similar errors, and not add the test?

Of course it is easy to add a new test if you think option (a) is best.

Thoughts?

-----
Kind Regards,
Peter Smith.
Fujitsu Australia