Re: Adding a '--two-phase' option to 'pg_createsubscriber' utility. - Mailing list pgsql-hackers

On Mon, 9 Dec 2024 at 16:25, Shubham Khanna <khannashubham1197@gmail.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'},

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;

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>.

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;

Regards,
Vignesh