From 1a00c0ef5368bb7b8ddcb3cf279df36577918ac4 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 4 Oct 2011 12:36:18 -0400 Subject: [PATCH] Remove the custom_variable_classes parameter. This variable provides only marginal error-prevention capability (since it can only check the prefix of a qualified GUC name), and the consensus is that that isn't worth the amount of hassle that maintaining the setting creates for DBAs. So, let's just remove it. With this commit, the system will silently accept a value for any qualified GUC name at all, whether it has anything to do with any known extension or not. (Unqualified names still have to match known built-in settings, though; and you will get a WARNING at extension load time if there's an unrecognized setting with that extension's prefix.) There's still some discussion ongoing about whether to tighten that up and if so how; but if we do come up with a solution, it's not likely to look anything like custom_variable_classes. --- doc/src/sgml/auth-delay.sgml | 6 +- doc/src/sgml/auto-explain.sgml | 6 +- doc/src/sgml/config.sgml | 57 ++----- doc/src/sgml/pgstatstatements.sgml | 7 +- doc/src/sgml/plperl.sgml | 4 - doc/src/sgml/plpgsql.sgml | 7 +- src/backend/utils/misc/guc-file.l | 114 ++----------- src/backend/utils/misc/guc.c | 159 ++---------------- src/backend/utils/misc/postgresql.conf.sample | 2 +- src/pl/plperl/expected/plperl_init.out | 2 +- src/pl/plperl/expected/plperl_shared.out | 2 +- src/pl/plperl/expected/plperlu.out | 2 +- src/pl/plperl/sql/plperl_init.sql | 2 +- src/pl/plperl/sql/plperl_shared.sql | 2 +- src/pl/plperl/sql/plperlu.sql | 2 +- 15 files changed, 51 insertions(+), 323 deletions(-) diff --git a/doc/src/sgml/auth-delay.sgml b/doc/src/sgml/auth-delay.sgml index e377c980ca..91549ffe4f 100644 --- a/doc/src/sgml/auth-delay.sgml +++ b/doc/src/sgml/auth-delay.sgml @@ -42,16 +42,14 @@ - In order to set these parameters in your postgresql.conf file, - you will need to add auth_delay to - . Typical usage might be: + These parameters must be set in postgresql.conf. + Typical usage might be: # postgresql.conf shared_preload_libraries = 'auth_delay' -custom_variable_classes = 'auth_delay' auth_delay.milliseconds = '500' diff --git a/doc/src/sgml/auto-explain.sgml b/doc/src/sgml/auto-explain.sgml index b16f9064ff..6a8da566fb 100644 --- a/doc/src/sgml/auto-explain.sgml +++ b/doc/src/sgml/auto-explain.sgml @@ -158,16 +158,14 @@ LOAD 'auto_explain'; - In order to set these parameters in your postgresql.conf file, - you will need to add auto_explain to - . Typical usage might be: + These parameters must be set in postgresql.conf. + Typical usage might be: # postgresql.conf shared_preload_libraries = 'auto_explain' -custom_variable_classes = 'auto_explain' auto_explain.log_min_duration = '3s' diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 3282ab4f20..fbcd455694 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -5940,58 +5940,25 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' This feature was designed to allow parameters not normally known to PostgreSQL to be added by add-on modules - (such as procedural languages). This allows add-on modules to be + (such as procedural languages). This allows extension modules to be configured in the standard ways. - - - - custom_variable_classes (string) - - custom_variable_classes configuration parameter - - - - This variable specifies one or several class names to be used for - custom variables, in the form of a comma-separated list. A custom - variable is a variable not normally known - to PostgreSQL proper but used by some - add-on module. Such variables must have names consisting of a class - name, a dot, and a variable name. custom_variable_classes - specifies all the class names in use in a particular installation. - This parameter can only be set in the postgresql.conf - file or on the server command line. - - - - - - - The difficulty with setting custom variables in - postgresql.conf is that the file must be read before add-on - modules have been loaded, and so custom variables would ordinarily be - rejected as unknown. When custom_variable_classes is set, - the server will accept definitions of arbitrary variables within each - specified class. These variables will be treated as placeholders and - will have no function until the module that defines them is loaded. When a - module for a specific class is loaded, it will add the proper variable - definitions for its class name, convert any placeholder - values according to those definitions, and issue warnings for any - unrecognized placeholders of its class that remain. + Custom options have two-part names: an extension name, then a dot, then + the parameter name proper, much like qualified names in SQL. An example + is plpgsql.variable_conflict. - Here is an example of what postgresql.conf might contain - when using custom variables: - - -custom_variable_classes = 'plpgsql,plperl' -plpgsql.variable_conflict = use_variable -plperl.use_strict = true -plruby.use_strict = true # generates error: unknown class name - + Because custom options may need to be set in processes that have not + loaded the relevant extension module, PostgreSQL + will accept a setting for any two-part parameter name. Such variables + are treated as placeholders and have no function until the module that + defines them is loaded. When an extension module is loaded, it will add + its variable definitions, convert any placeholder values according to + those definitions, and issue warnings for any unrecognized placeholders + that begin with its extension name. diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml index 52268c545d..5a0230c428 100644 --- a/doc/src/sgml/pgstatstatements.sgml +++ b/doc/src/sgml/pgstatstatements.sgml @@ -275,16 +275,13 @@ - In order to set any of these parameters in your - postgresql.conf file, - you will need to add pg_stat_statements to - . Typical usage might be: + These parameters must be set in postgresql.conf. + Typical usage might be: # postgresql.conf shared_preload_libraries = 'pg_stat_statements' -custom_variable_classes = 'pg_stat_statements' pg_stat_statements.max = 10000 pg_stat_statements.track = all diff --git a/doc/src/sgml/plperl.sgml b/doc/src/sgml/plperl.sgml index b957757da6..81b6de7adb 100644 --- a/doc/src/sgml/plperl.sgml +++ b/doc/src/sgml/plperl.sgml @@ -1219,10 +1219,6 @@ CREATE TRIGGER test_valid_id_trig This section lists configuration parameters that affect PL/Perl. - To set any of these parameters before PL/Perl has been loaded, - it is necessary to have added plperl to the - list in - postgresql.conf. diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml index 84fb012d1f..c14c34cd32 100644 --- a/doc/src/sgml/plpgsql.sgml +++ b/doc/src/sgml/plpgsql.sgml @@ -4007,11 +4007,8 @@ BEGIN use_column (where error is the factory default). This parameter affects subsequent compilations of statements in PL/pgSQL functions, but not statements - already compiled in the current session. To set the parameter before - PL/pgSQL has been loaded, it is necessary to have added - plpgsql to the list in - postgresql.conf. Because changing this setting + already compiled in the current session. + Because changing this setting can cause unexpected changes in the behavior of PL/pgSQL functions, it can only be changed by a superuser. diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l index a7cf0378dc..7728544c54 100644 --- a/src/backend/utils/misc/guc-file.l +++ b/src/backend/utils/misc/guc-file.l @@ -111,8 +111,6 @@ ProcessConfigFile(GucContext context) ConfigVariable *item, *head, *tail; - char *cvc = NULL; - struct config_string *cvc_struct; int i; /* @@ -138,50 +136,6 @@ ProcessConfigFile(GucContext context) goto cleanup_list; } - /* - * We need the proposed new value of custom_variable_classes to check - * custom variables with. ParseConfigFile ensured that if it's in - * the file, it's first in the list. But first check to see if we - * have an active value from the command line, which should override - * the file in any case. (Since there's no relevant env var, the - * only possible nondefault sources are the file and ARGV.) - */ - cvc_struct = (struct config_string *) - find_option("custom_variable_classes", false, elevel); - Assert(cvc_struct); - if (cvc_struct->gen.reset_source > PGC_S_FILE) - { - cvc = guc_strdup(elevel, cvc_struct->reset_val); - if (cvc == NULL) - { - error = true; - goto cleanup_list; - } - } - else if (head != NULL && - guc_name_compare(head->name, "custom_variable_classes") == 0) - { - /* - * Need to canonicalize the value by calling the check hook. - */ - void *extra = NULL; - - cvc = guc_strdup(elevel, head->value); - if (cvc == NULL) - { - error = true; - goto cleanup_list; - } - if (!call_string_check_hook(cvc_struct, &cvc, &extra, - PGC_S_FILE, elevel)) - { - error = true; - goto cleanup_list; - } - if (extra) - free(extra); - } - /* * Mark all extant GUC variables as not present in the config file. * We need this so that we can tell below which ones have been removed @@ -199,39 +153,29 @@ ProcessConfigFile(GucContext context) * quasi-syntactic check on the validity of the config file. It is * important that the postmaster and all backends agree on the results * of this phase, else we will have strange inconsistencies about which - * processes accept a config file update and which don't. Hence, custom - * variable names can only be checked against custom_variable_classes, - * not against any loadable modules that might (or might not) be present. - * Likewise, we don't attempt to validate the options' values here. + * processes accept a config file update and which don't. Hence, unknown + * custom variable names have to be accepted without complaint. For the + * same reason, we don't attempt to validate the options' values here. * * In addition, the GUC_IS_IN_FILE flag is set on each existing GUC * variable mentioned in the file. */ for (item = head; item; item = item->next) { - char *sep = strchr(item->name, GUC_QUALIFIER_SEPARATOR); struct config_generic *record; - if (sep) - { - /* Custom variable, so check against custom_variable_classes */ - if (!is_custom_class(item->name, sep - item->name, cvc)) - { - ereport(elevel, - (errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("unrecognized configuration parameter \"%s\" in file \"%s\" line %u", - item->name, - item->filename, item->sourceline))); - error = true; - continue; - } - } - + /* + * Try to find the variable; but do not create a custom placeholder + * if it's not there already. + */ record = find_option(item->name, false, elevel); if (record) + { + /* Found, so mark it as present in file */ record->status |= GUC_IS_IN_FILE; - else if (!sep) + } + else if (strchr(item->name, GUC_QUALIFIER_SEPARATOR) == NULL) { /* Invalid non-custom variable, so complain */ ereport(elevel, @@ -382,8 +326,6 @@ ProcessConfigFile(GucContext context) cleanup_list: FreeConfigVariables(head); - if (cvc) - free(cvc); if (error) { @@ -581,40 +523,6 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel, pfree(opt_name); pfree(opt_value); } - else if (guc_name_compare(opt_name, "custom_variable_classes") == 0) - { - /* - * This variable must be processed first as it controls - * the validity of other variables; so it goes at the head - * of the result list. If we already found a value for it, - * replace with this one. - */ - item = *head_p; - if (item != NULL && - guc_name_compare(item->name, "custom_variable_classes") == 0) - { - /* replace existing head item */ - pfree(item->name); - pfree(item->value); - item->name = opt_name; - item->value = opt_value; - item->filename = pstrdup(config_file); - item->sourceline = ConfigFileLineno-1; - } - else - { - /* prepend to list */ - item = palloc(sizeof *item); - item->name = opt_name; - item->value = opt_value; - item->filename = pstrdup(config_file); - item->sourceline = ConfigFileLineno-1; - item->next = *head_p; - *head_p = item; - if (*tail_p == NULL) - *tail_p = item; - } - } else { /* ordinary variable, append to list */ diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index c5b14522d5..2fd4867d25 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -178,7 +178,6 @@ static void assign_syslog_ident(const char *newval, void *extra); static void assign_session_replication_role(int newval, void *extra); static bool check_temp_buffers(int *newval, void **extra, GucSource source); static bool check_phony_autocommit(bool *newval, void **extra, GucSource source); -static bool check_custom_variable_classes(char **newval, void **extra, GucSource source); static bool check_debug_assertions(bool *newval, void **extra, GucSource source); static bool check_bonjour(bool *newval, void **extra, GucSource source); static bool check_ssl(bool *newval, void **extra, GucSource source); @@ -467,7 +466,6 @@ static char *log_timezone_string; static char *timezone_abbreviations_string; static char *XactIsoLevel_string; static char *session_authorization_string; -static char *custom_variable_classes; static int max_function_args; static int max_index_keys; static int max_identifier_length; @@ -2885,17 +2883,6 @@ static struct config_string ConfigureNamesString[] = NULL, NULL, NULL }, - { - {"custom_variable_classes", PGC_SIGHUP, CUSTOM_OPTIONS, - gettext_noop("Sets the list of known custom variable classes."), - NULL, - GUC_LIST_INPUT | GUC_LIST_QUOTE - }, - &custom_variable_classes, - NULL, - check_custom_variable_classes, NULL, NULL - }, - { {"data_directory", PGC_POSTMASTER, FILE_LOCATIONS, gettext_noop("Sets the server's data directory."), @@ -3623,8 +3610,7 @@ add_guc_variable(struct config_generic * var, int elevel) } /* - * Create and add a placeholder variable. It's presumed to belong - * to a valid custom variable class at this point. + * Create and add a placeholder variable for a custom variable name. */ static struct config_generic * add_placeholder_variable(const char *name, int elevel) @@ -3669,42 +3655,6 @@ add_placeholder_variable(const char *name, int elevel) return gen; } -/* - * Detect whether the portion of "name" before dotPos matches any custom - * variable class name listed in custom_var_classes. The latter must be - * formatted the way that assign_custom_variable_classes does it, ie, - * no whitespace. NULL is valid for custom_var_classes. - */ -static bool -is_custom_class(const char *name, int dotPos, const char *custom_var_classes) -{ - bool result = false; - const char *ccs = custom_var_classes; - - if (ccs != NULL) - { - const char *start = ccs; - - for (;; ++ccs) - { - char c = *ccs; - - if (c == '\0' || c == ',') - { - if (dotPos == ccs - start && strncmp(start, name, dotPos) == 0) - { - result = true; - break; - } - if (c == '\0') - break; - start = ccs + 1; - } - } - } - return result; -} - /* * Look up option NAME. If it exists, return a pointer to its record, * else return NULL. If create_placeholders is TRUE, we'll create a @@ -3745,13 +3695,9 @@ find_option(const char *name, bool create_placeholders, int elevel) if (create_placeholders) { /* - * Check if the name is qualified, and if so, check if the qualifier - * matches any custom variable class. If so, add a placeholder. + * Check if the name is qualified, and if so, add a placeholder. */ - const char *dot = strchr(name, GUC_QUALIFIER_SEPARATOR); - - if (dot != NULL && - is_custom_class(name, dot - name, custom_variable_classes)) + if (strchr(name, GUC_QUALIFIER_SEPARATOR) != NULL) return add_placeholder_variable(name, elevel); } @@ -7406,7 +7352,6 @@ write_nondefault_variables(GucContext context) { int elevel; FILE *fp; - struct config_generic *cvc_conf; int i; Assert(context == PGC_POSTMASTER || context == PGC_SIGHUP); @@ -7426,20 +7371,9 @@ write_nondefault_variables(GucContext context) return; } - /* - * custom_variable_classes must be written out first; otherwise we might - * reject custom variable values while reading the file. - */ - cvc_conf = find_option("custom_variable_classes", false, ERROR); - if (cvc_conf) - write_one_nondefault_variable(fp, cvc_conf); - for (i = 0; i < num_guc_variables; i++) { - struct config_generic *gconf = guc_variables[i]; - - if (gconf != cvc_conf) - write_one_nondefault_variable(fp, gconf); + write_one_nondefault_variable(fp, guc_variables[i]); } if (FreeFile(fp)) @@ -7886,20 +7820,15 @@ validate_option_array_item(const char *name, const char *value, * permissions normally (ie, allow if variable is USERSET, or if it's * SUSET and user is superuser). * - * name is not known, but exists or can be created as a placeholder - * (implying it has a prefix listed in custom_variable_classes). We allow - * this case if you're a superuser, otherwise not. Superusers are assumed - * to know what they're doing. We can't allow it for other users, because - * when the placeholder is resolved it might turn out to be a SUSET - * variable; define_custom_variable assumes we checked that. + * name is not known, but exists or can be created as a placeholder (i.e., + * it has a prefixed name). We allow this case if you're a superuser, + * otherwise not. Superusers are assumed to know what they're doing. + * We can't allow it for other users, because when the placeholder is + * resolved it might turn out to be a SUSET variable; + * define_custom_variable assumes we checked that. * * name is not known and can't be created as a placeholder. Throw error, - * unless skipIfNoPermissions is true, in which case return FALSE. (It's - * tempting to allow this case to superusers, if the name is qualified but - * not listed in custom_variable_classes. That would ease restoring of - * dumps containing ALTER ROLE/DATABASE SET. However, it's not clear that - * this usage justifies such a loss of error checking. You can always fix - * custom_variable_classes before you restore.) + * unless skipIfNoPermissions is true, in which case return FALSE. */ gconf = find_option(name, true, WARNING); if (!gconf) @@ -7909,7 +7838,8 @@ validate_option_array_item(const char *name, const char *value, return false; ereport(ERROR, (errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("unrecognized configuration parameter \"%s\"", name))); + errmsg("unrecognized configuration parameter \"%s\"", + name))); } if (gconf->flags & GUC_CUSTOM_PLACEHOLDER) @@ -8262,69 +8192,6 @@ check_phony_autocommit(bool *newval, void **extra, GucSource source) return true; } -static bool -check_custom_variable_classes(char **newval, void **extra, GucSource source) -{ - /* - * Check syntax. newval must be a comma separated list of identifiers. - * Whitespace is allowed but removed from the result. - */ - bool hasSpaceAfterToken = false; - const char *cp = *newval; - int symLen = 0; - char c; - StringInfoData buf; - - /* Default NULL is OK */ - if (cp == NULL) - return true; - - initStringInfo(&buf); - while ((c = *cp++) != '\0') - { - if (isspace((unsigned char) c)) - { - if (symLen > 0) - hasSpaceAfterToken = true; - continue; - } - - if (c == ',') - { - if (symLen > 0) /* terminate identifier */ - { - appendStringInfoChar(&buf, ','); - symLen = 0; - } - hasSpaceAfterToken = false; - continue; - } - - if (hasSpaceAfterToken || !(isalnum((unsigned char) c) || c == '_')) - { - /* - * Syntax error due to token following space after token or - * non-identifier character - */ - pfree(buf.data); - return false; - } - appendStringInfoChar(&buf, c); - symLen++; - } - - /* Remove stray ',' at end */ - if (symLen == 0 && buf.len > 0) - buf.data[--buf.len] = '\0'; - - /* GUC wants the result malloc'd */ - free(*newval); - *newval = guc_strdup(LOG, buf.data); - - pfree(buf.data); - return true; -} - static bool check_debug_assertions(bool *newval, void **extra, GucSource source) { diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index a18f14ae25..5bb7e7117b 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -560,4 +560,4 @@ # CUSTOMIZED OPTIONS #------------------------------------------------------------------------------ -#custom_variable_classes = '' # list of custom variable class names +# Add settings for extensions here diff --git a/src/pl/plperl/expected/plperl_init.out b/src/pl/plperl/expected/plperl_init.out index 4a04dbd6f3..e8a8e9bd83 100644 --- a/src/pl/plperl/expected/plperl_init.out +++ b/src/pl/plperl/expected/plperl_init.out @@ -1,5 +1,5 @@ -- test plperl.on_plperl_init errors are fatal --- Avoid need for custom_variable_classes = 'plperl' +-- Must load plperl before we can set on_plperl_init LOAD 'plperl'; SET SESSION plperl.on_plperl_init = ' system("/nonesuch") '; SHOW plperl.on_plperl_init; diff --git a/src/pl/plperl/expected/plperl_shared.out b/src/pl/plperl/expected/plperl_shared.out index 1a6bf5ee4d..67478ab454 100644 --- a/src/pl/plperl/expected/plperl_shared.out +++ b/src/pl/plperl/expected/plperl_shared.out @@ -1,6 +1,6 @@ -- test plperl.on_plperl_init via the shared hash -- (must be done before plperl is first used) --- Avoid need for custom_variable_classes = 'plperl' +-- Must load plperl before we can set on_plperl_init LOAD 'plperl'; -- testing on_plperl_init gets run, and that it can alter %_SHARED SET plperl.on_plperl_init = '$_SHARED{on_init} = 42'; diff --git a/src/pl/plperl/expected/plperlu.out b/src/pl/plperl/expected/plperlu.out index 25ac007b7a..1ba07eed9d 100644 --- a/src/pl/plperl/expected/plperlu.out +++ b/src/pl/plperl/expected/plperlu.out @@ -1,6 +1,6 @@ -- Use ONLY plperlu tests here. For plperl/plerlu combined tests -- see plperl_plperlu.sql --- Avoid need for custom_variable_classes = 'plperl' +-- Must load plperl before we can set on_plperlu_init LOAD 'plperl'; -- Test plperl.on_plperlu_init gets run SET plperl.on_plperlu_init = '$_SHARED{init} = 42'; diff --git a/src/pl/plperl/sql/plperl_init.sql b/src/pl/plperl/sql/plperl_init.sql index f6a32b9bae..51ac9192bd 100644 --- a/src/pl/plperl/sql/plperl_init.sql +++ b/src/pl/plperl/sql/plperl_init.sql @@ -1,6 +1,6 @@ -- test plperl.on_plperl_init errors are fatal --- Avoid need for custom_variable_classes = 'plperl' +-- Must load plperl before we can set on_plperl_init LOAD 'plperl'; SET SESSION plperl.on_plperl_init = ' system("/nonesuch") '; diff --git a/src/pl/plperl/sql/plperl_shared.sql b/src/pl/plperl/sql/plperl_shared.sql index d367d32ff0..d2fa8cbf93 100644 --- a/src/pl/plperl/sql/plperl_shared.sql +++ b/src/pl/plperl/sql/plperl_shared.sql @@ -1,7 +1,7 @@ -- test plperl.on_plperl_init via the shared hash -- (must be done before plperl is first used) --- Avoid need for custom_variable_classes = 'plperl' +-- Must load plperl before we can set on_plperl_init LOAD 'plperl'; -- testing on_plperl_init gets run, and that it can alter %_SHARED diff --git a/src/pl/plperl/sql/plperlu.sql b/src/pl/plperl/sql/plperlu.sql index 125691e5f7..831b8f4460 100644 --- a/src/pl/plperl/sql/plperlu.sql +++ b/src/pl/plperl/sql/plperlu.sql @@ -1,7 +1,7 @@ -- Use ONLY plperlu tests here. For plperl/plerlu combined tests -- see plperl_plperlu.sql --- Avoid need for custom_variable_classes = 'plperl' +-- Must load plperl before we can set on_plperlu_init LOAD 'plperl'; -- Test plperl.on_plperlu_init gets run