Allow ALTER SYSTEM SET on unrecognized custom GUCs - Mailing list pgsql-hackers

Currently we have this odd behavior (for a superuser):

regression=# ALTER SYSTEM SET foo.bar TO 'baz';
ERROR:  unrecognized configuration parameter "foo.bar"
regression=# SET foo.bar TO 'baz';
SET
regression=# ALTER SYSTEM SET foo.bar TO 'baz';
ALTER SYSTEM

That is, you can't ALTER SYSTEM SET a random custom GUC unless there
is already a placeholder GUC for it, because the find_option call in
AlterSystemSetConfigFile fails.  This is surely pretty inconsistent.
Either the first ALTER SYSTEM SET ought to succeed, or the second one
ought to fail too, because we don't have any more knowledge about the
custom GUC than we did before.

In the original discussion about this [1], I initially leaned towards
"they should both fail", but I reconsidered: there doesn't seem to be
any harm in allowing ALTER SYSTEM SET to succeed for any custom GUC
name, as long as you're superuser.

Hence, attached is a patch for that.  Much of it is refactoring to
avoid duplicating the code that checks for a reserved GUC name, which
I think should still be done here --- otherwise, we're losing a lot of
the typo detection that that check was intended to provide.  (That is,
if you have loaded an extension that defines "foo" as a prefix, we
should honor the extension's opinion about whether "foo.bar" is
valid.)  I also fixed the code for GRANT ON PARAMETER so that it
follows the same rules and throws the same errors for invalid cases.

There's a chunk of AlterSystemSetConfigFile that now needs indenting
one more tab stop, but I didn't do that yet for ease of review.

Thoughts?

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/169746329791.169914.16613647309012285391%40wrigleys.postgresql.org

diff --git a/src/backend/catalog/pg_parameter_acl.c b/src/backend/catalog/pg_parameter_acl.c
index 073392e2c4..f4bc10bafe 100644
--- a/src/backend/catalog/pg_parameter_acl.c
+++ b/src/backend/catalog/pg_parameter_acl.c
@@ -82,11 +82,7 @@ ParameterAclCreate(const char *parameter)
      * To prevent cluttering pg_parameter_acl with useless entries, insist
      * that the name be valid.
      */
-    if (!check_GUC_name_for_parameter_acl(parameter))
-        ereport(ERROR,
-                (errcode(ERRCODE_INVALID_NAME),
-                 errmsg("invalid parameter name \"%s\"",
-                        parameter)));
+    check_GUC_name_for_parameter_acl(parameter);

     /* Convert name to the form it should have in pg_parameter_acl. */
     parname = convert_GUC_name_for_parameter_acl(parameter);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index c25c697a06..e1ea5561d7 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -250,6 +250,8 @@ static void write_auto_conf_file(int fd, const char *filename, ConfigVariable *h
 static void replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p,
                                       const char *name, const char *value);
 static bool valid_custom_variable_name(const char *name);
+static bool assignable_custom_variable_name(const char *name, bool skip_errors,
+                                            int elevel);
 static void do_serialize(char **destptr, Size *maxbytes,
                          const char *fmt,...) pg_attribute_printf(3, 4);
 static bool call_bool_check_hook(struct config_bool *conf, bool *newval,
@@ -1063,7 +1065,7 @@ add_guc_variable(struct config_generic *var, int elevel)
  *
  * It must be two or more identifiers separated by dots, where the rules
  * for what is an identifier agree with scan.l.  (If you change this rule,
- * adjust the errdetail in find_option().)
+ * adjust the errdetail in assignable_custom_variable_name().)
  */
 static bool
 valid_custom_variable_name(const char *name)
@@ -1098,6 +1100,71 @@ valid_custom_variable_name(const char *name)
     return saw_sep;
 }

