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

Hi, Here are some comments for v8-0004

======
0.1 General - Patch name

/SUBSCIRPTION/SUBSCRIPTION/

======
0.2 General - Apply

FYI, there are whitespace warnings:

git apply ../patches_misc/v8-0004-Add-force_alter-option-for-ALTER-SUBSCIRPTION-.-S.patch
../patches_misc/v8-0004-Add-force_alter-option-for-ALTER-SUBSCIRPTION-.-S.patch:191:
trailing whitespace.
# command will abort the prepared transaction and succeed.
warning: 1 line adds whitespace errors.

======
0.3 General - Regression test fails

The subscription regression tests are not working.

ok 158       + publication                              1187 ms
not ok 159   + subscription                              123 ms

See review comments #4 and #5 below for the reason why.

======
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.
+      subscription is disabled. Altering the parameter from
<literal>on</literal>
+      to <literal>off</literal> will be failed when there are prepared
+      transactions done by the logical replication worker. If you want to alter
+      the parameter forcibly in this case, <literal>force_alter</literal>
+      option must be set to <literal>on</literal> at the same time. If
+      specified, the backend process aborts prepared transactions.
      </para>
1a.
That "will be failed when..." seems strange. Maybe say "will give an
error when..."

~
1b.
Because "force" is a verb, I think true/false is more natural than
on/off for this new boolean option. e.g. it acts more like a "flag"
than a "mode". See all the other boolean options in CREATE
SUBSCRIPTION -- those are mostly all verbs too and are all true/false
AFAIK.

======

2. CREATE SUBSCRIPTION

For my previous review, two comments [v7-0004#2] and [v7-0004#3] were
not addressed. Kuroda-san wrote:
Hmm, but the parameter cannot be used for CREATE SUBSCRIPTION. Should
we modify to accept and add the description in the doc?

~

Yes, that is what I am suggesting. IMO it is odd for the user to be
able to ALTER a parameter that cannot be included in the CREATE
SUBSCRIPTION in the first place. AFAIK there are no other parameters
that behave that way.

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

3. AlterSubscription

+ if (!opts.force_alter)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot alter %s when there are prepared transactions",
+ "two_phase = off"),
+ errhint("Resolve these transactions or set %s at the same time, and
then try again.",
+ "force_alter = true")));

I think saying "at the same time" in the hint is unnecessary. Surely
the user is allowed to set this parameter separately if they want to?

e.g.
ALTER SUBSCRIPTION sub SET (force_alter=true);
ALTER SUBSCRIPTION sub SET (two_phase=off);

======
src/test/regress/expected/subscription.out

4.
+-- fail - force_alter cannot be set alone
+ALTER SUBSCRIPTION regress_testsub SET (force_alter = true);
+ERROR:  force_alter must be specified with two_phase

This error cannot happen. You removed that error!

======
src/test/regress/sql/subscription.sql

5.
+-- fail - force_alter cannot be set alone
+ALTER SUBSCRIPTION regress_testsub SET (force_alter = true);

Why is this being tested? You removed that error condition.

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

6.
+# Try altering the two_phase option to "off." The command will fail since there
+# is a prepared transaction and the force option is not specified.
+my $stdout;
+my $stderr;
+
+($result, $stdout, $stderr) = $node_subscriber->psql(
+ 'postgres', "ALTER SUBSCRIPTION regress_sub SET (two_phase = off);");
+ok($stderr =~ /cannot alter two_phase = off when there are prepared
transactions/,
+ 'ALTER SUBSCRIPTION failed');

/force option is not specified./'force_alter' option is not specified as true./

~~~

7.
+# Verify the prepared transaction still exists
+$result = $node_subscriber->safe_psql('postgres',
+    "SELECT count(*) FROM pg_prepared_xacts;");
+is($result, q(1), "prepared transaction still exits");
+

TYPO: /exits/exists/

~~~

8.
+# Alter the two_phase with the force_alter option. Apart from the above, the
+# command will abort the prepared transaction and succeed.
+$node_subscriber->safe_psql('postgres',
+    "ALTER SUBSCRIPTION regress_sub SET (two_phase = off, force_alter
= true);");
+$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION
regress_sub ENABLE;");
+

What does "Apart from the above" mean? Be more explicit.

~~~

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

/transaction are aborted/transaction was aborted/

======
Response to my v7-0004 review --
https://www.postgresql.org/message-id/OSBPR01MB2552F738ACF1DA6838025C4FF5E62%40OSBPR01MB2552.jpnprd01.prod.outlook.com

Kind Regards,
Peter Smith.
Fujitsu Australia