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/