Re: [PATCH] Fix ALTER SYSTEM empty string bug for GUC_LIST_QUOTE parameters

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