I wrote:
> 0003 looks to me like it was superseded by 75d22069e. I do not
> particularly like that patch though; it seems both inefficient
> and ill-designed. 0003 is adding a check in an equally bizarre
> place. Why isn't add_placeholder_variable the place to deal
> with this? Also, once we know that a prefix is reserved,
> why don't we throw an error not just a warning for attempts to
> set some unknown variable under that prefix? We don't allow
> you to set unknown non-prefixed variables.
Concretely, I think we should do the attached, which removes much of
75d22069e and does the check at the point of placeholder creation.
(After looking closer, add_placeholder_variable's caller is the
function that is responsible for rejecting bad names, not
add_placeholder_variable itself.)
This also fixes an indisputable bug in 75d22069e, namely that it
did prefix comparisons incorrectly and would throw warnings for
cases it shouldn't. Reserving "plpgsql" shouldn't have the effect
of reserving "pl" as well.
I'm tempted to propose that we also rename EmitWarningsOnPlaceholders
to something like MarkGUCPrefixReserved, to more clearly reflect
what it does now. (We could provide the old name as a macro alias
to avoid breaking extensions needlessly.)
regards, tom lane
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index bff949a40b..f345eceb5b 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 2433038c3e..c50f3fe817 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -818,17 +818,11 @@ 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"
+ERROR: invalid configuration parameter name "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"
+ERROR: invalid configuration parameter name "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 b57758ed27..2f89974d8c 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -317,7 +317,6 @@ 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;
