Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs - Mailing list pgsql-hackers

So here's what I think we should do about this:

1. Only GUC_LIST_QUOTE variables need to be special-cased.  It works
fine to treat plain LIST variables (e.g. datestyle) with the regular
code path.  This is good because there are so few of them.

2. We should make an effort to minimize the number of places that know
explicitly which variables are GUC_LIST_QUOTE.  In the backend, the
right thing to do is ask guc.c.  In pg_dump, there's no convenient
way to ask the backend (and certainly nothing that would work against
old servers), but we should at least make sure there's only one place
to fix not two.

3. We need to prevent extensions from trying to create GUC_LIST_QUOTE
variables, because those are not going to work correctly if they're
set before the extension is loaded, nor is there any hope of pg_dump
dumping them correctly.

The attached 0001 patch does the first two things, and could be
back-patched.  The 0002 patch, which I'd only propose for HEAD,
is one way we could address point 3.  A different way would be
to throw a WARNING rather than ERROR and then just mask off the
problematic bit.  Or we could decide that either of these cures
is worse than the disease, but I don't really agree with that.

Comments?

            regards, tom lane

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 2cd54ec..b0559ca 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***************
*** 61,66 ****
--- 61,67 ----
  #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"
*************** pg_get_functiondef(PG_FUNCTION_ARGS)
*** 2597,2607 ****
                                   quote_identifier(configitem));

                  /*
!                  * Some GUC variable names are 'LIST' type and hence must not
!                  * be quoted.
                   */
!                 if (pg_strcasecmp(configitem, "DateStyle") == 0
!                     || pg_strcasecmp(configitem, "search_path") == 0)
                      appendStringInfoString(&buf, pos);
                  else
                      simple_quote_literal(&buf, pos);
--- 2598,2612 ----
                                   quote_identifier(configitem));

                  /*
!                  * 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 (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 7a7ac47..398680a 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** static const unit_conversion time_unit_c
*** 796,803 ****
   *
   * 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.
   */


--- 796,803 ----
   *
   * 6. Don't forget to document the option (at least in config.sgml).
   *
!  * 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.
   */


*************** GetConfigOptionResetString(const char *n
*** 6859,6864 ****
--- 6859,6888 ----
      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 7f5bb13..875545f 100644
*** a/src/bin/pg_dump/dumputils.c
--- b/src/bin/pg_dump/dumputils.c
*************** buildACLQueries(PQExpBuffer acl_subquery
*** 851,856 ****
--- 851,879 ----
  }

  /*
+  * 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;
+ }
+
+ /*
   * Helper function for dumping "ALTER DATABASE/ROLE SET ..." commands.
   *
   * Parse the contents of configitem (a "name=value" string), wrap it in
*************** makeAlterConfigCommand(PGconn *conn, con
*** 887,897 ****
      appendPQExpBuffer(buf, "SET %s TO ", fmtId(mine));

      /*
!      * Some GUC variable names are 'LIST' type and hence must not be quoted.
!      * XXX this list is incomplete ...
       */
!     if (pg_strcasecmp(mine, "DateStyle") == 0
!         || pg_strcasecmp(mine, "search_path") == 0)
          appendPQExpBufferStr(buf, pos);
      else
          appendStringLiteralConn(buf, pos, conn);
--- 910,924 ----
      appendPQExpBuffer(buf, "SET %s TO ", fmtId(mine));

      /*
!      * 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 (variable_is_guc_list_quote(mine))
          appendPQExpBufferStr(buf, pos);
      else
          appendStringLiteralConn(buf, pos, conn);
diff --git a/src/bin/pg_dump/dumputils.h b/src/bin/pg_dump/dumputils.h
index a9e26ae..7bd3e1d 100644
*** a/src/bin/pg_dump/dumputils.h
--- b/src/bin/pg_dump/dumputils.h
*************** extern void buildACLQueries(PQExpBuffer
*** 56,61 ****
--- 56,63 ----
                  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);
+
  extern void makeAlterConfigCommand(PGconn *conn, const char *configitem,
                         const char *type, const char *name,
                         const char *type2, const char *name2,
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 566cbf2..b8d65a9 100644
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11877,11887 ****
          appendPQExpBuffer(q, "\n    SET %s TO ", fmtId(configitem));

          /*
!          * Some GUC variable names are 'LIST' type and hence must not be
!          * quoted.
           */
!         if (pg_strcasecmp(configitem, "DateStyle") == 0
!             || pg_strcasecmp(configitem, "search_path") == 0)
              appendPQExpBufferStr(q, pos);
          else
              appendStringLiteralAH(q, pos, fout);
--- 11877,11891 ----
          appendPQExpBuffer(q, "\n    SET %s TO ", fmtId(configitem));

          /*
!          * 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 (variable_is_guc_list_quote(configitem))
              appendPQExpBufferStr(q, pos);
          else
              appendStringLiteralAH(q, pos, fout);
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 2e03640..3d13a33 100644
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
*************** extern void EmitWarningsOnPlaceholders(c
*** 349,354 ****
--- 349,355 ----
  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 5e0597e..9730c6b 100644
*** a/src/test/regress/expected/rules.out
--- b/src/test/regress/expected/rules.out
*************** SELECT pg_get_partkeydef(0);
*** 3228,3233 ****
--- 3228,3260 ----

  (1 row)

+ -- 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)
+
  -- test rename for a rule defined on a partitioned table
  CREATE TABLE rules_parted_table (a int) PARTITION BY LIST (a);
  CREATE TABLE rules_parted_table_1 PARTITION OF rules_parted_table FOR VALUES IN (1);
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index 6021212..61f97f5 100644
*** a/src/test/regress/sql/rules.sql
--- b/src/test/regress/sql/rules.sql
*************** SELECT pg_get_function_arg_default(0, 0)
*** 1170,1175 ****
--- 1170,1188 ----
  SELECT pg_get_function_arg_default('pg_class'::regclass, 0);
  SELECT pg_get_partkeydef(0);

+ -- 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);
+
  -- test rename for a rule defined on a partitioned table
  CREATE TABLE rules_parted_table (a int) PARTITION BY LIST (a);
  CREATE TABLE rules_parted_table_1 PARTITION OF rules_parted_table FOR VALUES IN (1);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 398680a..153373e 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** init_custom_variable(const char *name,
*** 7607,7612 ****
--- 7607,7621 ----
          elog(FATAL, "cannot create PGC_POSTMASTER variables after startup");

      /*
+      * We can't support custom GUC_LIST_QUOTE variables, because the wrong
+      * things would happen if such a variable were set or pg_dump'd when the
+      * defining extension isn't loaded.  Again, treat this as fatal because
+      * the loadable module may be partly initialized already.
+      */
+     if (flags & GUC_LIST_QUOTE)
+         elog(FATAL, "extensions cannot define GUC_LIST_QUOTE variables");
+
+     /*
       * Before pljava commit 398f3b876ed402bdaec8bc804f29e2be95c75139
       * (2015-12-15), two of that module's PGC_USERSET variables facilitated
       * trivial escalation to superuser privileges.  Restrict the variables to