MIME-Version: 1.0
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>
 <a375c573-6c9b-4f0f-82c4-eb19cef2a4ca@iki.fi> <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>
 <OSBPR01MB2552707A847936E6803CFAA5F5092@OSBPR01MB2552.jpnprd01.prod.outlook.com>
 <CAA4eK1KY=uwXXuVMtuNTYHGFbbXgDveoFoP3UbxNXqxCAx8GBQ@mail.gmail.com>
 <OSBPR01MB25528F4B0B8178D3AA8DE2BFF5082@OSBPR01MB2552.jpnprd01.prod.outlook.com>
 <CAA4eK1KNrm8xE-ZOAS249m+DOk=YQ_YaoM-GsZEvC8rTMtdW8w@mail.gmail.com>
 <CAFPTHDaz4cyRN3NbjYtkqCKFj7Dv1u0Z8wviY8fnJW4+0CkF_Q@mail.gmail.com>
 <CAFPTHDZNwMWfJZprw9jsoa4-19-h337KRY9Pt41PdNiQTTYaiQ@mail.gmail.com>
 <OSBPR01MB2552083E59F366B465DF0EF8F5122@OSBPR01MB2552.jpnprd01.prod.outlook.com>
 <CAA4eK1Jpj2Ebk2C_9meEDfZKen38cxpzOkxZ7818f1cfsT8=3w@mail.gmail.com> <OSBPR01MB2552A7C652D452185785A3C2F5DE2@OSBPR01MB2552.jpnprd01.prod.outlook.com>
In-Reply-To: <OSBPR01MB2552A7C652D452185785A3C2F5DE2@OSBPR01MB2552.jpnprd01.prod.outlook.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Thu, 4 Jul 2024 17:01:40 +0530
Message-ID: <CAA4eK1LhYsNq_0XKspfWKx4oXZpXk_q-A1Oa-tbA-wqQUudhCQ@mail.gmail.com>
Subject: Re: Slow catchup of 2PC (twophase) transactions on replica in LR
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: Ajin Cherian <itsajin@gmail.com>, 
	=?UTF-8?B?0JTQsNCy0YvQtNC+0LIg0JLQuNGC0LDQu9C40Lk=?= <v.davydov@postgrespro.ru>, 
	Heikki Linnakangas <hlinnaka@iki.fi>, 
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAA4eK1LhYsNq_0XKspfWKx4oXZpXk_q-A1Oa-tbA-wqQUudhCQ%40mail.gmail.com>
Precedence: bulk

On Thu, Jul 4, 2024 at 1:34=E2=80=AFPM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > >
> > > It succeeds if force_alter is also expressly set. Prepared transactio=
ns will be
> > > aborted at that time.
> > >
> > > ```
> > > subscriber=3D# ALTER SUBSCRIPTION sub SET (two_phase =3D off, force_a=
lter =3D
> > on);
> > > ALTER SUBSCRIPTION
> > >
> >
> > Isn't it better to give a Notice when force_alter option leads to the
> > rollback of already prepared transactions?
>
> Indeed. I think this can be added for 0003. For now, it says like:
>
> ```
> postgres=3D# ALTER SUBSCRIPTION sub SET (TWO_PHASE =3D off, FORCE_ALTER =
=3D on);
> WARNING:  requested altering to two_phase =3D false but there are prepare=
d transactions done by the subscription
> DETAIL:  Such transactions are being rollbacked.
> ALTER SUBSCRIPTION
>

Is it possible to get a NOTICE instead of a WARNING?

>
> > I have another question on the latest 0001 patch:
> > + /*
> > + * Stop all the subscription workers, just in case.
> > + * Workers may still survive even if the subscription is
> > + * disabled.
> > + */
> > + logicalrep_workers_stop(subid);
> >
> > In which case the workers will survive when the subscription is disable=
d?
>
> I think both normal and tablesync worker can survive, because ALTER SUBSC=
RIPTION
> DISABLE command does not send signal to workers. It just change the syste=
m catalog.
> logicalrep_workers_stop() is added to ensure all workers are stopped.
>
> Actually, earlier version (-v3) did not have a mechanism but they sometim=
es got
> assertion failures in maybe_reread_subscription(). This was because the s=
urvived
> workers read pg_subscription catalog and failed below assertion:
>
> ```
>         /* two-phase cannot be altered while the worker exists */
>         Assert(newsub->twophasestate =3D=3D MySubscription->twophasestate=
);
> ```
>

But that is not a good reason for this operation to stop workers
first. Instead, we should prohibit this operation if any worker is
present. The reason is that there is always a chance that if any
worker is alive, it can prepare a new transaction after we have
checked for the presence of any prepared transactions.

Comments:
=3D=3D=3D=3D=3D=3D=3D=3D=3D
1.
There is no need to do something remarkable regarding
+ * the "false" to "true" case; the backend process alters
+ * subtwophase <funny_char> to LOGICALREP_TWOPHASE_STATE_PENDING once.
+ * After the subscription is enabled, a new logical
+ * replication worker requests to change the two_phase
+ * option of its slot from pending to true when the
+ * initial data synchronization is done. The code path is
+ * the same as the case in which two_phase <funny_char> is initially
+ * set <funny_char> to true.

The patch has some funny characters in the above comment at the places
highlighted by me. It seems you have copied from some editor that has
inserted such characters.

2.
/*
* Do not allow toggling of two_phase option. Doing so could cause
* missing of transactions and lead to an inconsistent replica.
* See comments atop worker.c
*
* Note: Unsupported twophase indicates that this call originated
* from AlterSubscription.
*/
if (!IsSet(supported_opts, SUBOPT_TWOPHASE_COMMIT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("unrecognized subscription parameter: \"%s\"", defel->defname)));

This part of the code must either be removed or converted to an assert.

3. The tests added in 099_twophase_added.pl should be part of 021_twophase.=
pl

--=20
With Regards,
Amit Kapila.