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
