From 00029deaf65aad47044d9290fe80f2f68601f7ac Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 8 Dec 2021 12:36:31 +0900 Subject: [PATCH] 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 --- src/backend/commands/subscriptioncmds.c | 73 ++++++++++------------ src/test/regress/expected/subscription.out | 2 +- src/test/regress/sql/subscription.sql | 2 +- 3 files changed, 35 insertions(+), 42 deletions(-) diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 9427e86fee..2b658080fe 100644 --- 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->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("%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->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 15a1ac6398..80aae83562 100644 --- 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 7faa935a2a..bd0f4af1e4 100644 --- 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);