Re: Adding a '--two-phase' option to 'pg_createsubscriber' utility.

On Mon, Dec 9, 2024 at 7:43 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Mon, 9 Dec 2024 at 16:25, Shubham Khanna <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > Hi all,
> >
> > I am writing to propose the addition of the two_phase option in
> > pg_createsubscriber. As discussed in [1], supporting this feature
> > during the development of pg_createsubscriber was planned for this
> > version.
>
> Yes this will be useful.
>
> > Currently, pg_createsubscriber creates subscriptions with the
> > two_phase option set to false. Enabling the two_phase option after a
> > subscription has been created is not straightforward, as it requires
> > the subscription to be disabled first. This patch aims to address this
> > limitation by incorporating the two_phase option into
> > pg_createsubscriber which will help create subscription with two_phase
> > option and make use of the advantages of creating subscription with
> > two_phase option.
> > The attached patch has the changes for the same.
>
> Few comments:
> 1) You can keep it with no argument similar to how dry-run is handled:
> @@ -1872,6 +1881,7 @@ main(int argc, char **argv)
> {"publisher-server", required_argument, NULL, 'P'},
> {"socketdir", required_argument, NULL, 's'},
> {"recovery-timeout", required_argument, NULL, 't'},
> + {"two_phase", optional_argument, NULL, 'T'},
> {"subscriber-username", required_argument, NULL, 'U'},
> {"verbose", no_argument, NULL, 'v'},
> {"version", no_argument, NULL, 'V'},
>

Changed the argument to 'no_argument'.

> 2) After making it to no argument option, if this option is set, just
> set the value, strcmp's are not required:
> + case 'T':
> + if (optarg != NULL)
> + {
> + if (strcmp(optarg, "on") == 0)
> + opt.two_phase = true;
> + else if (strcmp(optarg, "off") == 0)
> + opt.two_phase = false;
> + else
> + pg_fatal("invalid
> value for --two-phase: must be 'on' or 'off'");
> + }
> + else
> + opt.two_phase = true; /*
> Default to true if no argument
> +
> * is provided */
> + break;
>

Updated the switch case according to the modified argument.

> 3) You can add a link to create subscription documentation page of
> two_phase option here:
> + Enables or disables two-phase commit for the subscription.
> + When the option is provided without a value, it defaults to
> + <literal>on</literal>. Specify <literal>on</literal> to enable or
> + <literal>off</literal> to disable.
> + Two-phase commit ensures atomicity in logical replication for prepared
> + transactions. By default, this option is enabled unless explicitly set
> + to <literal>off</literal>.
>

Added the link to create subscription in the documentation of
pg_createsubscriber.

> 4) Instead of adding new tests, can we include the prepare transaction
> and prepare transaction verification to the existing tests itself?
> +# Set up node A as primary
> +my $node_a = PostgreSQL::Test::Cluster->new('node_a');
> +my $aconnstr = $node_a->connstr;
> +$node_a->init(allows_streaming => 'logical');
> +$node_a->append_conf(
> + 'postgresql.conf', qq[
> + autovacuum = off
> + wal_level = logical
> + max_wal_senders = 10
> + max_worker_processes = 8
> + max_prepared_transactions = 10
> +]);
> +
> +$node_a->start;
>

Removed the new test case and added it to the existing test cases.

The attached Patch contains the required changes.

Thanks and regards,
Shubham Khanna.