Thread: BUG #15248: pg_upgrade fails when a function with an emptysearch_path is encountered

The following bug has been logged on the website:

Bug reference:      15248
Logged by:          Steven Winfield
Email address:      steven.winfield@cantabcapital.com
PostgreSQL version: 11beta1
Operating system:   RHEL 7.4
Description:

I suspect this is reproducible with pg_dump / pg_restore too.

If a function is defined like this:

CREATE FUNCTION public.foo(int) RETURNS int
    LANGUAGE "sql"
    SET search_path TO ''
    AS $_$ SELECT 1; $_$;

...then, during pg_upgrade, pg_restore renders this SQL to recreate the
function using double-quotes around the empty search_path:

pg_restore: creating FUNCTION "public.foo("int")"
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 3488; 1255 67351571
FUNCTION foo("int") dbadmin
pg_restore: [archiver (db)] could not execute query: ERROR:  zero-length
delimited identifier at or near """"
LINE 3:     SET "search_path" TO ""
                                 ^
    Command was: CREATE FUNCTION "public"."foo"("int") RETURNS int
    LANGUAGE "sql" 
    SET "search_path" TO ""
    AS $_$ SELECT 1 $_$

The fix is to use single quotes in this case, and I suppose the workaround
is to specify 'pg_temp,pg_catalog' instead of the empty string.

=?utf-8?q?PG_Bug_reporting_form?= <noreply@postgresql.org> writes:
> If a function is defined like this:

> CREATE FUNCTION public.foo(int) RETURNS int
>     LANGUAGE "sql"
>     SET search_path TO ''
>     AS $_$ SELECT 1; $_$;

> ...then, during pg_upgrade, pg_restore renders this SQL to recreate the
> function using double-quotes around the empty search_path:

>     Command was: CREATE FUNCTION "public"."foo"("int") RETURNS int
>     LANGUAGE "sql" 
>     SET "search_path" TO ""
>     AS $_$ SELECT 1 $_$

> [ which isn't legal syntax ]

Hm.  So, basically, this is due to insufficient thought about quoting
rules in commits 742869946 et al.  We made pg_dump print out exactly
what it sees in pg_proc.proconfig for this case --- but that string
has been constructed by flatten_set_variable_args, and while its
quoting rules are a lot like those for double-quoted SQL identifiers,
they aren't identical.  Notably we have this issue, that "" is a legal
empty string as per flatten_set_variable_args, but it's not a legal
SQL identifier.  Another problem, now that I think about it, is that
on reload a double-quoted identifier is going to get truncated to
NAMEDATALEN, but that's not necessarily appropriate --- some
GUC_LIST_QUOTE variables contain file path names that could legally
be longer than that.

If we were working in a green field I'd think about revising
flatten_set_variable_args, but touching those rules is pretty scary;
even if we found all the relevant places in the backend and pg_dump,
there might be third-party code that is looking at proconfig or
related catalog columns.  Also, that'd do nothing for existing
database objects that have config strings in the existing style.

So it seems like what we have to do here is to teach pg_dump and ruleutils
to parse a GUC_LIST_QUOTE value the same way SplitIdentifierString does,
and then quote each extracted list element as a string literal.  Bleah.
It's not *that* much code, but it's annoying, especially because of the
duplicated logic.

            regards, tom lane

I wrote:
> So it seems like what we have to do here is to teach pg_dump and ruleutils
> to parse a GUC_LIST_QUOTE value the same way SplitIdentifierString does,
> and then quote each extracted list element as a string literal.  Bleah.
> It's not *that* much code, but it's annoying, especially because of the
> duplicated logic.

Here's a proposed patch for this.  As I feared, there's kind of a lot of
code duplication :-(.  I thought for awhile about trying to unify the four
copies of the split-on-delimiters code into one function with a bunch of
option flags ... but it seemed like that would be pretty messy too, so I
desisted.

            regards, tom lane

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 5a61d2d..03e9a28 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** pg_get_functiondef(PG_FUNCTION_ARGS)
*** 2640,2653 ****
                  /*
                   * 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);
                  appendStringInfoChar(&buf, '\n');
--- 2640,2678 ----
                  /*
                   * Variables that are marked GUC_LIST_QUOTE were already fully
                   * quoted by flatten_set_variable_args() before they were put
!                  * into the proconfig array.  However, because the quoting
!                  * rules used there aren't exactly like SQL's, we have to
!                  * break the list value apart and then quote the elements as
!                  * string literals.  (The elements may be double-quoted as-is,
!                  * but we can't just feed them to the SQL parser; it would do
!                  * the wrong thing with elements that are zero-length or
!                  * longer than NAMEDATALEN.)
!                  *
!                  * 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 that; this makes it unsafe to use
!                  * GUC_LIST_QUOTE for extension variables.
                   */
                  if (GetConfigOptionFlags(configitem, true) & GUC_LIST_QUOTE)
!                 {
!                     List       *namelist;
!                     ListCell   *lc;
!
!                     /* Parse string into list of identifiers */
!                     if (!SplitGUCList(pos, ',', &namelist))
!                     {
!                         /* this shouldn't fail really */
!                         elog(ERROR, "invalid list syntax in proconfig item");
!                     }
!                     foreach(lc, namelist)
!                     {
!                         char       *curname = (char *) lfirst(lc);
!
!                         simple_quote_literal(&buf, curname);
!                         if (lnext(lc))
!                             appendStringInfoString(&buf, ", ");
!                     }
!                 }
                  else
                      simple_quote_literal(&buf, pos);
                  appendStringInfoChar(&buf, '\n');
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 31eaa92..a26ff82 100644
*** a/src/backend/utils/adt/varlena.c
--- b/src/backend/utils/adt/varlena.c
*************** SplitDirectoriesString(char *rawstring,
*** 3503,3508 ****
--- 3503,3620 ----
  }


+ /*
+  * SplitGUCList --- parse a string containing identifiers or file names
+  *
+  * This is used to split the value of a GUC_LIST_QUOTE GUC variable, without
+  * presuming whether the elements will be taken as identifiers or file names.
+  * We assume the input has already been through flatten_set_variable_args(),
+  * so that we need never downcase (if appropriate, that was done already).
+  * Nor do we ever truncate, since we don't know the correct max length.
+  * We disallow embedded whitespace for simplicity (it shouldn't matter,
+  * because any embedded whitespace should have led to double-quoting).
+  * Otherwise the API is identical to SplitIdentifierString.
+  *
+  * XXX it's annoying to have three copies of this string-splitting logic.
+  * However, it's not clear that having one function with a bunch of option
+  * flags would be much better.
+  *
+  * XXX there is a version of this function in src/bin/pg_dump/dumputils.c.
+  * Be sure to update that if you have to change this.
+  *
+  * Inputs:
+  *    rawstring: the input string; must be overwritable!    On return, it's
+  *               been modified to contain the separated identifiers.
+  *    separator: the separator punctuation expected between identifiers
+  *               (typically '.' or ',').  Whitespace may also appear around
+  *               identifiers.
+  * Outputs:
+  *    namelist: filled with a palloc'd list of pointers to identifiers within
+  *              rawstring.  Caller should list_free() this even on error return.
+  *
+  * Returns true if okay, false if there is a syntax error in the string.
+  */
+ bool
+ SplitGUCList(char *rawstring, char separator,
+              List **namelist)
+ {
+     char       *nextp = rawstring;
+     bool        done = false;
+
+     *namelist = NIL;
+
+     while (scanner_isspace(*nextp))
+         nextp++;                /* skip leading whitespace */
+
+     if (*nextp == '\0')
+         return true;            /* allow empty string */
+
+     /* At the top of the loop, we are at start of a new identifier. */
+     do
+     {
+         char       *curname;
+         char       *endp;
+
+         if (*nextp == '"')
+         {
+             /* Quoted name --- collapse quote-quote pairs */
+             curname = nextp + 1;
+             for (;;)
+             {
+                 endp = strchr(nextp + 1, '"');
+                 if (endp == NULL)
+                     return false;    /* mismatched quotes */
+                 if (endp[1] != '"')
+                     break;        /* found end of quoted name */
+                 /* Collapse adjacent quotes into one quote, and look again */
+                 memmove(endp, endp + 1, strlen(endp));
+                 nextp = endp;
+             }
+             /* endp now points at the terminating quote */
+             nextp = endp + 1;
+         }
+         else
+         {
+             /* Unquoted name --- extends to separator or whitespace */
+             curname = nextp;
+             while (*nextp && *nextp != separator &&
+                    !scanner_isspace(*nextp))
+                 nextp++;
+             endp = nextp;
+             if (curname == nextp)
+                 return false;    /* empty unquoted name not allowed */
+         }
+
+         while (scanner_isspace(*nextp))
+             nextp++;            /* skip trailing whitespace */
+
+         if (*nextp == separator)
+         {
+             nextp++;
+             while (scanner_isspace(*nextp))
+                 nextp++;        /* skip leading whitespace for next */
+             /* we expect another name, so done remains false */
+         }
+         else if (*nextp == '\0')
+             done = true;
+         else
+             return false;        /* invalid syntax */
+
+         /* Now safe to overwrite separator with a null */
+         *endp = '\0';
+
+         /*
+          * Finished isolating current name --- add it to list
+          */
+         *namelist = lappend(*namelist, curname);
+
+         /* Loop back if we didn't reach end of string */
+     } while (!done);
+
+     return true;
+ }
+
+
  /*****************************************************************************
   *    Comparison Functions used for bytea
   *
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 875545f..8a93ace 100644
*** a/src/bin/pg_dump/dumputils.c
--- b/src/bin/pg_dump/dumputils.c
***************
*** 14,19 ****
--- 14,21 ----
   */
  #include "postgres_fe.h"

+ #include <ctype.h>
+
  #include "dumputils.h"
  #include "fe_utils/string_utils.h"

*************** variable_is_guc_list_quote(const char *n
*** 874,879 ****
--- 876,990 ----
  }

  /*
+  * SplitGUCList --- parse a string containing identifiers or file names
+  *
+  * This is used to split the value of a GUC_LIST_QUOTE GUC variable, without
+  * presuming whether the elements will be taken as identifiers or file names.
+  * See comparable code in src/backend/utils/adt/varlena.c.
+  *
+  * Inputs:
+  *    rawstring: the input string; must be overwritable!    On return, it's
+  *               been modified to contain the separated identifiers.
+  *    separator: the separator punctuation expected between identifiers
+  *               (typically '.' or ',').  Whitespace may also appear around
+  *               identifiers.
+  * Outputs:
+  *    namelist: receives a malloc'd, null-terminated array of pointers to
+  *              identifiers within rawstring.  Caller should free this
+  *              even on error return.
+  *
+  * Returns true if okay, false if there is a syntax error in the string.
+  */
+ bool
+ SplitGUCList(char *rawstring, char separator,
+              char ***namelist)
+ {
+     char       *nextp = rawstring;
+     bool        done = false;
+     char      **nextptr;
+
+     /*
+      * Since we disallow empty identifiers, this is a conservative
+      * overestimate of the number of pointers we could need.  Allow one for
+      * list terminator.
+      */
+     *namelist = nextptr = (char **)
+         pg_malloc((strlen(rawstring) / 2 + 2) * sizeof(char *));
+     *nextptr = NULL;
+
+     while (isspace((unsigned char) *nextp))
+         nextp++;                /* skip leading whitespace */
+
+     if (*nextp == '\0')
+         return true;            /* allow empty string */
+
+     /* At the top of the loop, we are at start of a new identifier. */
+     do
+     {
+         char       *curname;
+         char       *endp;
+
+         if (*nextp == '"')
+         {
+             /* Quoted name --- collapse quote-quote pairs */
+             curname = nextp + 1;
+             for (;;)
+             {
+                 endp = strchr(nextp + 1, '"');
+                 if (endp == NULL)
+                     return false;    /* mismatched quotes */
+                 if (endp[1] != '"')
+                     break;        /* found end of quoted name */
+                 /* Collapse adjacent quotes into one quote, and look again */
+                 memmove(endp, endp + 1, strlen(endp));
+                 nextp = endp;
+             }
+             /* endp now points at the terminating quote */
+             nextp = endp + 1;
+         }
+         else
+         {
+             /* Unquoted name --- extends to separator or whitespace */
+             curname = nextp;
+             while (*nextp && *nextp != separator &&
+                    !isspace((unsigned char) *nextp))
+                 nextp++;
+             endp = nextp;
+             if (curname == nextp)
+                 return false;    /* empty unquoted name not allowed */
+         }
+
+         while (isspace((unsigned char) *nextp))
+             nextp++;            /* skip trailing whitespace */
+
+         if (*nextp == separator)
+         {
+             nextp++;
+             while (isspace((unsigned char) *nextp))
+                 nextp++;        /* skip leading whitespace for next */
+             /* we expect another name, so done remains false */
+         }
+         else if (*nextp == '\0')
+             done = true;
+         else
+             return false;        /* invalid syntax */
+
+         /* Now safe to overwrite separator with a null */
+         *endp = '\0';
+
+         /*
+          * Finished isolating current name --- add it to output array
+          */
+         *nextptr++ = curname;
+
+         /* Loop back if we didn't reach end of string */
+     } while (!done);
+
+     *nextptr = NULL;
+     return true;
+ }
+
+ /*
   * 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
*** 912,925 ****
      /*
       * 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);

--- 1023,1057 ----
      /*
       * Variables that are marked GUC_LIST_QUOTE were already fully quoted by
       * flatten_set_variable_args() before they were put into the setconfig
!      * array.  However, because the quoting rules used there aren't exactly
!      * like SQL's, we have to break the list value apart and then quote the
!      * elements as string literals.  (The elements may be double-quoted as-is,
!      * but we can't just feed them to the SQL parser; it would do the wrong
!      * thing with elements that are zero-length or longer than NAMEDATALEN.)
!      *
!      * 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 that; this makes it unsafe to
!      * use GUC_LIST_QUOTE for extension variables.
       */
      if (variable_is_guc_list_quote(mine))
!     {
!         char      **namelist;
!         char      **nameptr;
!
!         /* Parse string into list of identifiers */
!         /* this shouldn't fail really */
!         if (SplitGUCList(pos, ',', &namelist))
!         {
!             for (nameptr = namelist; *nameptr; nameptr++)
!             {
!                 if (nameptr != namelist)
!                     appendPQExpBufferStr(buf, ", ");
!                 appendStringLiteralConn(buf, *nameptr, conn);
!             }
!         }
!         pg_free(namelist);
!     }
      else
          appendStringLiteralConn(buf, pos, conn);

diff --git a/src/bin/pg_dump/dumputils.h b/src/bin/pg_dump/dumputils.h
index 7bd3e1d..0043124 100644
*** a/src/bin/pg_dump/dumputils.h
--- b/src/bin/pg_dump/dumputils.h
*************** extern void buildACLQueries(PQExpBuffer
*** 58,63 ****
--- 58,66 ----

  extern bool variable_is_guc_list_quote(const char *name);

+ extern bool SplitGUCList(char *rawstring, char separator,
+              char ***namelist);
+
  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 31ebd8b..20e8aed 100644
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11964,11977 ****
          /*
           * 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);
      }
--- 11964,11999 ----
          /*
           * Variables that are marked GUC_LIST_QUOTE were already fully quoted
           * by flatten_set_variable_args() before they were put into the
!          * proconfig array.  However, because the quoting rules used there
!          * aren't exactly like SQL's, we have to break the list value apart
!          * and then quote the elements as string literals.  (The elements may
!          * be double-quoted as-is, but we can't just feed them to the SQL
!          * parser; it would do the wrong thing with elements that are
!          * zero-length or longer than NAMEDATALEN.)
!          *
           * 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 that; this makes it unsafe
!          * to use GUC_LIST_QUOTE for extension variables.
           */
          if (variable_is_guc_list_quote(configitem))
!         {
!             char      **namelist;
!             char      **nameptr;
!
!             /* Parse string into list of identifiers */
!             /* this shouldn't fail really */
!             if (SplitGUCList(pos, ',', &namelist))
!             {
!                 for (nameptr = namelist; *nameptr; nameptr++)
!                 {
!                     if (nameptr != namelist)
!                         appendPQExpBufferStr(q, ", ");
!                     appendStringLiteralAH(q, *nameptr, fout);
!                 }
!             }
!             pg_free(namelist);
!         }
          else
              appendStringLiteralAH(q, pos, fout);
      }
diff --git a/src/include/utils/varlena.h b/src/include/utils/varlena.h
index 90e131a..c776931 100644
*** a/src/include/utils/varlena.h
--- b/src/include/utils/varlena.h
*************** extern bool SplitIdentifierString(char *
*** 31,36 ****
--- 31,38 ----
                        List **namelist);
  extern bool SplitDirectoriesString(char *rawstring, char separator,
                         List **namelist);
+ extern bool SplitGUCList(char *rawstring, char separator,
+              List **namelist);
  extern text *replace_text_regexp(text *src_text, void *regexp,
                      text *replace_text, bool glob);

diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 744d501..078129f 100644
*** a/src/test/regress/expected/rules.out
--- b/src/test/regress/expected/rules.out
*************** CREATE FUNCTION func_with_set_params() R
*** 3160,3180 ****
      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)

--- 3160,3180 ----
      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', '',
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
      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', '',
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'+
!  AS $function$select 1;$function$
                                                  + 

  (1 row)

diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index 3ca4c07..f4ee30e 100644
*** a/src/test/regress/sql/rules.sql
--- b/src/test/regress/sql/rules.sql
*************** CREATE FUNCTION func_with_set_params() R
*** 1164,1170 ****
      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);

--- 1164,1170 ----
      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', '',
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
      IMMUTABLE STRICT;
  SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);