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

Dear Peter,

Thanks for reviewing! Here are new patches.

> ======
> 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.

Fixed accordingly.

> //////////
> 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.

Ok, the part was reverted.

> //////////
> 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.
>

Fixed. But One point, the word "command" was removed. I checked other parts and
it seemed not to be needed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/