Hi, here are my review comments for patch v19-0002.
======
src/backend/commands/subscriptioncmds.c
CheckAlterSubOption:
nitpick - tweak some comment wording
~
On Wed, Jul 17, 2024 at 3:13 PM Hayato Kuroda (Fujitsu)
<kuroda.hay...@fujitsu.com> wrote:
>
> > 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.
>
> This style was needed to preserve error condition for failover option. Not
> changed.
>
nitpick - Hmm. I think you might be trying to preserve the ordering of
errors when that order is of no consequence. AFAICT which error comes
first here is neither documented nor regression tested. e.g.
reordering them gives no problem for testing, but OTOH reordering them
does simplify the code. Anyway, I have modified the code in my
attached nitpicks diffs to demonstrate this suggestion in case you
change your mind.
~~~
AlterSubscription:
nitpick - let's keep all the variables called 'update_xxx' together.
nitpick - comment typo /needs/need/
nitpick - tweak some comment wording
======
src/test/subscription/t/021_twophase.pl
nitpick - didn't quite understand the "Since we are..." comment. I
reworded it according to what I thought was the intention.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index f770e9c..a826be9 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1079,17 +1079,6 @@ CheckAlterSubOption(Subscription *sub, const char
*option,
strcmp(option, "two_phase") == 0);
/*
- * If the slot needs to be updated, the backend must connect to the
- * publisher and request the alteration. slot_name must be required at
that
- * time.
- */
- if (slot_needs_update && !sub->slotname)
- ereport(ERROR,
-
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("cannot set %s for a subscription that
does not have a slot name",
- option)));
-
- /*
* Do not allow changing the option if the subscription is enabled. This
* is because both failover and two_phase options of the slot on the
* publisher cannot be modified if the slot is currently acquired by the
@@ -1101,17 +1090,27 @@ CheckAlterSubOption(Subscription *sub, const char
*option,
errmsg("cannot set %s for enabled
subscription",
option)));
- /*
- * The changed option of the slot can't be rolled back, so disallow if
we
- * are in a transaction block.
- */
if (slot_needs_update)
{
StringInfoData cmd;
+ /*
+ * If the slot needs to be updated, the backend must connect to
the
+ * publisher and request the alteration. A slot_name is
required at
+ * that time.
+ */
+ if (!sub->slotname)
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot set %s for a
subscription that does not have a slot name",
+ option)));
+
+ /*
+ * The changed option of the slot can't be rolled back, so
disallow if we
+ * are in a transaction block.
+ */
initStringInfo(&cmd);
appendStringInfo(&cmd, "ALTER SUBSCRIPTION ... SET (%s)",
option);
-
PreventInTransactionBlock(isTopLevel, cmd.data);
pfree(cmd.data);
}
@@ -1132,12 +1131,12 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
HeapTuple tup;
Oid subid;
bool update_tuple = false;
+ bool update_failover = false;
+ bool update_two_phase = false;
Subscription *sub;
Form_pg_subscription form;
bits32 supported_opts;
SubOpts opts = {0};
- bool update_failover = false;
- bool update_two_phase = false;
rel = table_open(SubscriptionRelationId, RowExclusiveLock);
@@ -1271,7 +1270,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).
@@ -1295,15 +1294,14 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
* subscription option). The slot
option must be altered
* only when changing "true" to "false".
*
- * There is no need to do something
remarkable regarding
+ * There is no need to do anything
remarkable for
* the "false" to "true" case; the
backend process alters
* subtwophase to
LOGICALREP_TWOPHASE_STATE_PENDING once.
* After the subscription is enabled, a
new logical
* replication worker requests to
change the two_phase
* option of its slot from pending to
true when the
- * initial data synchronization is
done. The code path is
- * the same as the case in which
two_phase is initially
- * set to true.
+ * initial data synchronization is
done. That code path is
+ * the same as when two_phase is
initially set to true.
*/
update_two_phase = !opts.twophase;
diff --git a/src/test/subscription/t/021_twophase.pl
b/src/test/subscription/t/021_twophase.pl
index c8fb3b7..91e2e17 100644
--- a/src/test/subscription/t/021_twophase.pl
+++ b/src/test/subscription/t/021_twophase.pl
@@ -428,9 +428,8 @@ 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".
+# Also, set failover to "true" to test the code path where
+# both two_phase and failover are altered at the same time.
###############################
$node_subscriber->safe_psql('postgres',
"ALTER SUBSCRIPTION tap_sub_copy DISABLE;");
