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

From: vignesh C <vignesh21(at)gmail(dot)com>
To: Shubham Khanna <khannashubham1197(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Adding a '--two-phase' option to 'pg_createsubscriber' utility.
Date: 2024-12-09 14:13:20
Message-ID: CALDaNm1cjkdV-m6wG8XfK6sEHn6+jDTYHqBTM4ay00GVZaLf=w@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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'},

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2024-12-09 14:35:38 IWYU annotations
Previous Message Joe Conway 2024-12-09 14:11:11 Re: Replace current implementations in crypt() and gen_salt() to OpenSSL