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);