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 <[email protected]> wrote: > 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 >
# ALTER SYSTEM Empty String Bug Fix - Version 2 ## Problem Description When using `ALTER SYSTEM SET` with an empty string for parameters that have the `GUC_LIST_QUOTE` flag (such as `shared_preload_libraries`), PostgreSQL was writing malformed configuration to `postgresql.auto.conf`. Specifically: ```sql ALTER SYSTEM SET "shared_preload_libraries" TO ''; ``` Would result in this malformed configuration: ``` shared_preload_libraries = '""' ``` This double-quoted empty string caused server crashes on restart due to invalid configuration syntax. ## Root Cause Analysis The issue was **NOT** in the `flatten_set_variable_args` function as initially suspected, but rather in the configuration file writing logic in `write_auto_conf_file()`. The problem occurred because: 1. Empty strings for `GUC_LIST_QUOTE` parameters were being processed correctly during the ALTER SYSTEM operation 2. However, when writing to the configuration file, the value was being double-quoted 3. This resulted in `'""'` being written instead of `''` 4. When the server tried to restart, it couldn't parse the malformed configuration ## Solution Implemented The fix targets the `write_auto_conf_file()` function in `src/backend/utils/misc/guc.c` and: 1. Identifies when a `GUC_LIST_QUOTE` parameter has an empty string value (`''` or `""`) 2. For such cases, writes the configuration as `parameter = ''` instead of the malformed double-quoted version 3. Maintains compatibility**: Only affects the specific bug case, leaving all other functionality intact ## Files Modified - `src/backend/utils/misc/guc.c` - Added special handling for empty strings in `GUC_LIST_QUOTE` parameters ## Why This Approach This solution is safer and more maintainable because it: - Addresses the symptom (malformed configuration output) directly - Doesn't change the underlying logic that other parts of the system depend on - Is targeted and specific to the problematic case - Maintains backward compatibility
From 4642e4e0b0a63d9da6ac4479963dec6c7b71ac68 Mon Sep 17 00:00:00 2001 From: Andrew Klychkov <[email protected]> Date: Wed, 3 Sep 2025 10:48:49 +0200 Subject: [PATCH] Fix ALTER SYSTEM empty string bug for GUC_LIST_QUOTE parameters When ALTER SYSTEM SET is used with an empty string for parameters with GUC_LIST_QUOTE flag (like shared_preload_libraries), the empty string was being double-quoted, resulting in '""' being written to postgresql.auto.conf. This caused server crashes on restart due to malformed configuration syntax. The fix detects when a GUC_LIST_QUOTE parameter has the value '""' and writes it as '' instead, preventing the double-quoting issue while maintaining proper configuration file syntax. Fixes bug where 'ALTER SYSTEM SET "shared_preload_libraries" TO ''' would write 'shared_preload_libraries = '""'' to postgresql.auto.conf. --- src/backend/utils/misc/guc.c | 39 ++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 46fdefebe3..7494cbd56b 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -4497,6 +4497,45 @@ write_auto_conf_file(int fd, const char *filename, ConfigVariable *head) for (item = head; item != NULL; item = item->next) { char *escaped; + bool is_guc_list_quote = false; + + /* + * Check if this is a GUC_LIST_QUOTE parameter to handle empty strings + * specially and prevent double-quoting issues. + */ + if (item->value[0] == '\0' || strcmp(item->value, "\"\"") == 0) + { + struct config_generic *record = find_option(item->name, false, true, WARNING); + if (record && (record->flags & GUC_LIST_QUOTE)) + is_guc_list_quote = true; + } + + /* + * Special handling for GUC_LIST_QUOTE parameters with empty strings. + * When ALTER SYSTEM SET is used with an empty string for such parameters, + * the value comes through as '""' (quoted empty string). We want to write + * this as an empty string rather than the quoted version to avoid the + * double-quoting issue. + */ + if (is_guc_list_quote && (item->value[0] == '\0' || strcmp(item->value, "\"\"") == 0)) + { + /* For GUC_LIST_QUOTE parameters, write empty string as '' */ + resetStringInfo(&buf); + appendStringInfoString(&buf, item->name); + appendStringInfoString(&buf, " = ''\n"); + + errno = 0; + if (write(fd, buf.data, buf.len) != buf.len) + { + /* if write didn't set errno, assume problem is no disk space */ + if (errno == 0) + errno = ENOSPC; + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not write to file \"%s\": %m", filename))); + } + continue; + } resetStringInfo(&buf); -- 2.47.0
