Обсуждение: [PATCH] Fix ALTER SYSTEM empty string bug for GUC_LIST_QUOTE parameters

Hi Andrew

On 28.08.25 11:29, Andrei Klychkov wrote:
> I'm submitting a patch to fix a bug where ALTER SYSTEM SET with empty
> strings for
> GUC_LIST_QUOTE parameters (like shared_preload_libraries) results in
> malformed
> configuration entries that cause server crashes on restart.

I tested the patch and it does what you described

$ psql postgres -c "ALTER SYSTEM SET shared_preload_libraries TO '';"
ALTER SYSTEM
$ cat /usr/local/postgres-dev/testdb/postgresql.auto.conf
# Do not edit this file manually!
# It will be overwritten by the ALTER SYSTEM command.
shared_preload_libraries = ''

However, it breaks one of the rules.sql regression tests

@@ -3552,21 +3552,7 @@
     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)
-
+ERROR:  invalid list syntax in proconfig item

Best, Jim

On Wed, Sep 3, 2025 at 6:59 PM Andrei Klychkov
<andrew.a.klychkov@gmail.com> wrote:
>
> Hi Jim,
>
> Thanks a lot for reviewing! Nice catch, TIL!
> Version 2 of the patch is attached, please check it out.
> In a nutshell, the issue actually wasn't in the flatten_set_variable_args() function as initially suspected, but
ratherin the configuration file writing logic in the write_auto_conf_file(): more details in v2_README.md 

Even with this patch, an empty string set via SET is still quoted. For example:

    =# SET local_preload_libraries TO '';
    SET
    =# SHOW local_preload_libraries ;
     local_preload_libraries
    -------------------------
     ""
    (1 row)

Is this behavior acceptable? I was thinking that an empty string should not
be quoted, regardless of whether it's set by ALTER SYSTEM SET or SET.
Thought?

If we agree it should be handled in both cases, flatten_set_variable_args()
seems to need to be updated. For example:

@@ -289,7 +289,8 @@ flatten_set_variable_args(const char *name, List *args)
                                         * Plain string literal or
identifier.  For quote mode,
                                         * quote it if it's not a
vanilla identifier.
                                         */
-                                       if (flags & GUC_LIST_QUOTE)
+                                       if (flags & GUC_LIST_QUOTE &&
+                                               !(val[0] == '\0' &&
list_length(args) == 1))

appendStringInfoString(&buf, quote_identifier(val));
                                        else

appendStringInfoString(&buf, val);

Regards,

--
Fujii Masao

On 04.09.25 09:41, Andrei Klychkov wrote:
> Even with this patch, an empty string set via SET is still quoted. For
> example:
>
>     =# SET local_preload_libraries TO '';
>     SET
>     =# SHOW local_preload_libraries ;
>      local_preload_libraries
>     -------------------------
>      ""
>     (1 row)
>
> Is this behavior acceptable? I was thinking that an empty string
> should not
> be quoted, regardless of whether it's set by ALTER SYSTEM SET or SET.
> Thought?
>
> If we agree it should be handled in both cases,
> flatten_set_variable_args()
> seems to need to be updated. For example:
>
> @@ -289,7 +289,8 @@ flatten_set_variable_args(const char *name, List
> *args)
>                                          * Plain string literal or
> identifier.  For quote mode,
>                                          * quote it if it's not a
> vanilla identifier.
>                                          */
> -                                       if (flags & GUC_LIST_QUOTE)
> +                                       if (flags & GUC_LIST_QUOTE &&
> +                                               !(val[0] == '\0' &&
> list_length(args) == 1))
>
> appendStringInfoString(&buf, quote_identifier(val));
>                                         else
>
> appendStringInfoString(&buf, val);

Yeah, I also think that SET and ALTER SYSTEM SET should be consistent. I
tested your proposed changes in flatten_set_variable_args ..

/*
 * Plain string literal or identifier.  For quote mode,
 * quote it if it's not a vanilla identifier. However, if the value
 * is an empty string (val[0] == '\0') and it is the only element
 * in the list (list_length(args) == 1), display it as an empty string
 * without quotes for clarity and consistency.
 */
if (flags & GUC_LIST_QUOTE &&
    !(val[0] == '\0' && list_length(args) == 1))
    appendStringInfoString(&buf, quote_identifier(val));
else
    appendStringInfoString(&buf, val);

... and it seems to work:

$ psql postgres -c "SET local_preload_libraries TO ''; SHOW
local_preload_libraries;"
SET
 local_preload_libraries
-------------------------
 
(1 row)

$ psql postgres -c "ALTER SYSTEM SET local_preload_libraries TO '';"
ALTER SYSTEM

(restart ..)

$ cat /usr/local/postgres-dev/testdb/postgresql.auto.conf
# Do not edit this file manually!
# It will be overwritten by the ALTER SYSTEM command.
local_preload_libraries = ''

