Re: Slow catchup of 2PC (twophase) transactions on replica in LR - Mailing list pgsql-hackers

Hi, Here are some review comments for patch v7-0004

======
Commit message

1.
A detailed commit message is needed to describe the purpose and
details of this patch.

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

2. CREATE SUBSCRIPTION

Shouldn't there be an entry for "force_alter" parameter in the CREATE
SUBSCRIPTION "parameters" section, instead of just vaguely mentioning
it in passing when describing the "two_phase" in ALTER SUBSCRIPTION?

~

3. ALTER SUBSCRIPTION - alterable parameters

And shouldn't this new option also be named in the ALTER SUBSCRIPTION
list: "The parameters that can be altered are..."

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

4.
  XLogRecPtr lsn;
+ bool twophase_force;
 } SubOpts;

IMO this field ought to be called 'force_alter' to be the same as the
option name. Sure, now it is only relevant for 'two_phase', but that
might not always be the case in the future.

~~~

5. AlterSubscription

+ /*
+ * Abort prepared transactions if force option is also
+ * specified. Otherwise raise an ERROR.
+ */
+ if (!opts.twophase_force)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot alter %s when there are prepared transactions",
+ "two_phase = false")));
+

5a.
/if force option is also specified/only if the 'force_alter' option is true/

~

5b.
"two_phase = false" -- IMO that should say "two_phase = off"

~

5c.
IMO this ereport should include a errhint to tell the user they can
use 'force_alter = true' to avoid getting this error.

~~~

6.

+ /* force_alter cannot be used standalone */
+ if (IsSet(opts.specified_opts, SUBOPT_FORCE_ALTER) &&
+ !IsSet(opts.specified_opts, SUBOPT_TWOPHASE_COMMIT))
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("%s must be specified with %s",
+ "force_alter", "two_phase")));
+

IMO this rule is not necessary so the code should be removed. I think
using 'force_alter' standalone doesn't do anything at all (certainly,
it does no harm) so why add more complications (more rules, more code,
more tests) just for the sake of it?

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

7.
+$node_subscriber->safe_psql('postgres',
+    "ALTER SUBSCRIPTION sub SET (two_phase = off, force_alter = on);");

"force" is a verb, so it is better to say 'force_alter = true' instead
of 'force_alter = on'.

~~~

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

+$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub ENABLE;");
+

I felt the ENABLE statement should be above the SELECT statement so
that the code is more like it was before applying the patch.

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