From 2ed8a8cc5b634d33ea07d681c6b02213da07f792 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 27 Dec 2021 14:35:50 -0500 Subject: [PATCH] Rethink handling of settings with a prefix reserved by an extension. Commit 75d22069e made SET print a warning if you tried to set an unrecognized parameter within namespace previously reserved by an extension. It seems better for that to be an outright error though, for the same reason that we don't let you set unrecognized unqualified parameter names. In any case, the preceding implementation was inefficient and erroneous. Perform the check in a more appropriate spot, and be more careful about prefix-match cases. Discussion: https://postgr.es/m/116024.1640111629@sss.pgh.pa.us --- src/backend/utils/misc/guc.c | 96 ++++++++++++++----------------- src/test/regress/expected/guc.out | 36 ++++++------ src/test/regress/sql/guc.sql | 20 +++---- 3 files changed, 68 insertions(+), 84 deletions(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index bff949a40bc..f345eceb5b7 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -148,6 +148,8 @@ extern bool optimize_bounded_sort; static int GUC_check_errcode_value; +static List *reserved_class_prefix = NIL; + /* global variables for check hook support */ char *GUC_check_errmsg_string; char *GUC_check_errdetail_string; @@ -235,8 +237,6 @@ static bool check_recovery_target_lsn(char **newval, void **extra, GucSource sou static void assign_recovery_target_lsn(const char *newval, void *extra); static bool check_primary_slot_name(char **newval, void **extra, GucSource source); static bool check_default_with_oids(bool *newval, void **extra, GucSource source); -static void check_reserved_prefixes(const char *varName); -static List *reserved_class_prefix = NIL; /* Private functions in guc-file.l that need to be called from guc.c */ static ConfigVariable *ProcessConfigFileInternal(GucContext context, @@ -5569,18 +5569,44 @@ find_option(const char *name, bool create_placeholders, bool skip_errors, * doesn't contain a separator, don't assume that it was meant to be a * placeholder. */ - if (strchr(name, GUC_QUALIFIER_SEPARATOR) != NULL) + const char *sep = strchr(name, GUC_QUALIFIER_SEPARATOR); + + if (sep != NULL) { - if (valid_custom_variable_name(name)) - return add_placeholder_variable(name, elevel); - /* A special error message seems desirable here */ - if (!skip_errors) - ereport(elevel, - (errcode(ERRCODE_INVALID_NAME), - errmsg("invalid configuration parameter name \"%s\"", - name), - errdetail("Custom parameter names must be two or more simple identifiers separated by dots."))); - return NULL; + size_t classLen = sep - name; + ListCell *lc; + + /* The name must be syntactically acceptable ... */ + if (!valid_custom_variable_name(name)) + { + if (!skip_errors) + ereport(elevel, + (errcode(ERRCODE_INVALID_NAME), + errmsg("invalid configuration parameter name \"%s\"", + name), + errdetail("Custom parameter names must be two or more simple identifiers separated by dots."))); + return NULL; + } + /* ... and it must not match any previously-reserved prefix */ + foreach(lc, reserved_class_prefix) + { + const char *rcprefix = lfirst(lc); + + if (strlen(rcprefix) == classLen && + strncmp(name, rcprefix, classLen) == 0) + { + if (!skip_errors) + ereport(elevel, + (errcode(ERRCODE_INVALID_NAME), + errmsg("invalid configuration parameter name \"%s\"", + name), + errdetail("\"%s\" is a reserved prefix.", + rcprefix))); + return NULL; + } + } + /* OK, create it */ + return add_placeholder_variable(name, elevel); } } @@ -8764,7 +8790,6 @@ ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel) (superuser() ? PGC_SUSET : PGC_USERSET), PGC_S_SESSION, action, true, 0, false); - check_reserved_prefixes(stmt->name); break; case VAR_SET_MULTI: @@ -8850,8 +8875,6 @@ ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel) (superuser() ? PGC_SUSET : PGC_USERSET), PGC_S_SESSION, action, true, 0, false); - - check_reserved_prefixes(stmt->name); break; case VAR_RESET_ALL: ResetAllOptions(); @@ -9345,8 +9368,9 @@ EmitWarningsOnPlaceholders(const char *className) { int classLen = strlen(className); int i; - MemoryContext oldcontext; + MemoryContext oldcontext; + /* Check for existing placeholders. */ for (i = 0; i < num_guc_variables; i++) { struct config_generic *var = guc_variables[i]; @@ -9362,48 +9386,12 @@ EmitWarningsOnPlaceholders(const char *className) } } + /* And remember the name so we can prevent future mistakes. */ oldcontext = MemoryContextSwitchTo(TopMemoryContext); reserved_class_prefix = lappend(reserved_class_prefix, pstrdup(className)); MemoryContextSwitchTo(oldcontext); } -/* - * Check a setting name against prefixes previously reserved by - * EmitWarningsOnPlaceholders() and throw a warning if matching. - */ -static void -check_reserved_prefixes(const char *varName) -{ - char *sep = strchr(varName, GUC_QUALIFIER_SEPARATOR); - - if (sep) - { - size_t classLen = sep - varName; - ListCell *lc; - - foreach(lc, reserved_class_prefix) - { - char *rcprefix = lfirst(lc); - - if (strncmp(varName, rcprefix, classLen) == 0) - { - for (int i = 0; i < num_guc_variables; i++) - { - struct config_generic *var = guc_variables[i]; - - if ((var->flags & GUC_CUSTOM_PLACEHOLDER) != 0 && - strcmp(varName, var->name) == 0) - { - ereport(WARNING, - (errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("unrecognized configuration parameter \"%s\"", var->name), - errdetail("\"%.*s\" is a reserved prefix.", (int) classLen, var->name))); - } - } - } - } - } -} /* * SHOW command diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out index 2433038c3ec..5ad7477f618 100644 --- a/src/test/regress/expected/guc.out +++ b/src/test/regress/expected/guc.out @@ -548,6 +548,23 @@ ERROR: invalid configuration parameter name "special.weird name" DETAIL: Custom parameter names must be two or more simple identifiers separated by dots. SHOW special."weird name"; ERROR: unrecognized configuration parameter "special.weird name" +-- Check what happens when you try to set a "custom" GUC within the +-- namespace of an extension. +SET plpgsql.bogus_setting = 42; -- allowed if plpgsql is not loaded yet +LOAD 'plpgsql'; -- this will now warn about it +WARNING: unrecognized configuration parameter "plpgsql.bogus_setting" +SET plpgsql.extra_foo_warnings = false; -- but now, it's an error +ERROR: invalid configuration parameter name "plpgsql.extra_foo_warnings" +DETAIL: "plpgsql" is a reserved prefix. +SHOW plpgsql.extra_foo_warnings; +ERROR: unrecognized configuration parameter "plpgsql.extra_foo_warnings" +SET plpgsql.bogus_setting = 43; -- you can still use the pre-existing variable +SHOW plpgsql.bogus_setting; + plpgsql.bogus_setting +----------------------- + 43 +(1 row) + -- -- Test DISCARD TEMP -- @@ -813,22 +830,3 @@ set default_with_oids to f; -- Should not allow to set it to true. set default_with_oids to t; ERROR: tables declared WITH OIDS are not supported --- test SET unrecognized parameter -SET foo = false; -- no such setting -ERROR: unrecognized configuration parameter "foo" --- test setting a parameter with a registered prefix (plpgsql) -SET plpgsql.extra_foo_warnings = false; -- no such setting -WARNING: unrecognized configuration parameter "plpgsql.extra_foo_warnings" -DETAIL: "plpgsql" is a reserved prefix. -SHOW plpgsql.extra_foo_warnings; -- but the parameter is set - plpgsql.extra_foo_warnings ----------------------------- - false -(1 row) - --- cleanup -RESET foo; -ERROR: unrecognized configuration parameter "foo" -RESET plpgsql.extra_foo_warnings; -WARNING: unrecognized configuration parameter "plpgsql.extra_foo_warnings" -DETAIL: "plpgsql" is a reserved prefix. diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql index b57758ed273..f97f4e44884 100644 --- a/src/test/regress/sql/guc.sql +++ b/src/test/regress/sql/guc.sql @@ -163,6 +163,15 @@ SHOW custom."bad-guc"; SET special."weird name" = 'foo'; -- could be allowed, but we choose not to SHOW special."weird name"; +-- Check what happens when you try to set a "custom" GUC within the +-- namespace of an extension. +SET plpgsql.bogus_setting = 42; -- allowed if plpgsql is not loaded yet +LOAD 'plpgsql'; -- this will now warn about it +SET plpgsql.extra_foo_warnings = false; -- but now, it's an error +SHOW plpgsql.extra_foo_warnings; +SET plpgsql.bogus_setting = 43; -- you can still use the pre-existing variable +SHOW plpgsql.bogus_setting; + -- -- Test DISCARD TEMP -- @@ -311,14 +320,3 @@ reset check_function_bodies; set default_with_oids to f; -- Should not allow to set it to true. set default_with_oids to t; - --- test SET unrecognized parameter -SET foo = false; -- no such setting - --- test setting a parameter with a registered prefix (plpgsql) -SET plpgsql.extra_foo_warnings = false; -- no such setting -SHOW plpgsql.extra_foo_warnings; -- but the parameter is set - --- cleanup -RESET foo; -RESET plpgsql.extra_foo_warnings;