From 67e02cde7373b09a8f6897b7fea3800ceb91ad9e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 21 Mar 2018 20:03:28 -0400 Subject: [PATCH] Fix mishandling of quoted-list GUC values in pg_dump and ruleutils.c. 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 | 13 +++++++++---- src/backend/utils/misc/guc.c | 28 ++++++++++++++++++++++++++-- src/bin/pg_dump/dumputils.c | 23 +++++++++++++++++++++++ src/bin/pg_dump/dumputils.h | 2 ++ src/bin/pg_dump/pg_dump.c | 12 ++++++++---- src/bin/pg_dump/pg_dumpall.c | 11 ++++++++--- src/include/utils/guc.h | 1 + src/test/regress/expected/rules.out | 25 +++++++++++++++++++++++++ src/test/regress/sql/rules.sql | 12 ++++++++++++ 9 files changed, 114 insertions(+), 13 deletions(-) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index d161c5c2526..49aeb8835ed 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -54,6 +54,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" @@ -2052,11 +2053,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); diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index e393fd4786b..787a3e0b13c 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -674,8 +674,8 @@ const char *const config_type_names[] = * * 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. */ @@ -6370,6 +6370,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 diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c index 8eb9e0c9926..46fc56e86b2 100644 --- a/src/bin/pg_dump/dumputils.c +++ b/src/bin/pg_dump/dumputils.c @@ -1522,3 +1522,26 @@ simple_string_list_member(SimpleStringList *list, const char *val) } return false; } + +/* + * 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; +} diff --git a/src/bin/pg_dump/dumputils.h b/src/bin/pg_dump/dumputils.h index 24dc404ed1f..25da30284e8 100644 --- a/src/bin/pg_dump/dumputils.h +++ b/src/bin/pg_dump/dumputils.h @@ -75,4 +75,6 @@ extern void set_dump_section(const char *arg, int *dumpSections); extern void simple_string_list_append(SimpleStringList *list, const char *val); extern bool simple_string_list_member(SimpleStringList *list, const char *val); +extern bool variable_is_guc_list_quote(const char *name); + #endif /* DUMPUTILS_H */ diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 4501c8bed8e..523a12f3585 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -10149,11 +10149,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); diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index fc2ee0a07e3..0b493c04830 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -1610,10 +1610,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); diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index 564cf9255df..c3fbbf6d0ae 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -323,6 +323,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); diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index 78435fe9fa8..a5d62bcc72d 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -2706,3 +2706,28 @@ View definition: FROM ( VALUES (1,2)) v(q, w); drop view rule_v1; +-- 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 extra_float_digits TO 2 + SET work_mem TO '4MB' + SET datestyle to iso, mdy + SET search_path TO PG_CATALOG, "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 extra_float_digits TO '2' + + SET work_mem TO '4MB' + + SET "DateStyle" TO 'iso, mdy' + + SET search_path TO pg_catalog, "Mixed/Case", "c:/""a""/path"+ + AS $function$select 1;$function$ + + +(1 row) + diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql index 6dde91e9084..960e882b657 100644 --- a/src/test/regress/sql/rules.sql +++ b/src/test/regress/sql/rules.sql @@ -1023,3 +1023,15 @@ drop view rule_v1; create view rule_v1(x) as select * from (values(1,2)) v(q,w); \d+ rule_v1 drop view rule_v1; + +-- 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 extra_float_digits TO 2 + SET work_mem TO '4MB' + SET datestyle to iso, mdy + SET search_path TO PG_CATALOG, "Mixed/Case", 'c:/"a"/path' + IMMUTABLE STRICT; +SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);