I find the business with OPT_NONE a bit uselessly verbose. It's like we
haven't completely made up our minds that zero means no options set.
Wouldn't it be simpler to remove that #define and leave the variable
uninitialized until we want to set the options we want, and then use
plain assignment instead of |= ?
I propose the attached cleanup. Some comments seem a bit too obvious;
the use of a local variable for specified_opts instead of directly
assigning to the one in the struct seemed unnecessary; places that call
parse_subscription_options() with only one bit set don't need a separate
variable for the allowed options; added some whitespace.
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index edfef9f14f..eb88d877a5 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -50,16 +50,15 @@
* Options that can be specified by the user in CREATE/ALTER SUBSCRIPTION
* command.
*/
-#define SUBOPT_NONE 0x00000000
-#define SUBOPT_CONNECT 0x00000001
-#define SUBOPT_ENABLED 0x00000002
-#define SUBOPT_CREATE_SLOT 0x00000004
-#define SUBOPT_SLOT_NAME 0x00000008
-#define SUBOPT_COPY_DATA 0x00000010
-#define SUBOPT_SYNCHRONOUS_COMMIT 0x00000020
-#define SUBOPT_REFRESH 0x00000040
-#define SUBOPT_BINARY 0x00000080
-#define SUBOPT_STREAMING 0x00000100
+#define SUBOPT_CONNECT 0x00000001
+#define SUBOPT_ENABLED 0x00000002
+#define SUBOPT_CREATE_SLOT 0x00000004
+#define SUBOPT_SLOT_NAME 0x00000008
+#define SUBOPT_COPY_DATA 0x00000010
+#define SUBOPT_SYNCHRONOUS_COMMIT 0x00000020
+#define SUBOPT_REFRESH 0x00000040
+#define SUBOPT_BINARY 0x00000080
+#define SUBOPT_STREAMING 0x00000100
/* check if the 'val' has 'bits' set */
#define IsSet(val, bits) (((val) & (bits)) == (bits))
@@ -93,48 +92,35 @@ static void ReportSlotConnectionError(List *rstates, Oid subid, char *slotname,
*
* Since not all options can be specified in both commands, this function
* will report an error if mutually exclusive options are specified.
+ *
+ * Caller is expected to have cleared 'opts'.
*/
static void
parse_subscription_options(List *stmt_options, bits32 supported_opts, SubOpts *opts)
{
ListCell *lc;
- bits32 specified_opts;
-
- specified_opts = SUBOPT_NONE;
/* caller must expect some option */
- Assert(supported_opts != SUBOPT_NONE);
+ Assert(supported_opts != 0);
- /* If connect option is supported, the others also need to be. */
+ /* If connect option is supported, these others also need to be. */
Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
IsSet(supported_opts, SUBOPT_ENABLED | SUBOPT_CREATE_SLOT |
SUBOPT_COPY_DATA));
- /* Set default values for the supported options. */
+ /* Set default values for the boolean supported options. */
if (IsSet(supported_opts, SUBOPT_CONNECT))
opts->connect = true;
-
if (IsSet(supported_opts, SUBOPT_ENABLED))
opts->enabled = true;
-
if (IsSet(supported_opts, SUBOPT_CREATE_SLOT))
opts->create_slot = true;
-
- if (IsSet(supported_opts, SUBOPT_SLOT_NAME))
- opts->slot_name = NULL;
-
if (IsSet(supported_opts, SUBOPT_COPY_DATA))
opts->copy_data = true;
-
- if (IsSet(supported_opts, SUBOPT_SYNCHRONOUS_COMMIT))
- opts->synchronous_commit = NULL;
-
if (IsSet(supported_opts, SUBOPT_REFRESH))
opts->refresh = true;
-
if (IsSet(supported_opts, SUBOPT_BINARY))
opts->binary = false;
-
if (IsSet(supported_opts, SUBOPT_STREAMING))
opts->streaming = false;
@@ -146,45 +132,45 @@ parse_subscription_options(List *stmt_options, bits32 supported_opts, SubOpts *o
if (IsSet(supported_opts, SUBOPT_CONNECT) &&
strcmp(defel->defname, "connect") == 0)
{
- if (IsSet(specified_opts, SUBOPT_CONNECT))
+ if (IsSet(opts->specified_opts, SUBOPT_CONNECT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
- specified_opts |= SUBOPT_CONNECT;
+ opts->specified_opts |= SUBOPT_CONNECT;
opts->connect = defGetBoolean(defel);
}
else if (IsSet(supported_opts, SUBOPT_ENABLED) &&
strcmp(defel->defname, "enabled") == 0)
{
- if (IsSet(specified_opts, SUBOPT_ENABLED))
+ if (IsSet(opts->specified_opts, SUBOPT_ENABLED))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
- specified_opts |= SUBOPT_ENABLED;
+ opts->specified_opts |= SUBOPT_ENABLED;
opts->enabled = defGetBoolean(defel);
}
else if (IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
strcmp(defel->defname, "create_slot") == 0)
{
- if (IsSet(specified_opts, SUBOPT_CREATE_SLOT))
+ if (IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
- specified_opts |= SUBOPT_CREATE_SLOT;
+ opts->specified_opts |= SUBOPT_CREATE_SLOT;
opts->create_slot = defGetBoolean(defel);
}
else if (IsSet(supported_opts, SUBOPT_SLOT_NAME) &&
strcmp(defel->defname, "slot_name") == 0)
{
- if (IsSet(specified_opts, SUBOPT_SLOT_NAME))
+ if (IsSet(opts->specified_opts, SUBOPT_SLOT_NAME))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
- specified_opts |= SUBOPT_SLOT_NAME;
+ opts->specified_opts |= SUBOPT_SLOT_NAME;
opts->slot_name = defGetString(defel);
/* Setting slot_name = NONE is treated as no slot name. */
@@ -194,23 +180,23 @@ parse_subscription_options(List *stmt_options, bits32 supported_opts, SubOpts *o
else if (IsSet(supported_opts, SUBOPT_COPY_DATA) &&
strcmp(defel->defname, "copy_data") == 0)
{
- if (IsSet(specified_opts, SUBOPT_COPY_DATA))
+ if (IsSet(opts->specified_opts, SUBOPT_COPY_DATA))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
- specified_opts |= SUBOPT_COPY_DATA;
+ opts->specified_opts |= SUBOPT_COPY_DATA;
opts->copy_data = defGetBoolean(defel);
}
else if (IsSet(supported_opts, SUBOPT_SYNCHRONOUS_COMMIT) &&
strcmp(defel->defname, "synchronous_commit") == 0)
{
- if (IsSet(specified_opts, SUBOPT_SYNCHRONOUS_COMMIT))
+ if (IsSet(opts->specified_opts, SUBOPT_SYNCHRONOUS_COMMIT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
- specified_opts |= SUBOPT_SYNCHRONOUS_COMMIT;
+ opts->specified_opts |= SUBOPT_SYNCHRONOUS_COMMIT;
opts->synchronous_commit = defGetString(defel);
/* Test if the given value is valid for synchronous_commit GUC. */
@@ -221,34 +207,34 @@ parse_subscription_options(List *stmt_options, bits32 supported_opts, SubOpts *o
else if (IsSet(supported_opts, SUBOPT_REFRESH) &&
strcmp(defel->defname, "refresh") == 0)
{
- if (IsSet(specified_opts, SUBOPT_REFRESH))
+ if (IsSet(opts->specified_opts, SUBOPT_REFRESH))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
- specified_opts |= SUBOPT_REFRESH;
+ opts->specified_opts |= SUBOPT_REFRESH;
opts->refresh = defGetBoolean(defel);
}
else if (IsSet(supported_opts, SUBOPT_BINARY) &&
strcmp(defel->defname, "binary") == 0)
{
- if (IsSet(specified_opts, SUBOPT_BINARY))
+ if (IsSet(opts->specified_opts, SUBOPT_BINARY))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
- specified_opts |= SUBOPT_BINARY;
+ opts->specified_opts |= SUBOPT_BINARY;
opts->binary = defGetBoolean(defel);
}
else if (IsSet(supported_opts, SUBOPT_STREAMING) &&
strcmp(defel->defname, "streaming") == 0)
{
- if (IsSet(specified_opts, SUBOPT_STREAMING))
+ if (IsSet(opts->specified_opts, SUBOPT_STREAMING))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
- specified_opts |= SUBOPT_STREAMING;
+ opts->specified_opts |= SUBOPT_STREAMING;
opts->streaming = defGetBoolean(defel);
}
else
@@ -264,23 +250,26 @@ parse_subscription_options(List *stmt_options, bits32 supported_opts, SubOpts *o
if (!opts->connect && IsSet(supported_opts, SUBOPT_CONNECT))
{
/* Check for incompatible options from the user. */
- if (opts->enabled && IsSet(supported_opts, SUBOPT_ENABLED) &&
- IsSet(specified_opts, SUBOPT_ENABLED))
+ if (opts->enabled &&
+ IsSet(supported_opts, SUBOPT_ENABLED) &&
+ IsSet(opts->specified_opts, SUBOPT_ENABLED))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
/*- translator: both %s are strings of the form "option = value" */
errmsg("%s and %s are mutually exclusive options",
"connect = false", "enabled = true")));
- if (opts->create_slot && IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
- IsSet(specified_opts, SUBOPT_CREATE_SLOT))
+ if (opts->create_slot &&
+ IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+ IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s and %s are mutually exclusive options",
"connect = false", "create_slot = true")));
- if (opts->copy_data && IsSet(supported_opts, SUBOPT_COPY_DATA) &&
- IsSet(specified_opts, SUBOPT_COPY_DATA))
+ if (opts->copy_data &&
+ IsSet(supported_opts, SUBOPT_COPY_DATA) &&
+ IsSet(opts->specified_opts, SUBOPT_COPY_DATA))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s and %s are mutually exclusive options",
@@ -296,41 +285,46 @@ parse_subscription_options(List *stmt_options, bits32 supported_opts, SubOpts *o
* Do additional checking for disallowed combination when slot_name = NONE
* was used.
*/
- if (!opts->slot_name && IsSet(supported_opts, SUBOPT_SLOT_NAME) &&
- IsSet(specified_opts, SUBOPT_SLOT_NAME))
+ if (!opts->slot_name &&
+ IsSet(supported_opts, SUBOPT_SLOT_NAME) &&
+ IsSet(opts->specified_opts, SUBOPT_SLOT_NAME))
{
- if (opts->enabled && IsSet(supported_opts, SUBOPT_ENABLED) &&
- IsSet(specified_opts, SUBOPT_ENABLED))
+ if (opts->enabled &&
+ IsSet(supported_opts, SUBOPT_ENABLED) &&
+ IsSet(opts->specified_opts, SUBOPT_ENABLED))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
/*- translator: both %s are strings of the form "option = value" */
errmsg("%s and %s are mutually exclusive options",
"slot_name = NONE", "enabled = true")));
- if (opts->create_slot && IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
- IsSet(specified_opts, SUBOPT_CREATE_SLOT))
+ if (opts->create_slot &&
+ IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+ IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
+ /*- translator: both %s are strings of the form "option = value" */
errmsg("%s and %s are mutually exclusive options",
"slot_name = NONE", "create_slot = true")));
- if (opts->enabled && IsSet(supported_opts, SUBOPT_ENABLED) &&
- !IsSet(specified_opts, SUBOPT_ENABLED))
+ if (opts->enabled &&
+ IsSet(supported_opts, SUBOPT_ENABLED) &&
+ !IsSet(opts->specified_opts, SUBOPT_ENABLED))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
/*- translator: both %s are strings of the form "option = value" */
errmsg("subscription with %s must also set %s",
"slot_name = NONE", "enabled = false")));
- if (opts->create_slot && IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
- !IsSet(specified_opts, SUBOPT_CREATE_SLOT))
+ if (opts->create_slot &&
+ IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+ !IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
+ /*- translator: both %s are strings of the form "option = value" */
errmsg("subscription with %s must also set %s",
"slot_name = NONE", "create_slot = false")));
}
-
- opts->specified_opts = specified_opts;
}
/*
@@ -383,17 +377,15 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
bits32 supported_opts;
SubOpts opts = {0};
- /* Options that can be specified by CREATE SUBSCRIPTION command. */
- supported_opts = (SUBOPT_CONNECT | SUBOPT_ENABLED | SUBOPT_CREATE_SLOT |
- SUBOPT_SLOT_NAME | SUBOPT_COPY_DATA |
- SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
- SUBOPT_STREAMING);
-
/*
* Parse and check options.
*
* Connection and publication should not be specified here.
*/
+ supported_opts = (SUBOPT_CONNECT | SUBOPT_ENABLED | SUBOPT_CREATE_SLOT |
+ SUBOPT_SLOT_NAME | SUBOPT_COPY_DATA |
+ SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
+ SUBOPT_STREAMING);
parse_subscription_options(stmt->options, supported_opts, &opts);
/*
@@ -798,7 +790,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
bool update_tuple = false;
Subscription *sub;
Form_pg_subscription form;
- bits32 supported_opts = SUBOPT_NONE;
+ bits32 supported_opts;
SubOpts opts = {0};
rel = table_open(SubscriptionRelationId, RowExclusiveLock);
@@ -835,13 +827,9 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
{
case ALTER_SUBSCRIPTION_OPTIONS:
{
- /*
- * Options that can be specified by this ALTER SUBSCRIPTION
- * kind.
- */
- supported_opts |= (SUBOPT_SLOT_NAME |
- SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
- SUBOPT_STREAMING);
+ supported_opts = (SUBOPT_SLOT_NAME |
+ SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
+ SUBOPT_STREAMING);
parse_subscription_options(stmt->options, supported_opts, &opts);
@@ -888,13 +876,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
case ALTER_SUBSCRIPTION_ENABLED:
{
- /*
- * Option that can be specified by this ALTER SUBSCRIPTION
- * kind.
- */
- supported_opts |= SUBOPT_ENABLED;
-
- parse_subscription_options(stmt->options, supported_opts, &opts);
+ parse_subscription_options(stmt->options, SUBOPT_ENABLED, &opts);
Assert(IsSet(opts.specified_opts, SUBOPT_ENABLED));
if (!sub->slotname && opts.enabled)
@@ -927,12 +909,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
case ALTER_SUBSCRIPTION_SET_PUBLICATION:
{
- /*
- * Options that can be specified by this ALTER SUBSCRIPTION
- * kind.
- */
- supported_opts |= (SUBOPT_COPY_DATA | SUBOPT_REFRESH);
-
+ supported_opts = SUBOPT_COPY_DATA | SUBOPT_REFRESH;
parse_subscription_options(stmt->options, supported_opts, &opts);
values[Anum_pg_subscription_subpublications - 1] =
@@ -967,13 +944,9 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
List *publist;
bool isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
- /*
- * Options that can be specified by this ALTER SUBSCRIPTION
- * kind.
- */
+ supported_opts = SUBOPT_REFRESH;
if (isadd)
supported_opts |= SUBOPT_COPY_DATA;
- supported_opts |= SUBOPT_REFRESH;
parse_subscription_options(stmt->options, supported_opts, &opts);
@@ -1006,18 +979,12 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
case ALTER_SUBSCRIPTION_REFRESH:
{
- /*
- * Option that can be specified by this ALTER SUBSCRIPTION
- * kind.
- */
- supported_opts |= SUBOPT_COPY_DATA;
-
if (!sub->enabled)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled subscriptions")));
- parse_subscription_options(stmt->options, supported_opts, &opts);
+ parse_subscription_options(stmt->options, SUBOPT_COPY_DATA, &opts);
PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION ... REFRESH");
