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

On Fri, Mar 12, 2021 at 8:38 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
...
> 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);
>

It was deliberately coded this way for consistency with the other new
PG14 options - e.g. it mimics exactly binary_given, and
streaming_given.

I know the param is not currently used by the caller and so could be a
local (as you say), but I felt the code consistency and future-proof
benefits outweighed the idea of reducing the code to bare minimum
required to work just "because we can".

So I don't plan to change this, but if you still feel strongly that
the parameter must be removed please give a convincing reason.

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