Horton

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

Peter Smith <smithpb2250@gmail.com>

View thread
From: Peter Smith <smithpb2250@gmail.com>
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>, Amit Kapila <amit.kapila16@gmail.com>, Ajin Cherian <itsajin@gmail.com>, Давыдов Виталий <v.davydov@postgrespro.ru>
Date: 2024-05-17T02:20:55Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix random failure in 021_twophase.

  2. Allow altering of two_phase option of a SUBSCRIPTION.

  3. Doc: use true|false rather than on|off for "failover" option

  4. Support an optional asynchronous commit mode, in which we don't flush WAL

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