mirrors/postgres
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Improve parsing of options of CREATE/ALTER SUBSCRIPTION

Browse Source 
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
This commit is contained in:
Michael Paquier 2021-12-08 12:36:31 +09:00
parent f99870dd86
commit 00029deaf6
3 changed files with 35 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

73
src/backend/commands/subscriptioncmds.c
View File

@ -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 * Since not all options can be specified in both commands, this function
* will report an error if mutually exclusive options are specified. * will report an error if mutually exclusive options are specified.
*
* Caller is expected to have cleared 'opts'.
*/ */
static void static void
parse_subscription_options(ParseState *pstate, List *stmt_options, parse_subscription_options(ParseState *pstate, List *stmt_options,
@ -104,6 +102,9 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
{ {
ListCell *lc; ListCell *lc;
/* Start out with cleared opts. */
memset(opts, 0, sizeof(SubOpts));
/* caller must expect some option */ /* caller must expect some option */
Assert(supported_opts != 0); Assert(supported_opts != 0);
@ -262,7 +263,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
{ {
/* Check for incompatible options from the user. */ /* Check for incompatible options from the user. */
if (opts->enabled && if (opts->enabled &&
IsSet(supported_opts, SUBOPT_ENABLED) &&
IsSet(opts->specified_opts, SUBOPT_ENABLED)) IsSet(opts->specified_opts, SUBOPT_ENABLED))
ereport(ERROR, ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR), (errcode(ERRCODE_SYNTAX_ERROR),
@ -271,7 +271,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
"connect = false", "enabled = true"))); "connect = false", "enabled = true")));
if (opts->create_slot && if (opts->create_slot &&
IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT)) IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
ereport(ERROR, ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR), (errcode(ERRCODE_SYNTAX_ERROR),
@ -279,7 +278,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
"connect = false", "create_slot = true"))); "connect = false", "create_slot = true")));
if (opts->copy_data && if (opts->copy_data &&
IsSet(supported_opts, SUBOPT_COPY_DATA) &&
IsSet(opts->specified_opts, SUBOPT_COPY_DATA)) IsSet(opts->specified_opts, SUBOPT_COPY_DATA))
ereport(ERROR, ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR), (errcode(ERRCODE_SYNTAX_ERROR),
@ -297,44 +295,39 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
* was used. * was used.
*/ */
if (!opts->slot_name && if (!opts->slot_name &&
IsSet(supported_opts, SUBOPT_SLOT_NAME) &&
IsSet(opts->specified_opts, SUBOPT_SLOT_NAME)) IsSet(opts->specified_opts, SUBOPT_SLOT_NAME))
{ {
if (opts->enabled && if (opts->enabled)
IsSet(supported_opts, SUBOPT_ENABLED) && {
IsSet(opts->specified_opts, SUBOPT_ENABLED)) if (IsSet(opts->specified_opts, SUBOPT_ENABLED))
ereport(ERROR, ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR), (errcode(ERRCODE_SYNTAX_ERROR),
/*- translator: both %s are strings of the form "option = value" */ /*- translator: both %s are strings of the form "option = value" */
errmsg("%s and %s are mutually exclusive options", errmsg("%s and %s are mutually exclusive options",
"slot_name = NONE", "enabled = true"))); "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 && if (opts->create_slot)
IsSet(supported_opts, SUBOPT_CREATE_SLOT) && {
IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT)) if (IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
ereport(ERROR, ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR), (errcode(ERRCODE_SYNTAX_ERROR),
/*- translator: both %s are strings of the form "option = value" */ /*- translator: both %s are strings of the form "option = value" */
errmsg("%s and %s are mutually exclusive options", errmsg("%s and %s are mutually exclusive options",
"slot_name = NONE", "create_slot = true"))); "slot_name = NONE", "create_slot = true")));
else
if (opts->enabled && ereport(ERROR,
IsSet(supported_opts, SUBOPT_ENABLED) && (errcode(ERRCODE_SYNTAX_ERROR),
!IsSet(opts->specified_opts, SUBOPT_ENABLED)) /*- translator: both %s are strings of the form "option = value" */
ereport(ERROR, errmsg("subscription with %s must also set %s",
(errcode(ERRCODE_SYNTAX_ERROR), "slot_name = NONE", "create_slot = false")));
/*- 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")));
} }
} }

2
src/test/regress/expected/subscription.out
View File

@ -54,7 +54,7 @@ CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PU
ERROR: connect = false and create_slot = true are mutually exclusive options 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); 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 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 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); 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 ERROR: subscription with slot_name = NONE must also set enabled = false

2
src/test/regress/sql/subscription.sql
View File

@ -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, 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 (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, 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);
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, enabled = false);
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = false); CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);