]> granicus.if.org Git - postgresql/commitdiff
Fix mishandling of quoted-list GUC values in pg_dump and ruleutils.c.
authorTom Lane <tgl@sss.pgh.pa.us>
Thu, 22 Mar 2018 00:03:28 +0000 (20:03 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Thu, 22 Mar 2018 00:03:28 +0000 (20:03 -0400)
Code that prints out the contents of setconfig or proconfig arrays in
SQL format needs to handle GUC_LIST_QUOTE variables differently from
other ones, because for those variables, flatten_set_variable_args()
already applied a layer of quoting.  The value can therefore safely
be printed as-is, and indeed must be, or flatten_set_variable_args()
will muck it up completely on reload.  For all other GUC variables,
it's necessary and sufficient to quote the value as a SQL literal.

We'd recognized the need for this long ago, but mis-analyzed the
need slightly, thinking that all GUC_LIST_INPUT variables needed
the special treatment.  That's actually wrong, since a valid value
of a LIST variable might include characters that need quoting,
although no existing variables accept such values.

More to the point, we hadn't made any particular effort to keep the
various places that deal with this up-to-date with the set of variables
that actually need special treatment, meaning that we'd do the wrong
thing with, for example, temp_tablespaces values.  This affects dumping
of SET clauses attached to functions, as well as ALTER DATABASE/ROLE SET
commands.

In ruleutils.c we can fix it reasonably honestly by exporting a guc.c
function that allows discovering the flags for a given GUC variable.
But pg_dump doesn't have easy access to that, so continue the old method
of having a hard-wired list of affected variable names.  At least we can
fix it to have just one list not two, and update the list to match
current reality.

A remaining problem with this is that it only works for built-in
GUC variables.  pg_dump's list obvious knows nothing of third-party
extensions, and even the "ask guc.c" method isn't bulletproof since
the relevant extension might not be loaded.  There's no obvious
solution to that, so for now, we'll just have to discourage extension
authors from inventing custom GUCs that need GUC_LIST_QUOTE.

This has been busted for a long time, so back-patch to all supported
branches.

Michael Paquier and Tom Lane, reviewed by Kyotaro Horiguchi and
Pavel Stehule

Discussion: https://postgr.es/m/20180111064900.GA51030@paquier.xyz

src/backend/utils/adt/ruleutils.c
src/backend/utils/misc/guc.c
src/bin/pg_dump/dumputils.c
src/bin/pg_dump/dumputils.h
src/bin/pg_dump/pg_dump.c
src/bin/pg_dump/pg_dumpall.c
src/include/utils/guc.h
src/test/regress/expected/rules.out
src/test/regress/sql/rules.sql

index 1b2486c27079c0f3e509647ca5dddd6ae19033fe..8b755c4f0cd26b62ee8c87cd2066ecdc83d4d200 100644 (file)
@@ -58,6 +58,7 @@
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
+#include "utils/guc.h"
 #include "utils/hsearch.h"
 #include "utils/lsyscache.h"
 #include "utils/rel.h"
@@ -2197,11 +2198,15 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
                                                                 quote_identifier(configitem));
 
                                /*
-                                * Some GUC variable names are 'LIST' type and hence must not
-                                * be quoted.
+                                * Variables that are marked GUC_LIST_QUOTE were already fully
+                                * quoted by flatten_set_variable_args() before they were put
+                                * into the proconfig array; we mustn't re-quote them or we'll
+                                * make a mess.  Variables that are not so marked should just
+                                * be emitted as simple string literals.  If the variable is
+                                * not known to guc.c, we'll do the latter; this makes it
+                                * unsafe to use GUC_LIST_QUOTE for extension variables.
                                 */
-                               if (pg_strcasecmp(configitem, "DateStyle") == 0
-                                       || pg_strcasecmp(configitem, "search_path") == 0)
+                               if (GetConfigOptionFlags(configitem, true) & GUC_LIST_QUOTE)
                                        appendStringInfoString(&buf, pos);
                                else
                                        simple_quote_literal(&buf, pos);
index 9ecbe4a9ba9f13277df39a8e35b7616ddfa80d5c..0b403a327295802ffbad871be4bde8307bf195fe 100644 (file)
@@ -769,8 +769,8 @@ static const unit_conversion time_unit_conversion_table[] =
  *
  * 6. Don't forget to document the option (at least in config.sgml).
  *
