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

Hi Shubham,

Here are my review comments for the patch v5-0001.

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

1.
- must accept local connections.
+ must accept local connections. If you are planning to use the
+ --enable-two-phase switch then you will also need to set the
+ <xref linkend="guc-max-prepared-transactions"/> appropriately.

Should use sgml <option> markup for "--enable-two-phase".

~~~

2.
- <application>pg_createsubscriber</application> sets up logical
- replication with two-phase commit disabled. This means that any
- prepared transactions will be replicated at the time
- of <command>COMMIT PREPARED</command>, without advance preparation.
- Once setup is complete, you can manually drop and re-create the
- subscription(s) with
+ If --enable-two-phase switch is not specified, the
+ <application>pg_createsubscriber</application> sets up logical replication
+ with two-phase commit disabled. This means that any prepared transactions
+ will be replicated at the time of <command>COMMIT PREPARED</command>,
+ without advance preparation. Once setup is complete, you can manually drop
+ and re-create the subscription(s) with

Should use sgml <option> markup for "--enable-two-phase".

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

3.
+# Verify that the subtwophase is 'p' in the pg_subscription catalog
+my $poll_query_until = $node_s->safe_psql('postgres',
+ "SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT
IN ('e');"
+);
+
+is($poll_query_until, qq(t),
+ 'Timed out while waiting for subscriber to enable twophase');

3a.
Hmm. Does this code match the comment? The comment says verify
subtwophase is 'p' (aka "pending") but AFAICT the code is actually
waiting until every subtwophase is 'e' (aka "enabled").

~

3b.
Also, if you are going to name char-codes (like 'p') in comments, it
might be helpful to include the equivalent words, saving readers from
having to search the documentation to find the meaning.

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