$ psql postgres -c "SHOW local_preload_libraries;"
 local_preload_libraries
-------------------------
 
(1 row)


I'm wondering if we should add some tests, e.g. in guc.sql:

SET local_preload_libraries TO '';
SHOW local_preload_libraries;

and also it's equivalents for ALTER SYSTEM SET (still not sure where :)).

Best regards, Jim

On Thu, Sep 4, 2025 at 4:42 PM Andrei Klychkov
<andrew.a.klychkov@gmail.com> wrote:
>
> > Even with this patch, an empty string set via SET is still quoted. For example:
> >
> >    =# SET local_preload_libraries TO '';
> >    SET
> >    =# SHOW local_preload_libraries ;
> >     local_preload_libraries
> >    -------------------------
> >     ""
> >    (1 row)
> >
> > Is this behavior acceptable? I was thinking that an empty string should not
> > be quoted, regardless of whether it's set by ALTER SYSTEM SET or SET.
> > Thought?
> >
> > If we agree it should be handled in both cases, flatten_set_variable_args()
> > seems to need to be updated.
>
> Hello Fujii,
> Thanks for your review!
>
> I'm personally not sure because this is my first patch and I'm trying to solve a specific issue of the
postgresql.auto.conf-relatedserver crashes. 
> If what your *broader-impact* suggestion makes sense to more experienced devs in this area, I'd be happy to update
thepatch as you put it, test it (as much as I can), and re-submit v3. 
> Otherwise, I'd be happy with the v2 implementation that seemingly solves my specific issue.

Yeah, I think my suggestion makes sense.

BTW, regarding the behavior change, I believe that users likely expect
the parameter to be reset when specifying an empty string, rather than
being set to "". So the proposed change seems reasonable. However,
the current behavior has existed for a long time, and I haven’t seen
any complaints about it. Some users may rely on the existing behavior
(I think that’s unlikely, though). So I'm not completely sure yet if this change
should be applied.

Regards,

--
Fujii Masao

On Thu, Sep 4, 2025 at 11:58 PM Jim Jones <jim.jones@uni-muenster.de> wrote:
> Yeah, I also think that SET and ALTER SYSTEM SET should be consistent. I
> tested your proposed changes in flatten_set_variable_args ..

Thanks for the test!


> I'm wondering if we should add some tests, e.g. in guc.sql:

+1 to add regression tests.

Regards,

--
Fujii Masao

Fujii Masao <masao.fujii@gmail.com> writes:
>>> Even with this patch, an empty string set via SET is still quoted. For example:
>>>
>>> =# SET local_preload_libraries TO '';
>>> SET
>>> =# SHOW local_preload_libraries ;
>>> local_preload_libraries
>>> -------------------------
>>> ""
>>> (1 row)
>>>
>>> Is this behavior acceptable? I was thinking that an empty string should not
>>> be quoted, regardless of whether it's set by ALTER SYSTEM SET or SET.

> BTW, regarding the behavior change, I believe that users likely expect
> the parameter to be reset when specifying an empty string, rather than
> being set to "". So the proposed change seems reasonable. However,
> the current behavior has existed for a long time, and I haven’t seen
> any complaints about it.

I think this is largely based on confusion.  In the above example,
local_preload_libraries is being set to a list containing a single
entry that is an empty string, and the output of SHOW is a fully
accurate depiction of that state.  It is *not* being set to an
empty list --- we actually don't have any syntax that would permit
doing so in SET.  For comparison, there is a big difference between

    SET local_preload_libraries = a, b;
    SET local_preload_libraries = 'a, b';

In the latter case you get a single list entry containing the
string "a, b".  We do not try to parse that into multiple entries,
and by the same token parsing an empty string into an empty list
would be the Wrong Thing.

We might want to start resolving this by inventing a syntax for
setting a list GUC to an empty list.  I'm not very sure what that
should look like, except that it mustn't be SET ... TO ''.

I'm not certain whether config-file parsing or ALTER SYSTEM
would need any code changes once we resolve the ambiguity in SET.
The config-file syntax is different and doesn't have this problem
of not being able to represent an empty list.

(Also, "let's unify the list-GUC syntax between config file and SET"
seems like a non-starter.  It'd be better no doubt if they hadn't
diverged, but at this point we'd break far more than we fix if
we change either one.)

            regards, tom lane

I wrote:
> We might want to start resolving this by inventing a syntax for
> setting a list GUC to an empty list.  I'm not very sure what that
> should look like, except that it mustn't be SET ... TO ''.

I experimented with the idea of allowing

    SET var TO ;

and got a bunch of shift-reduce conflicts from bison, which don't
look easy to avoid.  It's probably a bit too surprising anyway.

This works from a grammar standpoint:

    SET var TO NULL;