- * 7. If it's a new GUC_LIST option you must edit pg_dumpall.c to ensure
- *       it is not single quoted at dump time.
+ * 7. If it's a new GUC_LIST_QUOTE option, you must add it to
+ *       variable_is_guc_list_quote() in src/bin/pg_dump/dumputils.c.
  */
 
 
@@ -6689,6 +6689,30 @@ GetConfigOptionResetString(const char *name)
        return NULL;
 }
 
+/*
+ * Get the GUC flags associated with the given option.
+ *
+ * If the option doesn't exist, return 0 if missing_ok is true,
+ * otherwise throw an ereport and don't return.
+ */
+int
+GetConfigOptionFlags(const char *name, bool missing_ok)
+{
+       struct config_generic *record;
+
+       record = find_option(name, false, WARNING);
+       if (record == NULL)
+       {
+               if (missing_ok)
+                       return 0;
+               ereport(ERROR,
+                               (errcode(ERRCODE_UNDEFINED_OBJECT),
+                                errmsg("unrecognized configuration parameter \"%s\"",
+                                               name)));
+       }
+       return record->flags;
+}
+
 
 /*
  * flatten_set_variable_args
index abd0579d5d60652952457783b5f243fac24e0de8..008f28483573dfdf6443be5d5a5b318f0b17e510 100644 (file)
@@ -853,3 +853,26 @@ buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
                printfPQExpBuffer(init_racl_subquery, "NULL");
        }
 }
+
+/*
+ * Detect whether the given GUC variable is of GUC_LIST_QUOTE type.
+ *
+ * It'd be better if we could inquire this directly from the backend; but even
+ * if there were a function for that, it could only tell us about variables
+ * currently known to guc.c, so that it'd be unsafe for extensions to declare
+ * GUC_LIST_QUOTE variables anyway.  Lacking a solution for that, it doesn't
+ * seem worth the work to do more than have this list, which must be kept in
+ * sync with the variables actually marked GUC_LIST_QUOTE in guc.c.
+ */
+bool
+variable_is_guc_list_quote(const char *name)
+{
+       if (pg_strcasecmp(name, "temp_tablespaces") == 0 ||
+               pg_strcasecmp(name, "session_preload_libraries") == 0 ||
+               pg_strcasecmp(name, "shared_preload_libraries") == 0 ||
+               pg_strcasecmp(name, "local_preload_libraries") == 0 ||
+               pg_strcasecmp(name, "search_path") == 0)
+               return true;
+       else
+               return false;
+}
index e2241cf40ee4acb94b65ea8c343304d81649cf2d..e6c4fcc15bafe9ff2f6a41cdce447f7aad09317c 100644 (file)
@@ -56,4 +56,6 @@ extern void buildACLQueries(PQExpBuffer acl_subquery, PQExpBuffer racl_subquery,
                                const char *acl_column, const char *acl_owner,
                                const char *obj_kind, bool binary_upgrade);
 
+extern bool variable_is_guc_list_quote(const char *name);
+
 #endif   /* DUMPUTILS_H */
index 41d0a1afeef444245afb381c8acd8ecc166ffe2e..2e6225049c06b6f6bb9c55d44bd19ec19ce6ae65 100644 (file)
@@ -11799,11 +11799,15 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
                appendPQExpBuffer(q, "\n    SET %s TO ", fmtId(configitem));
 
                /*
-                * Some GUC variable names are 'LIST' type and hence must not be
-                * quoted.
+                * Variables that are marked GUC_LIST_QUOTE were already fully quoted
+                * by flatten_set_variable_args() before they were put into the
+                * proconfig array; we mustn't re-quote them or we'll make a mess.
+                * Variables that are not so marked should just be emitted as simple
+                * string literals.  If the variable is not known to
+                * variable_is_guc_list_quote(), we'll do the latter; this makes it
+                * unsafe to use GUC_LIST_QUOTE for extension variables.
                 */
-               if (pg_strcasecmp(configitem, "DateStyle") == 0
-                       || pg_strcasecmp(configitem, "search_path") == 0)
+               if (variable_is_guc_list_quote(configitem))
                        appendPQExpBufferStr(q, pos);
                else
                        appendStringLiteralAH(q, pos, fout);
