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

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