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