Here are some review comments for patch v18-0002.
======
src/backend/commands/subscriptioncmds.c
1. CheckAlterSubOption
1a.
It's not obvious why we are only checking the 'slot name' when
needs_update==true, but OTOH is always checking the 'enabled' state.
~
1b.
Param 'needs_update' is a vague name. It needs more explanatory comments or
a better name. e.g. First impression was "Why are we calling 'Alter'
function if needs_update is false?". I know it encapsulates some common
code, but if special cases cause the logic to be more confusing then that
cost may outweigh the benefit of this function.
~
1c.
If the error checks can be moved to be done up-front, then all the
'needs_update' can be combined. Avoiding multiple checks to 'needs_update'
will make this function simpler.
~~~
AlterSubscription:
nitpick - typo /needs/need/
nitpick - typo /wo_phase/two_phase/
nitpick - The comment wording "the later part...", was confusing. I've
reworded the whole comment. But this belongs in patch 0001.
======
src/test/subscription/t/021_twophase.pl
nitpick - Use the same "###############################" comment style as
in patch 0001 to indicate each main TEST scenario.
======
99.
Please refer to the diffs attachment patch, which implements any nitpicks
mentioned above.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index d80d60c..5c6f0a5 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1262,7 +1262,7 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
if (IsSet(opts.specified_opts, SUBOPT_FAILOVER))
{
/*
- * First mark the needs to alter the
replication slot.
+ * First, mark the need to alter the
replication slot.
* Failover option is controlled by
both the publisher (as
* a slot option) and the subscriber
(as a subscription
* option).
@@ -1280,7 +1280,7 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
if (IsSet(opts.specified_opts,
SUBOPT_TWOPHASE_COMMIT))
{
/*
- * First check the need to alter the
replication slot.
+ * First, check the need to alter the
replication slot.
* Two_phase option is controlled by
both the publisher
* (as a slot option) and the
subscriber (as a
* subscription option). The slot
option must be altered
@@ -1302,11 +1302,9 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
isTopLevel);
/*
- * If the wo_phase slot option must be
altered, this
- * cannot be altered with slot_name
simultaneously. The
- * latter part refers to the pre-set
slot name and tries
- * to modify the slot option, so
changing both does not
- * make sense.
+ * Modifying the two_phase slot option
requires a slot
+ * lookup by slot name, so changing the
slot name
+ * at the same time is not allowed.
*/
if (update_two_phase &&
IsSet(opts.specified_opts,
SUBOPT_SLOT_NAME))
diff --git a/src/test/subscription/t/021_twophase.pl
b/src/test/subscription/t/021_twophase.pl
index f56dff4..26b9e2c 100644
--- a/src/test/subscription/t/021_twophase.pl
+++ b/src/test/subscription/t/021_twophase.pl
@@ -423,9 +423,12 @@ $result = $node_subscriber->safe_psql('postgres',
"SELECT count(*) FROM pg_prepared_xacts;");
is($result, qq(0), 'should be no prepared transactions on subscriber');
-# Toggle the two_phase to "true" *before* the COMMIT PREPARED. Since we are the
-# special path for the case where both two_phase and failover are altered, it
-# is also set to "true".
+###############################
+# Toggle the two_phase to "true" before the COMMIT PREPARED.
+#
+# Since we are the special path for the case where both two_phase
+# and failover are altered, it is also set to "true".
+###############################
$node_subscriber->safe_psql('postgres',
"ALTER SUBSCRIPTION tap_sub_copy DISABLE;");
$node_subscriber->poll_query_until('postgres',