Because NULL is fully reserved, this isn't usurping any cases that
weren't syntax errors before.  It might still be too surprising.

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.  This doesn't look like it'd be a big issue for the current
set of such variables:

local_preload_libraries
search_path
session_preload_libraries
shared_preload_libraries
temp_tablespaces
unix_socket_directories

We might break some applications that're relying on the current
behavior that an empty item would be effectively ignored.
But that seems a bit remote, and anyway they could just switch
to some other nonexistent name.

Actually, looking at the small number of GUCs that are marked
GUC_LIST_INPUT but not GUC_LIST_QUOTE, I wonder if we shouldn't
prohibit empty strings across-the-board for GUC_LIST_INPUT GUCs.
I don't see any where it'd be useful to allow them.  Then we
could allow '' to mean empty list for all of them.

            regards, tom lane

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);

On 04.09.25 23:52, Tom Lane wrote:
> 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.

This approach LGTM. It solves the initial issue:

$ psql postgres -c "ALTER SYSTEM SET local_preload_libraries TO '';"
ALTER SYSTEM

$ cat /usr/local/postgres-dev/testdb/postgresql.auto.conf
# Do not edit this file manually!
# It will be overwritten by the ALTER SYSTEM command.
local_preload_libraries = ''

... making a clear distinction between empty elements and empty lists:

postgres=# SET local_preload_libraries TO '','foo';
ERROR:  SET local_preload_libraries does not accept empty-string elements

postgres=# SET local_preload_libraries TO '';
SET
postgres=# SHOW local_preload_libraries;
 local_preload_libraries
-------------------------
 
(1 row)

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.

Best regards, Jim

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);

Hi Tom

On 05.09.25 23:06, Tom Lane wrote:
> 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.  

I think this is a nice addition. The way I see it is: it provides an
unambiguous way to "clear" the variable, which, as you pointed out,
might carry different semantics in the future than an empty string. More
generally, I understand that using NULL (unknown/undefined) to represent
an empty list could be seen as a semantic stretch, but in this case it
doesn’t feel unintuitive to me. Although I prefer this new syntax, I can
definitely live without it :)

Here some tests:

== ALTER SYSTEM SET var TO ''

$ psql postgres -c "ALTER SYSTEM SET local_preload_libraries TO '';"
ALTER SYSTEM

$ cat /usr/local/postgres-dev/testdb/postgresql.auto.conf
# Do not edit this file manually!
# It will be overwritten by the ALTER SYSTEM command.
local_preload_libraries = '""'

$ pg_ctl -D /usr/local/postgres-dev/testdb -l
/usr/local/postgres-dev/logfile restart
waiting for server to shut down.... done
server stopped
waiting for server to start.... done
server started

$ psql postgres
psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed:
FATAL:  could not access file "$libdir/plugins/": No such file or directory

The error itself is expected, but the message does not make it
immediately clear that the problem comes from a misconfigured GUC. But
this seems to be more related to the specific variable than to the scope
of this patch.

== ALTER SYSTEM SET var TO NULL
Using the new syntax it works just fine:

$ psql postgres -c "ALTER SYSTEM SET local_preload_libraries TO NULL;"
ALTER SYSTEM

$ cat /usr/local/postgres-dev/testdb/postgresql.auto.conf
# Do not edit this file manually!
# It will be overwritten by the ALTER SYSTEM command.
local_preload_libraries = ''

$ pg_ctl -D /usr/local/postgres-dev/testdb -l
/usr/local/postgres-dev/logfile restart
waiting for server to shut down.... done
server stopped
waiting for server to start.... done
server started

$ psql postgres -c "SHOW local_preload_libraries;"
 local_preload_libraries
-------------------------
 
(1 row)

== SET var TO ''

$ psql postgres -c "SET local_preload_libraries TO ''; SHOW
local_preload_libraries;"
SET
 local_preload_libraries
-------------------------
 ""
(1 row)

== SET var TO NULL

$ psql postgres -c "SET local_preload_libraries TO NULL; SHOW
local_preload_libraries;"
SET
 local_preload_libraries
-------------------------
 
(1 row)

== SET var TO list containing empty element

$ psql postgres -c "SET local_preload_libraries TO 'foo',''; SHOW
local_preload_libraries;"
SET
 local_preload_libraries
-------------------------
 foo, ""
(1 row)

== SET var TO list containing NULL element

$ psql postgres -c "SET local_preload_libraries TO NULL,''; SHOW
local_preload_libraries;"
ERROR:  syntax error at or near ","
LINE 1: SET local_preload_libraries TO NULL,''; SHOW local_preload_l...

== SET var TO list containing multiple empty elements

$ /usr/local/postgres-dev/bin/psql postgres -c "SET
local_preload_libraries TO '',''; SHOW local_preload_libraries;"
SET
 local_preload_libraries
-------------------------
 "", ""
(1 row)


Best regards, Jim