+/*
+ * Decide whether an unrecognized variable name is allowed to be SET.
+ *
+ * It must pass the syntactic rules of valid_custom_variable_name(),
+ * and it must not be in any namespace already reserved by an extension.
+ * (We make this separate from valid_custom_variable_name() because we don't
+ * apply the reserved-namespace test when reading configuration files.)
+ *
+ * If valid, return true.  Otherwise, return false if skip_errors is true,
+ * else throw a suitable error at the specified elevel (and return false
+ * if that's less than ERROR).
+ */
+static bool
+assignable_custom_variable_name(const char *name, bool skip_errors, int elevel)
+{
+    /* If there's no separator, it can't be a custom variable */
+    const char *sep = strchr(name, GUC_QUALIFIER_SEPARATOR);
+
+    if (sep != 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 false;
+        }
+        /* ... 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 false;
+            }
+        }
+        /* OK to create it */
+        return true;
+    }
+
+    /* Unrecognized single-part name */
+    if (!skip_errors)
+        ereport(elevel,
+                (errcode(ERRCODE_UNDEFINED_OBJECT),
+                 errmsg("unrecognized configuration parameter \"%s\"",
+                        name)));
+    return false;
+}
+
 /*
  * Create and add a placeholder variable for a custom variable name.
  */
@@ -1191,52 +1258,15 @@ find_option(const char *name, bool create_placeholders, bool skip_errors,
     if (create_placeholders)
     {
         /*
-         * Check if the name is valid, and if so, add a placeholder.  If it
-         * doesn't contain a separator, don't assume that it was meant to be a
-         * placeholder.
+         * Check if the name is valid, and if so, add a placeholder.
          */
-        const char *sep = strchr(name, GUC_QUALIFIER_SEPARATOR);
-
-        if (sep != 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 */
+        if (assignable_custom_variable_name(name, skip_errors, elevel))
             return add_placeholder_variable(name, elevel);
-        }
+        else
+            return NULL;        /* error message, if any, already emitted */
     }

-    /* Unknown name */
+    /* Unknown name and we're not supposed to make a placeholder */
     if (!skip_errors)
         ereport(elevel,
                 (errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -1369,18 +1399,16 @@ convert_GUC_name_for_parameter_acl(const char *name)
 /*
  * Check whether we should allow creation of a pg_parameter_acl entry
  * for the given name.  (This can be applied either before or after
- * canonicalizing it.)
+ * canonicalizing it.)  Throws error if not.
  */
-bool
+void
 check_GUC_name_for_parameter_acl(const char *name)
 {
     /* OK if the GUC exists. */
-    if (find_option(name, false, true, DEBUG1) != NULL)
-        return true;
+    if (find_option(name, false, true, DEBUG5) != NULL)
+        return;
     /* Otherwise, it'd better be a valid custom GUC name. */
-    if (valid_custom_variable_name(name))
-        return true;
-    return false;
+    (void) assignable_custom_variable_name(name, false, ERROR);
 }

 /*
@@ -4515,9 +4543,10 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
     {
         struct config_generic *record;

-        record = find_option(name, false, false, ERROR);
-        Assert(record != NULL);
-
+        /* We don't want to create a placeholder if there's not one already */
+        record = find_option(name, false, true, DEBUG5);
+        if (record != NULL)
+        {
         /*
          * Don't allow parameters that can't be set in configuration files to
          * be set in PG_AUTOCONF_FILENAME file.
@@ -4561,6 +4590,18 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
                         (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                          errmsg("parameter value for ALTER SYSTEM must not contain a newline")));
         }
+        }
+        else
+        {
+            /*
+             * Variable not known; check we'd be allowed to create it.  (We
+             * cannot validate the value, but that's fine.  A non-core GUC in
+             * the config file cannot cause postmaster start to fail, so we
+             * don't have to be too tense about possibly installing a bad
+             * value.)
+             */
+            (void) assignable_custom_variable_name(name, false, ERROR);
+        }
     }

     /*
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index e89083ee0e..902e57c0ca 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -363,7 +363,7 @@ extern const char *GetConfigOptionResetString(const char *name);
 extern int    GetConfigOptionFlags(const char *name, bool missing_ok);
 extern void ProcessConfigFile(GucContext context);
 extern char *convert_GUC_name_for_parameter_acl(const char *name);
-extern bool check_GUC_name_for_parameter_acl(const char *name);
+extern void check_GUC_name_for_parameter_acl(const char *name);
 extern void InitializeGUCOptions(void);
 extern bool SelectConfigFiles(const char *userDoption, const char *progname);
 extern void ResetAllOptions(void);
diff --git a/src/test/modules/unsafe_tests/expected/guc_privs.out
b/src/test/modules/unsafe_tests/expected/guc_privs.out
index f43a1da214..6c0ad89834 100644
--- a/src/test/modules/unsafe_tests/expected/guc_privs.out
+++ b/src/test/modules/unsafe_tests/expected/guc_privs.out
@@ -220,9 +220,31 @@ SELECT 1 FROM pg_parameter_acl WHERE parname = 'none.such';
 ----------
 (0 rows)

+-- Superuser should be able to ALTER SYSTEM SET a non-existent custom GUC.
+ALTER SYSTEM SET none.such = 'whiz bang';
+-- None of the above should have created a placeholder GUC for none.such.
+SHOW none.such;  -- error
+ERROR:  unrecognized configuration parameter "none.such"
+-- However, if we reload ...
+SELECT pg_reload_conf();
+ pg_reload_conf
+----------------
+ t
+(1 row)
+
+-- and start a new session to avoid race condition ...
+\c -
+SET SESSION AUTHORIZATION regress_admin;
+-- then it should be there.
+SHOW none.such;
+ none.such
+-----------
+ whiz bang
+(1 row)
+
 -- Can't grant on a non-existent core GUC.
 GRANT ALL ON PARAMETER no_such_guc TO regress_host_resource_admin;  -- fail
-ERROR:  invalid parameter name "no_such_guc"
+ERROR:  unrecognized configuration parameter "no_such_guc"
 -- Initially there are no privileges and no catalog entry for this GUC.
 SELECT has_parameter_privilege('regress_host_resource_admin', 'enable_material', 'SET');
  has_parameter_privilege
@@ -459,6 +481,8 @@ SELECT set_config ('temp_buffers', '8192', false); -- ok
 ALTER SYSTEM RESET autovacuum_work_mem;  -- ok, privileges have been granted
 ALTER SYSTEM RESET ALL;  -- fail, insufficient privileges
 ERROR:  permission denied to perform ALTER SYSTEM RESET ALL
+ALTER SYSTEM SET none.such2 = 'whiz bang';  -- fail, not superuser
+ERROR:  permission denied to set parameter "none.such2"
 ALTER ROLE regress_host_resource_admin SET lc_messages = 'POSIX';  -- fail
 ERROR:  permission denied to set parameter "lc_messages"
 ALTER ROLE regress_host_resource_admin SET max_stack_depth = '1MB';  -- ok
diff --git a/src/test/modules/unsafe_tests/sql/guc_privs.sql b/src/test/modules/unsafe_tests/sql/guc_privs.sql
index 7a4fb24b9d..9bcbbfa904 100644
--- a/src/test/modules/unsafe_tests/sql/guc_privs.sql
+++ b/src/test/modules/unsafe_tests/sql/guc_privs.sql
@@ -98,6 +98,19 @@ GRANT ALL ON PARAMETER none.such TO regress_host_resource_admin;
 SELECT 1 FROM pg_parameter_acl WHERE parname = 'none.such';
 REVOKE ALL ON PARAMETER "None.Such" FROM regress_host_resource_admin;
 SELECT 1 FROM pg_parameter_acl WHERE parname = 'none.such';
+
+-- Superuser should be able to ALTER SYSTEM SET a non-existent custom GUC.
+ALTER SYSTEM SET none.such = 'whiz bang';
+-- None of the above should have created a placeholder GUC for none.such.
+SHOW none.such;  -- error
+-- However, if we reload ...
+SELECT pg_reload_conf();
+-- and start a new session to avoid race condition ...
+\c -
+SET SESSION AUTHORIZATION regress_admin;
+-- then it should be there.
+SHOW none.such;
+
 -- Can't grant on a non-existent core GUC.
 GRANT ALL ON PARAMETER no_such_guc TO regress_host_resource_admin;  -- fail

@@ -190,6 +203,7 @@ ALTER SYSTEM RESET lc_messages;  -- fail, insufficient privileges
 SELECT set_config ('temp_buffers', '8192', false); -- ok
 ALTER SYSTEM RESET autovacuum_work_mem;  -- ok, privileges have been granted
 ALTER SYSTEM RESET ALL;  -- fail, insufficient privileges
+ALTER SYSTEM SET none.such2 = 'whiz bang';  -- fail, not superuser
 ALTER ROLE regress_host_resource_admin SET lc_messages = 'POSIX';  -- fail
 ALTER ROLE regress_host_resource_admin SET max_stack_depth = '1MB';  -- ok
 SELECT setconfig FROM pg_db_role_setting