Re: Slow catchup of 2PC (twophase) transactions on replica in LR

Hi, Here are some review comments for v8-0003.

======
src/sgml/ref/alter_subscription.sgml

1.
+     <para>
+      The <literal>two_phase</literal> parameter can only be altered when the
+      subscription is disabled. When altering the parameter from
<literal>on</literal>
+      to <literal>off</literal>, the backend process checks prepared
+      transactions done by the logical replication worker and aborts them.
+     </para>

The text may be OK as-is, but I was wondering if it might be better to
give a more verbose explanation.

BEFORE
... the backend process checks prepared transactions done by the
logical replication worker and aborts them.

SUGGESTION
... the backend process checks for any incomplete prepared
transactions done by the logical replication worker (from when
two_phase parameter was still "on") and, if any are found, those are
aborted.

======
src/backend/commands/subscriptioncmds.c

2. AlterSubscription

- /*
- * Since the altering two_phase option of subscriptions
- * also leads to the change of slot option, this command
- * cannot be rolled back. So prevent we are in the
- * transaction block.
+ * If two_phase was enabled, there is a possibility the
+ * transactions has already been PREPARE'd. They must be
+ * checked and rolled back.
  */

BEFORE
... there is a possibility the transactions has already been PREPARE'd.

SUGGESTION
... there is a possibility that transactions have already been PREPARE'd.

~~~

3. AlterSubscription
+ /*
+ * Since the altering two_phase option of subscriptions
+ * (especially on->off case) also leads to the
+ * change of slot option, this command cannot be rolled
+ * back. So prevent we are in the transaction block.
+ */
  PreventInTransactionBlock(isTopLevel,
    "ALTER SUBSCRIPTION ... SET (two_phase = off)");


This comment is a bit vague and includes some typos, but IIUC these
problems will already be addressed by the 0002 patch changes.AFAIK
patch 0003 is only moving the 0002 comment.

======
src/test/subscription/t/099_twophase_added.pl

4.
+# Check the case that prepared transactions exist on the subscriber node
+#
+# If the two_phase is altering from "on" to "off" and there are prepared
+# transactions on the subscriber, they must be aborted. This test checks it.
+

Similar to the comment that I gave for v8-0002. I think there should
be #################### comment for the major test comment to
distinguish it from comments for the sub-steps.

~~~

5.
+# Verify the prepared transaction are aborted because two_phase is changed to
+# "off".
+$result = $node_subscriber->safe_psql('postgres',
+    "SELECT count(*) FROM pg_prepared_xacts;");
+is($result, q(0), "prepared transaction done by worker is aborted");
+

/the prepared transaction are aborted/any prepared transactions are aborted/

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