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

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