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: Amit Kapila <amit.kapila16@gmail.com>
Cc: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>, Vitaly Davydov <v.davydov@postgrespro.ru>, Ajin Cherian <itsajin@gmail.com>
Date: 2024-07-19T02:36:04Z
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

Attachments

On Thu, Jul 18, 2024 at 9:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
...
> I agree and have done that in the attached. I have made some
> additional changes: (a) removed the unrelated change of two_phase in
> protocol.sgml, (b) tried to make the two_phase change before failover
> option wherever it makes sense to keep the code consistent, (c)
> changed/added comments and doc changes at various places.
>
> I'll continue my review and testing of the patch but I thought of
> sharing what I have done till now.
>

Here some minor comments for patch v21

======
You wrote "tried to make the two_phase change before failover option
wherever it makes sense to keep the code consistent". But, still
failover is coded first in lots of places:
- libpqrcv_alter_slot
- ReplicationSlotAlter
- AlterReplicationSlot
etc.

Q. Why not change those ones?

======
src/backend/access/transam/twophase.c

IsTwoPhaseTransactionGidForSubid:
nitpick - nicer to rename the temporary gid variable: /gid_generated/gid_tmp/

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

CheckAlterSubOption:
nitpick = function comment period/plural.
nitpick - typo /Samilar/Similar/

======
src/include/replication/slot.h

1.
-extern void ReplicationSlotAlter(const char *name, bool failover);
+extern void ReplicationSlotAlter(const char *name, bool *failover,
+ bool *two_phase);

Use const?

======
99.
Please see attached diffs implementing the nitpicks mentioned above

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