Re: [PATCH] Fix ALTER SYSTEM empty string bug for GUC_LIST_QUOTE parameters - Mailing list pgsql-hackers

I wrote:
> Another idea is that we could redefine a single '' as meaning an empty
> list if we were to forbid empty strings as members of GUC_LIST_QUOTE
> variables.

I tried to work through what this'd imply, and arrived at the attached
patch.  I might've missed some places, and I did not think about what
documentation updates would be appropriate.

Note that the patch includes changing SplitIdentifierString and its
clones to forbid zero-length quoted elements, which were formerly
allowed.  Without this, we'd accept values from config files that
could not be represented in SET, which is exactly the situation we
are trying to fix.

I'm not entirely sure if this is the way to go, or if we want to
adopt some other solution that doesn't involve forbidding empty
list elements.  I suspect that anything else we come up with would
be less intuitive than letting SET list_var = '' do the job;
but maybe I just lack imagination today.

            regards, tom lane

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 3d6e6bdbfd2..27ba1920e68 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -3086,9 +3086,10 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
                  * 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.)
+                 * but we can't just feed them to the SQL parser that way; it
+                 * would truncate elements that are longer than NAMEDATALEN,
+                 * which would be wrong if they're paths.)  Also, we need a
+                 * special case for empty lists.
                  *
                  * Variables that are not so marked should just be emitted as
                  * simple string literals.  If the variable is not known to
@@ -3106,6 +3107,9 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
                         /* this shouldn't fail really */
                         elog(ERROR, "invalid list syntax in proconfig item");
                     }
+                    /* Special case: represent an empty list as '' */
+                    if (namelist == NIL)
+                        appendStringInfoString(&buf, "''");
                     foreach(lc, namelist)
                     {
                         char       *curname = (char *) lfirst(lc);
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 2c398cd9e5c..d5ef492ee41 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -2753,7 +2753,7 @@ SplitIdentifierString(char *rawstring, char separator,
         nextp++;                /* skip leading whitespace */

     if (*nextp == '\0')
-        return true;            /* allow empty string */
+        return true;            /* empty string represents empty list */

     /* At the top of the loop, we are at start of a new identifier. */
     do
@@ -2777,6 +2777,8 @@ SplitIdentifierString(char *rawstring, char separator,
                 nextp = endp;
             }
             /* endp now points at the terminating quote */
+            if (curname == endp)
+                return false;    /* empty quoted name not allowed */
             nextp = endp + 1;
         }
         else
@@ -2880,7 +2882,7 @@ SplitDirectoriesString(char *rawstring, char separator,
         nextp++;                /* skip leading whitespace */

     if (*nextp == '\0')
-        return true;            /* allow empty string */
+        return true;            /* empty string represents empty list */

     /* At the top of the loop, we are at start of a new directory. */
     do
@@ -2904,6 +2906,8 @@ SplitDirectoriesString(char *rawstring, char separator,
                 nextp = endp;
             }
             /* endp now points at the terminating quote */
+            if (curname == endp)
+                return false;    /* empty quoted name not allowed */
             nextp = endp + 1;
         }
         else
@@ -3001,7 +3005,7 @@ SplitGUCList(char *rawstring, char separator,
         nextp++;                /* skip leading whitespace */

     if (*nextp == '\0')
-        return true;            /* allow empty string */
+        return true;            /* empty string represents empty list */

     /* At the top of the loop, we are at start of a new identifier. */
     do
@@ -3025,6 +3029,8 @@ SplitGUCList(char *rawstring, char separator,
                 nextp = endp;
             }
             /* endp now points at the terminating quote */
+            if (curname == endp)
+                return false;    /* empty quoted name not allowed */
             nextp = endp + 1;
         }
         else
diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c
index b9e26982abd..a0557d164d8 100644
--- a/src/backend/utils/misc/guc_funcs.c
+++ b/src/backend/utils/misc/guc_funcs.c
@@ -210,12 +210,30 @@ flatten_set_variable_args(const char *name, List *args)
     else
         flags = 0;

-    /* Complain if list input and non-list variable */
-    if ((flags & GUC_LIST_INPUT) == 0 &&
-        list_length(args) != 1)
-        ereport(ERROR,
-                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                 errmsg("SET %s takes only one argument", name)));
+    /*
+     * Handle special cases for list input.
+     */
+    if (flags & GUC_LIST_INPUT)
+    {
+        /* A single empty-string item is treated as an empty list. */
+        if (list_length(args) == 1)
+        {
+            Node       *arg = (Node *) linitial(args);
+
+            if (IsA(arg, A_Const) &&
+                nodeTag(&((A_Const *) arg)->val) == T_String &&
+                *strVal(&((A_Const *) arg)->val) == '\0')
+                return pstrdup("");
+        }
+    }
+    else
+    {
+        /* Complain if list input and non-list variable. */
+        if (list_length(args) != 1)
+            ereport(ERROR,
+                    (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                     errmsg("SET %s takes only one argument", name)));
+    }

     initStringInfo(&buf);

@@ -269,6 +287,9 @@ flatten_set_variable_args(const char *name, List *args)
                     Datum        interval;
                     char       *intervalout;

+                    /* gram.y ensures this is only reachable for TIME ZONE */
+                    Assert(!(flags & GUC_LIST_QUOTE));
+
                     typenameTypeIdAndMod(NULL, typeName, &typoid, &typmod);
                     Assert(typoid == INTERVALOID);