index 3fc7b024c8a061551ae8d124a66492700b9e8ebf..3554f51ec564e08e45621a0815753bd3c404ae1b 100644 (file)
@@ -1719,10 +1719,15 @@ makeAlterConfigCommand(PGconn *conn, const char *arrayitem,
        appendPQExpBuffer(buf, "SET %s TO ", fmtId(mine));
 
        /*
-        * Some GUC variable names are 'LIST' type and hence must not be quoted.
+        * Variables that are marked GUC_LIST_QUOTE were already fully quoted by
+        * flatten_set_variable_args() before they were put into the setconfig
+        * array; we mustn't re-quote them or we'll make a mess.  Variables that
+        * are not so marked should just be emitted as simple string literals.  If
+        * the variable is not known to variable_is_guc_list_quote(), we'll do the
+        * latter; this makes it unsafe to use GUC_LIST_QUOTE for extension
+        * variables.
         */
-       if (pg_strcasecmp(mine, "DateStyle") == 0
-               || pg_strcasecmp(mine, "search_path") == 0)
+       if (variable_is_guc_list_quote(mine))
                appendPQExpBufferStr(buf, pos + 1);
        else
                appendStringLiteralConn(buf, pos + 1, conn);
index 68c911030446d54b64ee4d9a6782ee2c1eb3031f..2b8a0b933ddcdfa00a3f45294d4931e57fd39660 100644 (file)
@@ -349,6 +349,7 @@ extern void EmitWarningsOnPlaceholders(const char *className);
 extern const char *GetConfigOption(const char *name, bool missing_ok,
                                bool restrict_superuser);
 extern const char *GetConfigOptionResetString(const char *name);
+extern int     GetConfigOptionFlags(const char *name, bool missing_ok);
 extern void ProcessConfigFile(GucContext context);
 extern void InitializeGUCOptions(void);
 extern bool SelectConfigFiles(const char *userDoption, const char *progname);
index 4be33aac84e766ed977a95760d7f0ecc489201e4..3f9592f2b5eadc4e22bf24b304a89d5931b2dd85 100644 (file)
@@ -3059,6 +3059,33 @@ SELECT * FROM hat_data WHERE hat_name IN ('h8', 'h9', 'h7') ORDER BY hat_name;
 DROP RULE hat_upsert ON hats;
 drop table hats;
 drop table hat_data;
+-- test for pg_get_functiondef properly regurgitating SET parameters
+-- Note that the function is kept around to stress pg_dump.
+CREATE FUNCTION func_with_set_params() RETURNS integer
+    AS 'select 1;'
+    LANGUAGE SQL
+    SET search_path TO PG_CATALOG
+    SET extra_float_digits TO 2
+    SET work_mem TO '4MB'
+    SET datestyle to iso, mdy
+    SET local_preload_libraries TO "Mixed/Case", 'c:/"a"/path'
+    IMMUTABLE STRICT;
+SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
+                      pg_get_functiondef                       
+---------------------------------------------------------------
+ CREATE OR REPLACE FUNCTION public.func_with_set_params()     +
+  RETURNS integer                                             +
+  LANGUAGE sql                                                +
+  IMMUTABLE STRICT                                            +
+  SET search_path TO pg_catalog                               +
+  SET extra_float_digits TO '2'                               +
+  SET work_mem TO '4MB'                                       +
+  SET "DateStyle" TO 'iso, mdy'                               +
+  SET local_preload_libraries TO "Mixed/Case", "c:/""a""/path"+
+ AS $function$select 1;$function$                             +
+(1 row)
+
 -- tests for pg_get_*def with invalid objects
 SELECT pg_get_constraintdef(0);
  pg_get_constraintdef 
index 90dc9ceaf46a93ed217dad2711aa6c3a014c6103..14b2276bf156592baaaf89e87c4b49c40a241ada 100644 (file)
@@ -1145,6 +1145,19 @@ DROP RULE hat_upsert ON hats;
 drop table hats;
 drop table hat_data;
 
+-- test for pg_get_functiondef properly regurgitating SET parameters
+-- Note that the function is kept around to stress pg_dump.
+CREATE FUNCTION func_with_set_params() RETURNS integer
+    AS 'select 1;'
+    LANGUAGE SQL
+    SET search_path TO PG_CATALOG
+    SET extra_float_digits TO 2
+    SET work_mem TO '4MB'
+    SET datestyle to iso, mdy
+    SET local_preload_libraries TO "Mixed/Case", 'c:/"a"/path'
+    IMMUTABLE STRICT;
+SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
+
 -- tests for pg_get_*def with invalid objects
 SELECT pg_get_constraintdef(0);
 SELECT pg_get_functiondef(0);