[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

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 rather in the configuration file
writing logic in the write_auto_conf_file(): more details in v2_README.md

Looking forward to your feedback, thanks!
Regards
Andrew

On Tue, Sep 2, 2025 at 2:16 PM Jim Jones <jim.jones@uni-muenster.de> wrote:

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 rather in 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

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-related server crashes.
If what your *broader-impact* suggestion makes sense to more experienced
devs in this area, I'd be happy to update the patch 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.

Thanks
Regards
Andrew

On Wed, Sep 3, 2025 at 4:48 PM Fujii Masao <masao.fujii@gmail.com> wrote:

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-related server crashes.
If what your *broader-impact* suggestion makes sense to more experienced devs in this area, I'd be happy to update the patch 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

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

As it solves the initial issue, it SGTM too.
I applied v3, updated the docs and added some tests in attached v4.
Hopefully it's OK.
Please take a look

Thanks
Regards

On Fri, Sep 5, 2025 at 9:33 AM Jim Jones <jim.jones@uni-muenster.de> wrote:

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

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

Hello,

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.

Looks like the patch v5 resolves a long-standing limitation where there was
no SQL syntax to set a list-based GUC to an empty list. I like this
approach. Also the changes seem non-breaking. Thanks

1. Would be great to have some explanations in docs about this new behavior.

2. It doesn't look to me that v5 solves the original issue of a user
running ALTER SYSTEM SET <setting like shared_preload_libraries> = ''; ,
then restarting the server and not getting it back online.

Regards
Andrew

On Sat, Sep 6, 2025 at 4:44 PM Jim Jones <jim.jones@uni-muenster.de> wrote:

Andrei Klychkov <andrew.a.klychkov@gmail.com> writes:

Looks like the patch v5 resolves a long-standing limitation where there was
no SQL syntax to set a list-based GUC to an empty list. I like this
approach. Also the changes seem non-breaking. Thanks

1. Would be great to have some explanations in docs about this new behavior.

Yeah, I hadn't bothered with docs, pending decisions about which way
we were going to implement this. But it seems like we're leaning
towards using the NULL syntax, so I went ahead and did some docs work.

2. It doesn't look to me that v5 solves the original issue of a user
running ALTER SYSTEM SET <setting like shared_preload_libraries> = ''; ,
then restarting the server and not getting it back online.

[ shrug... ] It's not supposed to "solve" that. That command is
erroneous, and if you didn't test the setting before restarting the
server, you shouldn't be too surprised if restart fails. What this
patch is meant to do is provide a valid way to accomplish what you
wanted, namely

ALTER SYSTEM SET shared_preload_libraries = NULL;

Anyway, the attached v6 is the same as v5, except now with proposed
doc changes and a draft commit message. I spent some effort on
getting the ALTER SYSTEM and SET ref pages back into sync; it seemed
like more care has been taken with the ALTER SYSTEM synopsis and
other details.

I'm not sure if we want to change anything about this in config.sgml.
There are enough GUC_LIST_INPUT GUCs that I can't see mentioning it
for each one.

regards, tom lane

I wrote:

Andrei Klychkov <andrew.a.klychkov@gmail.com> writes:

2. It doesn't look to me that v5 solves the original issue of a user
running ALTER SYSTEM SET <setting like shared_preload_libraries> = ''; ,
then restarting the server and not getting it back online.

[ shrug... ] It's not supposed to "solve" that. That command is
erroneous, and if you didn't test the setting before restarting the
server, you shouldn't be too surprised if restart fails.

If you are feeling excited about that specific case, I think the
correct solution would be to install a GUC check_hook for
shared_preload_libraries (and probably its siblings too). It couldn't
go so far as to actually dlopen() the list items, but it could check
that each one resolves as an accessible file.

A potential objection is that this could result in unwanted failures
in some scenarios, say where you're restoring a dump and haven't
yet installed all the relevant extensions. I'm not quite sure if
there are realistic scenarios where that's actually a problem.
If it is, perhaps we could adjust the check_hook so it issues
WARNINGs not hard errors.

I'm not sure it's worth the trouble though. A quick look at dfmgr.c
suggests that it'd take quite a lot of refactoring (or else code
duplication, not good) to be able to apply the library lookup process
without actually doing dlopen. In any case that would be a totally
different patch from what we are discussing here.

Anyway, the attached v6 is the same as v5, except now with proposed
doc changes and a draft commit message. I spent some effort on
getting the ALTER SYSTEM and SET ref pages back into sync; it seemed
like more care has been taken with the ALTER SYSTEM synopsis and
other details.

Hearing no further comments, I'm going to go ahead with v6.

regards, tom lane

On 11/4/25 5:58 PM, Tom Lane wrote:

Hearing no further comments, I'm going to go ahead with v6.

Honestly none of the alternatives is very appealing and v6 is probably
the least bad.

When I ran into this issue I thought about adding and EMPTY keyword (we
do not want more keywords) or adding support for specifying array
literals like this [] or [a, b], but that would be confusing since
postgresql.conf does not allow any such syntax.

Andreas

Andreas Karlsson <andreas@proxel.se> writes:

Honestly none of the alternatives is very appealing and v6 is probably
the least bad.

Yeah, that's about where I'm at with this.

When I ran into this issue I thought about adding and EMPTY keyword (we
do not want more keywords) or adding support for specifying array
literals like this [] or [a, b], but that would be confusing since
postgresql.conf does not allow any such syntax.

I would have preferred a different keyword. But AFAICS it'd have to
be a fully-reserved word, and creating a new one of those has to
clear a pretty high bar. NULL is not so far off...

regards, tom lane