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

Hi, Here are my remaining review comments for the latest v11* patches.

//////////
v11-0001
//////////

No changes. No comments.

//////////
v11-0002
//////////

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

2.1.
<command>ALTER SUBSCRIPTION ... SET (failover = on|off)</command> and
- <command>ALTER SUBSCRIPTION ... SET (two_phase = on|off)</command>
+ <command>ALTER SUBSCRIPTION ... SET (two_phase = off)</command>

My other thread patch has already been pushed [1], so now you can
modify this to say "true|false" as previously suggested.

//////////
v11-0003
//////////

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

3.1. AlterSubscription

+ subtwophase = LOGICALREP_TWOPHASE_STATE_DISABLED;
+ }
+ else
+ subtwophase = LOGICALREP_TWOPHASE_STATE_PENDING;
+
+
/* Change system catalog acoordingly */
values[Anum_pg_subscription_subtwophasestate - 1] =
- CharGetDatum(opts.twophase ?
- LOGICALREP_TWOPHASE_STATE_PENDING :
- LOGICALREP_TWOPHASE_STATE_DISABLED);
+ CharGetDatum(subtwophase);
replaces[Anum_pg_subscription_subtwophasestate - 1] = true;

Sorry, I don't think that 'subtwophase' change is an improvement --
IMO the existing ternary code was fine as-is.

I only reported the excessive flag checking in the previous v10-0003
review because of some extra "if (!opts.twophase)" code but that was
caused by what you called "wrong git operations." and is already fixed
now.

//////////
v11-0004
//////////

======
src/sgml/catalogs.sgml

4.1.
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>subforcealter</structfield> <type>bool</type>
+ </para>
+ <para>
+ If true, then the <link
linkend="sql-altersubscription"><command>ALTER
SUBSCRIPTION</command></link>
+ can disable <literal>two_phase</literal> option, even if there are
+ uncommitted prepared transactions from when <literal>two_phase</literal>
+ was enabled
+ </para></entry>
+ </row>
+

I think this description should be changed to say what it *really*
does. IMO, the stuff about 'two_phase' is more like a side-effect.

SUGGESTION (this is just pseudo-code. You can add the CREATE
SUBSCRIPTION 'force_alter' link appropriately)

If true, then the <command>ALTER SUBSCRIPTION</command> command can
sometimes be forced to proceed instead of giving an error. See
<link>force_alter</link> parameter for details about when this might
be useful.

======
[1] https://github.com/postgres/postgres/commit/fa65a022db26bc446fb67ce1d7ac543fa4bb72e4

Kind Regards,
Peter Smith.
Fujitsu Australia