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

Hi Shubham,

Here are some review comments for the patch v4-0001.

======
GENERAL.

1.
After reading Vignesh's last review and then the pg_createsubscriber
documentation I see there can be multiple databases simultaneously
specified (by writing multiple -d switches) and in that case each one
gets its own:
--publication
--replication-slot
--subscription

OTOH, this new '--enable-two-phase' switch is just a blanket two_phase
enablement across all subscriptions. There is no way for the user to
say if they want it enabled for some subscriptions (on some databases)
but not for others. I suppose this was intentional and OK (right?),
but this point needs to be clarified in the docs.

======
doc/src/sgml/ref/pg_createsubscriber.sgml

2.
+ <para>
+ Enables <link
linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
+ commit for the subscription. The default is <literal>false</literal>.
+ </para>

Following on from the previous review comment. Since this switch is
applied to all database subscriptions I think the help needs to say
that. Something like.

"If there are multiple subscriptions specified, this option applies to
all of them."

~~~

3.
In the "Prerequisites" sections of the docs, it gives requirements for
various GUC settings on the source server and the target server. So,
should there be another note here advising to configure the
'max_prepared_transactions' GUC when the '--enable-two-phase' is
specified?

~~~

4. "Warnings" section includes the following:

pg_createsubscriber sets up logical replication with two-phase commit
disabled. This means that any prepared transactions will be replicated
at the time of COMMIT PREPARED, without advance preparation. Once
setup is complete, you can manually drop and re-create the
subscription(s) with the two_phase option enabled.

~

The above text is from the "Warnings" section, but it seems stale
information that needs to be updated due to the introduction of this
new '--enable-two-phase' option.

======
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"));

Given the previous review comments (#1/#2 etc), I was wondering if it
might be better to say more like "enable two-phase commit for all
subscriptions".

======
.../t/040_pg_createsubscriber.pl

6.
+is($count_prepared_s, qq(1), 'Prepared transaction replicated to subscriber');

Should there also have been an earlier check (way back before the
PREPARE) just to make sure this count was initially 0?

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