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

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 17 May 2024 03:06:34 +0000

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/

---

From: Peter Smith <[email-masked]>
Date: Fri, 17 May 2024 14:08:15 +1000

Hi Kuroda-san,

I did not apply these v12* patches, but I have diff'ed all of them
with the previous v11* patches and confirmed that all of my previous
review comments now seem to be addressed.

I don't have any more comments to make at this time. Thanks!

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 25 Jun 2024 08:06:10 +0000

Dear hackers,

I found that v12 patch set could not be accepted by the cfbot. PSA new version.

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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 25 Jun 2024 09:11:16 +0000

> I found that v12 patch set could not be accepted by the cfbot. PSA new version.

To make others more trackable, I shared changes just in case. All failures were occurred
on the pg_dump code. I added an attribute in pg_subscription and modified pg_dump code,
but it was wrong. A constructed SQL became incomplete. I.e., in [1]: 

```
pg_dump: error: query failed: ERROR:  syntax error at or near "."
LINE 15:  s.subforcealter
           ^
pg_dump: detail: Query was: SELECT s.tableoid, s.oid, s.subname,
 s.subowner,
 s.subconninfo, s.subslotname, s.subsynccommit,
 s.subpublications,
 s.subbinary,
 s.substream,
 s.subtwophasestate,
 s.subdisableonerr,
 s.subpasswordrequired,
 s.subrunasowner,
 s.suborigin,
 NULL AS suboriginremotelsn,
 false AS subenabled,
 s.subfailover
 s.subforcealter
FROM pg_subscription s
WHERE s.subdbid = (SELECT oid FROM pg_database
                   WHERE datname = current_database())
```

Based on that I just added a comma in 0004 patch.

[1]: https://cirrus-ci.com/task/6710166165389312

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