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

Jim Jones <jim.jones@uni-muenster.de> writes:
> On 04.09.25 23:52, Tom Lane wrote:
>> 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.

> The ambiguity between an empty list and an empty element has always
> existed in list-valued GUCs. This patch resolves the issue by
> disallowing empty elements, thereby making '' an unambiguous
> representation of an empty list. Personally, I find SET var TO NULL (or
> perhaps a keyword like EMPTY or NONE) a more palatable syntax for
> expressing empty lists in this case. However, I’m not sure the
> additional complexity and compatibility implications would justify such
> a change.

Since you expressed interest, I made a draft patch that does it like
that.  Unsurprisingly, it has to touch mostly the same places that
the v3 patch did, plus the grammar.  Still ends up a bit shorter
though.

I remain unsure which way I like better.  The NULL approach has the
advantage of not foreclosing use of empty-string list elements, which
we might want someday even if there's no obvious value today.  (And
for the same reason, it's less of a behavioral change.)  But it still
feels a bit less intuitive to me.  It might flow better with some
other keyword --- but we have to use a fully-reserved keyword, and we
are surely not going to make a new one of those just for this purpose,
and NULL is the only existing one that's even slightly on-point.

            regards, tom lane

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 9fd48acb1f8..ff31653b6ac 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -1709,6 +1709,26 @@ generic_set:
                     n->location = @3;
                     $$ = n;
                 }
+            | var_name TO NULL_P
+                {
+                    VariableSetStmt *n = makeNode(VariableSetStmt);
+
+                    n->kind = VAR_SET_VALUE;
+                    n->name = $1;
+                    n->args = list_make1(makeNullAConst(@3));
+                    n->location = @3;
+                    $$ = n;
+                }
+            | var_name '=' NULL_P
+                {
+                    VariableSetStmt *n = makeNode(VariableSetStmt);
+
+                    n->kind = VAR_SET_VALUE;
+                    n->name = $1;
+                    n->args = list_make1(makeNullAConst(@3));
+                    n->location = @3;
+                    $$ = n;
+                }
             | var_name TO DEFAULT
                 {
                     VariableSetStmt *n = makeNode(VariableSetStmt);
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 3d6e6bdbfd2..f81cfd17fd0 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -3088,7 +3088,8 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
                  * 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.)
+                 * longer than NAMEDATALEN.)  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 NULL */
+                    if (namelist == NIL)
+                        appendStringInfoString(&buf, "NULL");
                     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..1e1f69f7528 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
@@ -2880,7 +2880,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
@@ -3001,7 +3001,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
diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c
index b9e26982abd..d0f4e396acc 100644
--- a/src/backend/utils/misc/guc_funcs.c
+++ b/src/backend/utils/misc/guc_funcs.c
@@ -210,12 +210,29 @@ 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)
+    {
+        /* NULL represents an empty list. */
+        if (list_length(args) == 1)
+        {
+            Node       *arg = (Node *) linitial(args);
+
+            if (IsA(arg, A_Const) &&
+                ((A_Const *) arg)->isnull)
+                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);

@@ -246,6 +263,12 @@ flatten_set_variable_args(const char *name, List *args)
             elog(ERROR, "unrecognized node type: %d", (int) nodeTag(arg));
         con = (A_Const *) arg;

+        /* Complain if NULL is used with a non-list variable. */
+        if (con->isnull)
+            ereport(ERROR,
+                    (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                     errmsg("NULL is an invalid value for %s", name)));
+
         switch (nodeTag(&con->val))
         {
             case T_Integer:
@@ -269,6 +292,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);

diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 05b84c0d6e7..2d22723aa91 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
@@ -893,6 +893,7 @@ makeAlterConfigCommand(PGconn *conn, const char *configitem,
      * 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.)
+     * 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 +909,9 @@ makeAlterConfigCommand(PGconn *conn, const char *configitem,
         /* this shouldn't fail really */
         if (SplitGUCList(pos, ',', &namelist))
         {
+            /* Special case: represent an empty list as NULL */
+            if (*namelist == NULL)
+                appendPQExpBufferStr(buf, "NULL");
             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..08fcfcfbdfe 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -13700,7 +13700,8 @@ dumpFunc(Archive *fout, const FuncInfo *finfo)
          * 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.)
+         * zero-length or longer than NAMEDATALEN.)  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 NULL */
+                if (*namelist == NULL)
+                    appendPQExpBufferStr(q, "NULL");
                 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..d6fb879f500 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -31,6 +31,28 @@ 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 = null;  -- means empty list
+SHOW search_path;
+ search_path
+-------------
+
+(1 row)
+
+SET search_path = null, null;  -- syntax error
+ERROR:  syntax error at or near ","
+LINE 1: SET search_path = null, null;
+                              ^
+SET enable_seqscan = null;  -- error
+ERROR:  NULL is an invalid value for enable_seqscan
+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..3cc5f8a77b2 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3549,6 +3549,7 @@ 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 temp_tablespaces to NULL
     SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '',
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
     IMMUTABLE STRICT;
 SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
@@ -3562,6 +3563,7 @@ SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
   SET extra_float_digits TO '2'
                                                 + 
   SET work_mem TO '4MB'
                                                 + 
   SET "DateStyle" TO 'iso, mdy'
                                                 + 
+  SET temp_tablespaces TO NULL
                                                 + 
   SET local_preload_libraries TO 'Mixed/Case', 'c:/''a"/path', '',
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'+
  AS $function$select 1;$function$
                                                 + 

diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index f65f84a2632..bafaf067e82 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -12,6 +12,15 @@ 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 = null;  -- means empty list
+SHOW search_path;
+SET search_path = null, null;  -- syntax error
+SET enable_seqscan = null;  -- error
+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..3f240bec7b0 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -1217,6 +1217,7 @@ 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 temp_tablespaces to NULL
     SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '',
'0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
     IMMUTABLE STRICT;
 SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);