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(dot)fujii(at)gmail(dot)com> wrote:

> On Wed, Sep 3, 2025 at 6:59 PM Andrei Klychkov
> <andrew(dot)a(dot)klychkov(at)gmail(dot)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
>