Improve parsing of options of CREATE/ALTER SUBSCRIPTION

This simplifies the code so as it is not necessary anymore for the
caller of parse_subscription_options() to zero SubOpts, holding a
bitmaps of the provided options as well as the default/parsed option
values.  This also simplifies some checks related to the options
supported by a command when checking for incompatibilities.

While on it, the errors generated for unsupported combinations with
"slot_name = NONE" are reordered.  This may generate a different errors
compared to the previous major versions, but users have to go through
all those errors to get a correct command in this case when using
incorrect values for options "enabled" and "create\slot", so at the end
the resulting command would remain the same.

Author: Peter Smith
Reviewed-by: Nathan Bossart
Discussion: https://postgr.es/m/CAHut+PtXHfLgLHDDJ8ZN5f5Be_37mJoxpEsRg8LNmm4XCr06Rw@mail.gmail.com

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 9427e86fee1088940b39ec5611e17e58d4d2da3a..2b658080fee52457a3df8fb241fa3c4abbc6489a 100644 (file)
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -95,8 +95,6 @@ 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(ParseState *pstate, List *stmt_options,
@@ -104,6 +102,9 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 {
        ListCell   *lc;
 
+       /* Start out with cleared opts. */
+       memset(opts, 0, sizeof(SubOpts));
+
        /* caller must expect some option */
        Assert(supported_opts != 0);
 
@@ -262,7 +263,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
        {
                /* Check for incompatible options from the user. */
                if (opts->enabled &&
-                       IsSet(supported_opts, SUBOPT_ENABLED) &&
                        IsSet(opts->specified_opts, SUBOPT_ENABLED))
                        ereport(ERROR,
                                        (errcode(ERRCODE_SYNTAX_ERROR),
@@ -271,7 +271,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
                                                        "connect = false", "enabled = true")));
 
                if (opts->create_slot &&
-                       IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
                        IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
                        ereport(ERROR,
                                        (errcode(ERRCODE_SYNTAX_ERROR),
@@ -279,7 +278,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
                                                        "connect = false", "create_slot = true")));
 
                if (opts->copy_data &&
-                       IsSet(supported_opts, SUBOPT_COPY_DATA) &&
                        IsSet(opts->specified_opts, SUBOPT_COPY_DATA))
                        ereport(ERROR,
                                        (errcode(ERRCODE_SYNTAX_ERROR),
@@ -297,44 +295,39 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
         * was used.
         */
        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(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(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(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->enabled)
+               {
+                       if (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")));
+                       else
+                               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(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")));
+               if (opts->create_slot)
+               {
+                       if (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")));
+                       else
+                               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")));
+               }
        }
 }
 

diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 15a1ac6398b0ffc468288ccb90c7930854a04b93..80aae83562cb213465b362acda29ceef2be6c59f 100644 (file)
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -54,7 +54,7 @@ CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PU
 ERROR:  connect = false and create_slot = true are mutually exclusive options
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = true);
 ERROR:  slot_name = NONE and enabled = true are mutually exclusive options
-CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = true);
+CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false, create_slot = true);
 ERROR:  slot_name = NONE and create_slot = true are mutually exclusive options
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE);
 ERROR:  subscription with slot_name = NONE must also set enabled = false

diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 7faa935a2a7575b57012b5d47a937ad63a2c9b59..bd0f4af1e490d9d392f00136b233dd93a6d12790 100644 (file)
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -43,7 +43,7 @@ CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PU
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, enabled = true);
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, create_slot = true);
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = true);
-CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = true);
+CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false, create_slot = true);
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE);
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false);
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);