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

From Peter Smith
Subject Re: Slow catchup of 2PC (twophase) transactions on replica in LR
Date
Msg-id CAHut+PuWui2cfUO_YY+Jqgku4qvYcK8M3557aWdv4rLh1C0nXQ@mail.gmail.com
Whole thread Raw
In response to RE: Slow catchup of 2PC (twophase) transactions on replica in LR  ("Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>)
Responses RE: Slow catchup of 2PC (twophase) transactions on replica in LR
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Nathan Bossart
Date:
Subject: Re: allow changing autovacuum_max_workers without restarting
Next
From: Robert Haas
Date:
Subject: Re: commitfest.postgresql.org is no longer fit for purpose