@@ -286,9 +307,17 @@ flatten_set_variable_args(const char *name, List *args)
                 else
                 {
                     /*
-                     * Plain string literal or identifier.  For quote mode,
+                     * Plain string literal or identifier.  In a list GUC,
+                     * disallow empty-string elements (so that the preceding
+                     * hack for empty lists is unambiguous).  For quote mode,
                      * quote it if it's not a vanilla identifier.
                      */
+                    if ((flags & GUC_LIST_INPUT) && *val == '\0')
+                        ereport(ERROR,
+                                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                 errmsg("SET %s does not accept empty-string elements",
+                                        name)));
+
                     if (flags & GUC_LIST_QUOTE)
                         appendStringInfoString(&buf, quote_identifier(val));
                     else
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 05b84c0d6e7..00a369c8861 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -781,7 +781,7 @@ SplitGUCList(char *rawstring, char separator,
         nextp++;                /* skip leading whitespace */

     if (*nextp == '\0')
-        return true;            /* allow empty string */
+        return true;            /* empty string represents empty list */

     /* At the top of the loop, we are at start of a new identifier. */
     do
@@ -805,6 +805,8 @@ SplitGUCList(char *rawstring, char separator,
                 nextp = endp;
             }
             /* endp now points at the terminating quote */
+            if (curname == endp)
+                return false;    /* empty quoted name not allowed */
             nextp = endp + 1;
         }
         else
@@ -891,8 +893,9 @@ makeAlterConfigCommand(PGconn *conn, const char *configitem,
      * 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.)
+     * but we can't just feed them to the SQL parser that way; it would
+     * truncate elements that are longer than NAMEDATALEN, which would be
+     * wrong if they're paths.)  Also, we need a special case for empty lists.
      *
      * Variables that are not so marked should just be emitted as simple
      * string literals.  If the variable is not known to
@@ -908,6 +911,9 @@ makeAlterConfigCommand(PGconn *conn, const char *configitem,
         /* this shouldn't fail really */
         if (SplitGUCList(pos, ',', &namelist))
         {
+            /* Special case: represent an empty list as '' */
+            if (*namelist == NULL)
+                appendPQExpBufferStr(buf, "''");
             for (nameptr = namelist; *nameptr; nameptr++)
             {
                 if (nameptr != namelist)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index bea793456f9..9d36a6a5aaf 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -13699,8 +13699,9 @@ dumpFunc(Archive *fout, const FuncInfo *finfo)
          * 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.)
+         * parser that way; it would truncate elements that are longer than
+         * NAMEDATALEN, which would be wrong if they're paths.)  Also, we need
+         * a special case for empty lists.
          *
          * Variables that are not so marked should just be emitted as simple
          * string literals.  If the variable is not known to
@@ -13716,6 +13717,9 @@ dumpFunc(Archive *fout, const FuncInfo *finfo)
             /* this shouldn't fail really */
             if (SplitGUCList(pos, ',', &namelist))
             {
+                /* Special case: represent an empty list as '' */
+                if (*namelist == NULL)
+                    appendPQExpBufferStr(q, "''");
                 for (nameptr = namelist; *nameptr; nameptr++)
                 {
                     if (nameptr != namelist)
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 7f9e29c765c..e49e609415b 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -31,6 +31,24 @@ SELECT '2006-08-13 12:34:56'::timestamptz;
  2006-08-13 12:34:56-07
 (1 row)

+-- Check handling of list GUCs
+SET search_path = 'pg_catalog', Foo, 'Bar';
+SHOW search_path;
+      search_path
+------------------------
+ pg_catalog, foo, "Bar"
+(1 row)
+
+SET search_path = '';  -- means empty list
+SHOW search_path;
+ search_path
+-------------
+
+(1 row)
+
+SET search_path = '', 'foo';  -- error, empty list elements not OK
+ERROR:  SET search_path does not accept empty-string elements
+RESET search_path;
 -- SET LOCAL has no effect outside of a transaction
 SET LOCAL vacuum_cost_delay TO 50;
 WARNING:  SET LOCAL can only be used in transaction blocks
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 35e8aad7701..43d5cf10266 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3549,21 +3549,23 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
     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'
+    SET temp_tablespaces to ''
+    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$
                                                 + 
+                                                                          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 temp_tablespaces TO ''
                                             + 
+  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/guc.sql b/src/test/regress/sql/guc.sql
index f65f84a2632..65630135f18 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -12,6 +12,14 @@ SHOW vacuum_cost_delay;
 SHOW datestyle;
 SELECT '2006-08-13 12:34:56'::timestamptz;

+-- Check handling of list GUCs
+SET search_path = 'pg_catalog', Foo, 'Bar';
+SHOW search_path;
+SET search_path = '';  -- means empty list
+SHOW search_path;
+SET search_path = '', 'foo';  -- error, empty list elements not OK
+RESET search_path;
+
 -- SET LOCAL has no effect outside of a transaction
 SET LOCAL vacuum_cost_delay TO 50;
 SHOW vacuum_cost_delay;
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index fdd3ff1d161..5c5ff5e9cca 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -1217,7 +1217,8 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
     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'
+    SET temp_tablespaces to ''
+    SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path',
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
     IMMUTABLE STRICT;
 SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);