On Mon, Dec 9, 2024 at 7:43 PM vignesh C <[email protected]> wrote: > > On Mon, 9 Dec 2024 at 16:25, Shubham Khanna <[email protected]> > 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.
v2-0001-Add-support-for-enabling-disabling-two-phase-comm.patch
Description: Binary data
