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

On Wed, Dec 11, 2024 at 4:21 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham,
>
> Here are some review comments for patch v2-0001.
>
> ======
> GENERAL - the new option name
>
> 1.
> I am not sure if this new switch needed to be called
> '--enable-two-phase'; probably just calling it '--two-phase' would
> mean the same because specifying the switch already implies "enabling"
> it to me.
>
> Perhaps either name is fine. What do others think?
>
> ======
> Commit message
>
> 2.
> This patch introduces the '--enable-two-phase' option to the
> 'pg_createsubscriber' utility, allowing users to enable or disable two-phase
> commit for subscriptions during their creation.
>
> ~
>
> It seems a bit strange IMO to say "enable or disable", because this is
> only for "enable", and the default is disable as described in the
> following sentence.
>
> ~~~
>
> 3.
> By default, two-phase commit is disabled if the option is provided without
> arguments.
>
> ~
>
> But, this option now has no arguments so the part "if the option is
> provided without arguments" is not applicable anymore and should be
> removed. Or, if you want to say something you could say "if the option
> is not provided."
>
> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 4.
> + <varlistentry>
> + <term><option>-T</option></term>
> + <term><option>--enable-two-phase</option></term>
> + <listitem>
> + <para>
> + Enables two-phase commit for the subscription. When the option is
> + provided, it is explicitly enabled. By default, two-phase commit is
> + <literal>off</literal>.
> + Two-phase commit ensures atomicity in logical replication for prepared
> + transactions. See the
> + <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> + documentation for more details.
> + </para>
> + </listitem>
> + </varlistentry>
> +
>
> I felt this was more verbose than necessary. IMO you only needed to
> say something very simple; the user can chase the link to learn more
> about two_phase if they want to.
>
> SUGGESTION
> Enables <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> commit for the subscription. The default is <literal>false</literal>.
>
> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> usage:
>
> 5.
> printf(_(" -t, --recovery-timeout=SECS seconds to wait for
> recovery to end\n"));
> + printf(_(" -T, --enable-two-phase enable two-phase commit for the
> subscription\n"));
> printf(_(" -U, --subscriber-username=NAME user name for subscriber
> connection\n"));
>
> AFAICT the patch is wrongly using tabs here instead of spaces.
>
> ~~~
>
> store_pub_sub_info:
>
> 6.
> + dbinfo[i].two_phase = opt->two_phase; /* Set two-phase option */
>
> I still think this comment only states the obvious so it is not
> helpful. Can remove it.
>
> ~~~
>
> 7.
> dbinfo[i].made_publication = false;
> +
> /* Fill subscriber attributes */
> This whitespace change is unrelated to this patch.
>
> ~~~
>
> 8.
> - pg_log_debug("subscriber(%d): subscription: %s ; connection string: %s", i,
> + pg_log_debug("subscriber(%d): subscription: %s ; connection string:
> %s, --enable-two-phase: %s", i,
> dbinfo[i].subname ? dbinfo[i].subname : "(auto)",
> - dbinfo[i].subconninfo);
> + dbinfo[i].subconninfo,
> + dbinfo[i].two_phase ? "true" : "false");
>
> IMO say "two_phase" here, not "--enable-two-phase".
>
> ======
> .../t/040_pg_createsubscriber.pl
>
> 9.
> max_worker_processes = 8
> +max_prepared_transactions = 10
> });
>
> AFAICT the comment for this test code said:
>
> # Restore default settings here but only apply it after testing standby. Some
> # standby settings should not be a lower setting than on the primary.
>
> But max_prepared_transactions = 10 is not the default setting for this GUC.
>
> ~~~
>
> 10.
> is($node_s->safe_psql('postgres', 'SELECT pg_catalog.pg_is_in_recovery()'),
> 't', 'standby is in recovery');
> +
> $node_s->stop;
>
> This whitespace change has nothing to do with the patch.
>
> ~~~
>
> 11.
> - 'replslot1'
> + 'replslot1', '--enable-two-phase'
>
> The comment for this test only says
> # pg_createsubscriber can run without --databases option
>
> Now it is doing more, so maybe it should give more details like "In
> passing, also test the --enable-two-phase option."
>
> ~~~
>
> 12.
> +# Verify that the prepared transaction is replicated to the subscriber
> +my $count_prepared_s =
> + $node_s->safe_psql($db1, "SELECT count(*) FROM pg_prepared_xacts;");
> +
> +is($count_prepared_s, qq(0), 'Prepared transaction replicated to subscriber');
> +
>
> Is this test OK? It says to verify it is replicated. How does checking
> subscriber has zero pg_prepared_xacts ensure replication occurred?
>
> ======
> Please see the attached NITPICKS diffs which includes some (not all)
> of the above suggestions.
>
> ======

I have fixed the given comments. The attached patch contains the
suggested changes.

Thanks and regards,
Shubham Khanna.