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: Amit Kapila <amit.kapila16@gmail.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-16T10:02:21Z
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

Hi, here are some review comments for patch v18-0001.

======
doc/src/sgml/protocol.sgml

nitpick - Although it is no fault of your patch, IMO it would be nicer for
the TWO_PHASE description (of CREATE REPLICATION SLOT) to also be in the
same consistent order as what you have (e.g. below FAILOVER). So I moved it.

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

LookupGXactBySubid:
nitpick - add a blank line before return

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

CommonChecksForFailoverAndTwophase:
nitpick - added Assert for the generic-looking "option" parameter name
nitpick - modified comment about transaction block

~~~

1. AlterSubscription
+ * Workers may still survive even if the subscription has
+ * been disabled. They may read the pg_subscription
+ * catalog and detect that the twophase parameter is
+ * updated, which causes the assertion failure. Ensure
+ * workers have already been exited to avoid it.

"which causes the assertion failure" -- what assertion failure is that? The
comment is not very clear.

~

nitpick - in comment /twophase/two_phase/
nitpick - typo /acoordingly/accordingly/

======
src/backend/replication/logical/launcher.c

logicalrep_workers_find:
nitpick - /require_lock/acquire_lock/
nitpick - take the Assert out of the else.

======
src/backend/replication/slot.c

nitpick - refactor the code to check (failover) only one time. See the
nitpicks attachment.

~

2. ParseAlterReplSlotOptions

nitpick -- IMO the ParseAlterReplSlotOptions(). function does more harm
than good here by adding the unnecessary complexity of messing around with
multiple parameters that are passed-by-reference. All this would be simpler
if it was just coded inline in the AlterReplicationSlot() function, which
is the only caller. I've refactored all this to demonstrate (see nitpicks
attachment)

======
src/include/replication/worker_internal.h

nitpick - /require_lock/acquire_lock/

======
src/test/regress/sql/subscription.sql

nitpick - tweak comments

======
src/test/subscription/t/021_twophase.pl

nitpick - change comment style to indicate each test part better.

======
99.
Please also see the attached diffs patch which implements any nitpicks
mentioned above.

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