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

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.

======
Kind Regards,
Peter Smith.
Fujitsu Australia