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

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Peter Smith <[email protected]>
To: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Ajin Cherian <[email protected]>
Cc: Давыдов Виталий <[email protected]>
Subject: Re: Slow catchup of 2PC (twophase) transactions on replica in LR
Date: Thu, 9 May 2024 17:28:57 +1000
Message-ID: <CAHut+Pv14RO--+gMr_BRwmqOLdJ+3qO-S8M7C1L4vF-QDC6aQA@mail.gmail.com> (raw)
In-Reply-To: <OSBPR01MB2552521ABDBC2F995FBACDF7F5E52@OSBPR01MB2552.jpnprd01.prod.outlook.com>
References: <8fab8-65d74c80-1-2f28e880@39088166>
	<CAA4eK1KOs3s6syZqUgrd2WvjTz64SGf0ToZcRoPMCKKH+M0YFQ@mail.gmail.com>
	<99df2-65d8d200-1-63c26880@172687752>
	<CAA4eK1KtjjPxe5tjWeGNf5CfF9qfceVQCCC9XGsKzR-ifQD9uA@mail.gmail.com>
	<ba4b6-65ddc580-9-5c3e3080@134362348>
	<CAA4eK1J6wOMkDDt3iOs+JqkvQvuwSORosTpqdejgvXqFmxex=A@mail.gmail.com>
	<badcf-65dde500-3-24ba3c80@28790566>
	<cedbe-65e0c000-1-6db17700@133269862>
	<[email protected]>
	<f9f1b-65e72c80-7-533dda00@76203995>
	<CAFPTHDbU4EkVmKL66i5BzrmQ6LFftQPx_TZ1euKt9fvVzQDyjA@mail.gmail.com>
	<CAA4eK1+n7M2S1OpoGWDd+YZkDCuURMdVRbvP0eELQUvWgmDneg@mail.gmail.com>
	<CAFPTHDa=pJSZ_4dV5DPAOapRSgPcyyUTP0WzGY2Rz_D3-gwraw@mail.gmail.com>
	<CAA4eK1K1fSkeK=kc26G5cq87vQG4=1qs_b+no4+ep654SeBy1w@mail.gmail.com>
	<23d1ca-66169f80-1f-5a90aa00@207285398>
	<26b30e-661d4880-9-62d2df00@231402233>
	<OSBPR01MB2552F73968D5A460157B57B9F5122@OSBPR01MB2552.jpnprd01.prod.outlook.com>
	<OSBPR01MB25521E993657E754B0C51A98F5122@OSBPR01MB2552.jpnprd01.prod.outlook.com>
	<OSBPR01MB2552936A8C9F9790A4CFD389F5112@OSBPR01MB2552.jpnprd01.prod.outlook.com>
	<CAHut+Pt2v3dCvWCE-ng4nxS5qU-Go4T=Q2BBnv73SAAczgrqvg@mail.gmail.com>
	<OSBPR01MB2552521ABDBC2F995FBACDF7F5E52@OSBPR01MB2552.jpnprd01.prod.outlook.com>

Hi, Here are some review comments for patch v7-0004

======
Commit message

1.
A detailed commit message is needed to describe the purpose and
details of this patch.

======
doc/src/sgml/ref/alter_subscription.sgml

2. CREATE SUBSCRIPTION

Shouldn't there be an entry for "force_alter" parameter in the CREATE
SUBSCRIPTION "parameters" section, instead of just vaguely mentioning
it in passing when describing the "two_phase" in ALTER SUBSCRIPTION?

~

3. ALTER SUBSCRIPTION - alterable parameters

And shouldn't this new option also be named in the ALTER SUBSCRIPTION
list: "The parameters that can be altered are..."

======
src/backend/commands/subscriptioncmds.c

4.
  XLogRecPtr lsn;
+ bool twophase_force;
 } SubOpts;

IMO this field ought to be called 'force_alter' to be the same as the
option name. Sure, now it is only relevant for 'two_phase', but that
might not always be the case in the future.

~~~

5. AlterSubscription

+ /*
+ * Abort prepared transactions if force option is also
+ * specified. Otherwise raise an ERROR.
+ */
+ if (!opts.twophase_force)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot alter %s when there are prepared transactions",
+ "two_phase = false")));
+

5a.
/if force option is also specified/only if the 'force_alter' option is true/

~

5b.
"two_phase = false" -- IMO that should say "two_phase = off"

~

5c.
IMO this ereport should include a errhint to tell the user they can
use 'force_alter = true' to avoid getting this error.

~~~

6.

+ /* force_alter cannot be used standalone */
+ if (IsSet(opts.specified_opts, SUBOPT_FORCE_ALTER) &&
+ !IsSet(opts.specified_opts, SUBOPT_TWOPHASE_COMMIT))
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("%s must be specified with %s",
+ "force_alter", "two_phase")));
+

IMO this rule is not necessary so the code should be removed. I think
using 'force_alter' standalone doesn't do anything at all (certainly,
it does no harm) so why add more complications (more rules, more code,
more tests) just for the sake of it?

======
src/test/subscription/t/099_twophase_added.pl

7.
+$node_subscriber->safe_psql('postgres',
+    "ALTER SUBSCRIPTION sub SET (two_phase = off, force_alter = on);");

"force" is a verb, so it is better to say 'force_alter = true' instead
of 'force_alter = on'.

~~~

8.
 $result = $node_subscriber->safe_psql('postgres',
     "SELECT count(*) FROM pg_prepared_xacts;");
 is($result, q(0), "prepared transaction done by worker is aborted");

+$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub ENABLE;");
+

I felt the ENABLE statement should be above the SELECT statement so
that the code is more like it was before applying the patch.

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

view thread (25+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Slow catchup of 2PC (twophase) transactions on replica in LR
  In-Reply-To: <CAHut+Pv14RO--+gMr_BRwmqOLdJ+3qO-S8M7C1L4vF-QDC6aQA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox