Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1vFHx6-00AARh-EM for pgsql-hackers@arkaria.postgresql.org; Sat, 01 Nov 2025 20:11:55 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1vFHx5-0076ep-B2 for pgsql-hackers@arkaria.postgresql.org; Sat, 01 Nov 2025 20:11:54 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1vFHx4-0076eh-Ve for pgsql-hackers@lists.postgresql.org; Sat, 01 Nov 2025 20:11:54 +0000 Received: from sss.pgh.pa.us ([68.162.161.243]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1vFHx1-005T3h-1L for pgsql-hackers@lists.postgresql.org; Sat, 01 Nov 2025 20:11:53 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 5A1KBk2E117312; Sat, 1 Nov 2025 16:11:46 -0400 From: Tom Lane To: Andrei Klychkov cc: Jim Jones , Fujii Masao , pgsql-hackers@lists.postgresql.org Subject: Re: [PATCH] Fix ALTER SYSTEM empty string bug for GUC_LIST_QUOTE parameters In-reply-to: References: <38091678-2820-4bc8-8df7-b417562f7860@uni-muenster.de>

<1046095.1757007639@sss.pgh.pa.us> <1056071.1757010288@sss.pgh.pa.us> <1235943.1757022769@sss.pgh.pa.us> <12e88f4f-82f0-4153-ab19-7f1c5f317912@uni-muenster.de> <1528424.1757106377@sss.pgh.pa.us> Comments: In-reply-to Andrei Klychkov message dated "Mon, 08 Sep 2025 10:08:34 +0200" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <117193.1762027839.0@sss.pgh.pa.us> Date: Sat, 01 Nov 2025 16:11:46 -0400 Message-ID: <117311.1762027906@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <117193.1762027839.1@sss.pgh.pa.us> Andrei Klychkov 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 = ''; , > 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name*0="v6-0001-Allow-SET-list_guc-TO-NULL-to-specify-setting-the.p"; name*1="atch"; charset="us-ascii" Content-ID: <117193.1762027839.2@sss.pgh.pa.us> Content-Description: v6-0001-Allow-SET-list_guc-TO-NULL-to-specify-setting-the.patch Content-Transfer-Encoding: quoted-printable =46rom 78044b38eda13ee6abe49ac7cd4c689ad7f58b9c Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 1 Nov 2025 15:59:00 -0400 Subject: [PATCH v6] Allow "SET list_guc TO NULL" to specify setting the GU= C to empty. We have never had a SET syntax that allows setting a GUC_LIST_INPUT parameter to be an empty list. A locution such as SET search_path =3D ''; doesn't mean that; it means setting the GUC to contain a single item that is an empty string. (For search_path the net effect is much the same, because search_path ignores invalid schema names and '' must be invalid.) This is confusing, not least because configuration-file entries and the set_config() function can easily produce empty-list values. We debated making the above syntax work, but that would foreclose ever allowing empty-string items to be valid in list GUCs. While there isn't any obvious use-case for that today, it feels like the kind of restriction that might hurt someday. Instead, let's accept the forbidden-up-to-now value NULL and treat that as meaning an empty list. (An objection to this could be "what if we someday want to allow NULL as a GUC value?". That seems unlikely though, and even if we did allow it for scalar GUCs, we could continue to treat it as meaning an empty list for list GUCs.) --- doc/src/sgml/ref/alter_system.sgml | 2 ++ doc/src/sgml/ref/set.sgml | 24 +++++++++++++++--- src/backend/parser/gram.y | 20 +++++++++++++++ src/backend/utils/adt/ruleutils.c | 6 ++++- src/backend/utils/adt/varlena.c | 6 ++--- src/backend/utils/misc/guc_funcs.c | 38 ++++++++++++++++++++++++----- src/bin/pg_dump/dumputils.c | 6 ++++- src/bin/pg_dump/pg_dump.c | 6 ++++- src/test/regress/expected/guc.out | 22 +++++++++++++++++ src/test/regress/expected/rules.out | 2 ++ src/test/regress/sql/guc.sql | 9 +++++++ src/test/regress/sql/rules.sql | 1 + 12 files changed, 126 insertions(+), 16 deletions(-) diff --git a/doc/src/sgml/ref/alter_system.sgml b/doc/src/sgml/ref/alter_s= ystem.sgml index 1bde66d6ad2..65676e5c1c5 100644 --- a/doc/src/sgml/ref/alter_system.sgml +++ b/doc/src/sgml/ref/alter_system.sgml @@ -84,6 +84,8 @@ ALTER SYSTEM RESET ALL constants, identifiers, numbers, or comma-separated lists of these, as appropriate for the particular parameter. Values that are neither numbers nor valid identifiers must be quote= d. + If the parameter accepts a list of values, NULL + can be written to specify an empty list. DEFAULT can be written to specify removing the parameter and its value from postgresql.auto.conf. diff --git a/doc/src/sgml/ref/set.sgml b/doc/src/sgml/ref/set.sgml index 2218f54682e..16c7e9a7b26 100644 --- a/doc/src/sgml/ref/set.sgml +++ b/doc/src/sgml/ref/set.sgml @@ -21,8 +21,8 @@ PostgreSQL documentation = -SET [ SESSION | LOCAL ] configuration_pa= rameter { TO | =3D } { valu= e | 'value' |= DEFAULT } -SET [ SESSION | LOCAL ] TIME ZONE { valu= e | 'value' |= LOCAL | DEFAULT } +SET [ SESSION | LOCAL ] configuration_pa= rameter { TO | =3D } { valu= e [, ...] | DEFAULT } +SET [ SESSION | LOCAL ] TIME ZONE { valu= e | LOCAL | DEFAULT } = @@ -123,7 +123,7 @@ SET [ SESSION | LOCAL ] TIME ZONE { valueconfiguration_parameter - Name of a settable run-time parameter. Available parameters are + Name of a settable configuration parameter. Available parameters a= re documented in and below. @@ -133,9 +133,12 @@ SET [ SESSION | LOCAL ] TIME ZONE { valuevalue - New value of parameter. Values can be specified as string + New value of the parameter. Values can be specified as string constants, identifiers, numbers, or comma-separated lists of these, as appropriate for the particular parameter. + Values that are neither numbers nor valid identifiers must be quote= d. + If the parameter accepts a list of values, NULL + can be written to specify an empty list. DEFAULT can be written to specify resetting the parameter to its default value (that is, whatever value it would have had if no SET had been execu= ted @@ -283,6 +286,19 @@ SELECT setseed(value); Set the schema search path: SET search_path TO my_schema, public; + + Note that this is not the same as + +SET search_path TO 'my_schema, public'; + + which would have the effect of setting search_path + to contain a single, probably-nonexistent schema name. + + + + Set the list of temporary tablespace names to be empty: + +SET temp_tablespaces TO NULL; = diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index a4b29c822e8..50828a27053 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -1716,6 +1716,26 @@ generic_set: n->location =3D @3; $$ =3D n; } + | var_name TO NULL_P + { + VariableSetStmt *n =3D makeNode(VariableSetStmt); + + n->kind =3D VAR_SET_VALUE; + n->name =3D $1; + n->args =3D list_make1(makeNullAConst(@3)); + n->location =3D @3; + $$ =3D n; + } + | var_name '=3D' NULL_P + { + VariableSetStmt *n =3D makeNode(VariableSetStmt); + + n->kind =3D VAR_SET_VALUE; + n->name =3D $1; + n->args =3D list_make1(makeNullAConst(@3)); + n->location =3D @3; + $$ =3D n; + } | var_name TO DEFAULT { VariableSetStmt *n =3D makeNode(VariableSetStmt); diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/rul= eutils.c index 79ec136231b..5398679cce2 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -3087,7 +3087,8 @@ pg_get_functiondef(PG_FUNCTION_ARGS) * string literals. (The elements may be double-quoted as-is, * but we can't just feed them to the SQL parser; it would do * the wrong thing with elements that are zero-length or - * longer than NAMEDATALEN.) + * longer than NAMEDATALEN.) Also, we need a special case for + * empty lists. * * Variables that are not so marked should just be emitted as * simple string literals. If the variable is not known to @@ -3105,6 +3106,9 @@ pg_get_functiondef(PG_FUNCTION_ARGS) /* this shouldn't fail really */ elog(ERROR, "invalid list syntax in proconfig item"); } + /* Special case: represent an empty list as NULL */ + if (namelist =3D=3D NIL) + appendStringInfoString(&buf, "NULL"); foreach(lc, namelist) { char *curname =3D (char *) lfirst(lc); diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varle= na.c index 8d735786e51..3894457ab40 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -2753,7 +2753,7 @@ SplitIdentifierString(char *rawstring, char separato= r, nextp++; /* skip leading whitespace */ = if (*nextp =3D=3D '\0') - return true; /* allow empty string */ + return true; /* empty string represents empty list */ = /* At the top of the loop, we are at start of a new identifier. */ do @@ -2880,7 +2880,7 @@ SplitDirectoriesString(char *rawstring, char separat= or, nextp++; /* skip leading whitespace */ = if (*nextp =3D=3D '\0') - return true; /* allow empty string */ + return true; /* empty string represents empty list */ = /* At the top of the loop, we are at start of a new directory. */ do @@ -3001,7 +3001,7 @@ SplitGUCList(char *rawstring, char separator, nextp++; /* skip leading whitespace */ = if (*nextp =3D=3D '\0') - return true; /* allow empty string */ + return true; /* empty string represents empty list */ = /* At the top of the loop, we are at start of a new identifier. */ do diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/g= uc_funcs.c index 4f58fa3d4e0..9dbc5d3aeb9 100644 --- a/src/backend/utils/misc/guc_funcs.c +++ b/src/backend/utils/misc/guc_funcs.c @@ -210,12 +210,29 @@ flatten_set_variable_args(const char *name, List *ar= gs) else flags =3D 0; = - /* Complain if list input and non-list variable */ - if ((flags & GUC_LIST_INPUT) =3D=3D 0 && - list_length(args) !=3D 1) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("SET %s takes only one argument", name))); + /* + * Handle special cases for list input. + */ + if (flags & GUC_LIST_INPUT) + { + /* NULL represents an empty list. */ + if (list_length(args) =3D=3D 1) + { + Node *arg =3D (Node *) linitial(args); + + if (IsA(arg, A_Const) && + ((A_Const *) arg)->isnull) + return pstrdup(""); + } + } + else + { + /* Complain if list input and non-list variable. */ + if (list_length(args) !=3D 1) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("SET %s takes only one argument", name))); + } = initStringInfo(&buf); = @@ -246,6 +263,12 @@ flatten_set_variable_args(const char *name, List *arg= s) elog(ERROR, "unrecognized node type: %d", (int) nodeTag(arg)); con =3D (A_Const *) arg; = + /* Complain if NULL is used with a non-list variable. */ + if (con->isnull) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("NULL is an invalid value for %s", name))); + switch (nodeTag(&con->val)) { case T_Integer: @@ -269,6 +292,9 @@ flatten_set_variable_args(const char *name, List *args= ) Datum interval; char *intervalout; = + /* gram.y ensures this is only reachable for TIME ZONE */ + Assert(!(flags & GUC_LIST_QUOTE)); + typenameTypeIdAndMod(NULL, typeName, &typoid, &typmod); Assert(typoid =3D=3D INTERVALOID); = diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c index 05b84c0d6e7..2d22723aa91 100644 --- a/src/bin/pg_dump/dumputils.c +++ b/src/bin/pg_dump/dumputils.c @@ -781,7 +781,7 @@ SplitGUCList(char *rawstring, char separator, nextp++; /* skip leading whitespace */ = if (*nextp =3D=3D '\0') - return true; /* allow empty string */ + return true; /* empty string represents empty list */ = /* At the top of the loop, we are at start of a new identifier. */ do @@ -893,6 +893,7 @@ makeAlterConfigCommand(PGconn *conn, const char *confi= gitem, * elements as string literals. (The elements may be double-quoted as-i= s, * but we can't just feed them to the SQL parser; it would do the wrong * thing with elements that are zero-length or longer than NAMEDATALEN.) + * Also, we need a special case for empty lists. * * Variables that are not so marked should just be emitted as simple * string literals. If the variable is not known to @@ -908,6 +909,9 @@ makeAlterConfigCommand(PGconn *conn, const char *confi= gitem, /* this shouldn't fail really */ if (SplitGUCList(pos, ',', &namelist)) { + /* Special case: represent an empty list as NULL */ + if (*namelist =3D=3D NULL) + appendPQExpBufferStr(buf, "NULL"); for (nameptr =3D namelist; *nameptr; nameptr++) { if (nameptr !=3D namelist) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 47913178a93..a00918bacb4 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -13764,7 +13764,8 @@ dumpFunc(Archive *fout, const FuncInfo *finfo) * and then quote the elements as string literals. (The elements may * be double-quoted as-is, but we can't just feed them to the SQL * parser; it would do the wrong thing with elements that are - * zero-length or longer than NAMEDATALEN.) + * zero-length or longer than NAMEDATALEN.) Also, we need a special + * case for empty lists. * * Variables that are not so marked should just be emitted as simple * string literals. If the variable is not known to @@ -13780,6 +13781,9 @@ dumpFunc(Archive *fout, const FuncInfo *finfo) /* this shouldn't fail really */ if (SplitGUCList(pos, ',', &namelist)) { + /* Special case: represent an empty list as NULL */ + if (*namelist =3D=3D NULL) + appendPQExpBufferStr(q, "NULL"); for (nameptr =3D namelist; *nameptr; nameptr++) { if (nameptr !=3D namelist) diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected= /guc.out index 7f9e29c765c..d6fb879f500 100644 --- a/src/test/regress/expected/guc.out +++ b/src/test/regress/expected/guc.out @@ -31,6 +31,28 @@ SELECT '2006-08-13 12:34:56'::timestamptz; 2006-08-13 12:34:56-07 (1 row) = +-- Check handling of list GUCs +SET search_path =3D 'pg_catalog', Foo, 'Bar', ''; +SHOW search_path; + search_path = +---------------------------- + pg_catalog, foo, "Bar", "" +(1 row) + +SET search_path =3D null; -- means empty list +SHOW search_path; + search_path = +------------- + = +(1 row) + +SET search_path =3D null, null; -- syntax error +ERROR: syntax error at or near "," +LINE 1: SET search_path =3D null, null; + ^ +SET enable_seqscan =3D null; -- error +ERROR: NULL is an invalid value for enable_seqscan +RESET search_path; -- SET LOCAL has no effect outside of a transaction SET LOCAL vacuum_cost_delay TO 50; WARNING: SET LOCAL can only be used in transaction blocks diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expect= ed/rules.out index 77e25ca029e..2bf968ae3d3 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -3572,6 +3572,7 @@ CREATE FUNCTION func_with_set_params() RETURNS integ= er SET extra_float_digits TO 2 SET work_mem TO '4MB' SET datestyle to iso, mdy + SET temp_tablespaces to NULL SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '012= 34567890123456789012345678901234567890123456789012345678901234567890123456= 78901234567890123456789' IMMUTABLE STRICT; SELECT pg_get_functiondef('func_with_set_params()'::regprocedure); @@ -3585,6 +3586,7 @@ SELECT pg_get_functiondef('func_with_set_params()'::= regprocedure); SET extra_float_digits TO '2' = = + SET work_mem TO '4MB' = = + SET "DateStyle" TO 'iso, mdy' = = + + SET temp_tablespaces TO NULL = = + SET local_preload_libraries TO 'Mixed/Case', 'c:/''a"/path', '', '01234= 56789012345678901234567890123456789012345678901234567890123456789012345678= 901234567890123456789'+ AS $function$select 1;$function$ = = + = diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql index f65f84a2632..bafaf067e82 100644 --- a/src/test/regress/sql/guc.sql +++ b/src/test/regress/sql/guc.sql @@ -12,6 +12,15 @@ SHOW vacuum_cost_delay; SHOW datestyle; SELECT '2006-08-13 12:34:56'::timestamptz; = +-- Check handling of list GUCs +SET search_path =3D 'pg_catalog', Foo, 'Bar', ''; +SHOW search_path; +SET search_path =3D null; -- means empty list +SHOW search_path; +SET search_path =3D null, null; -- syntax error +SET enable_seqscan =3D null; -- error +RESET search_path; + -- SET LOCAL has no effect outside of a transaction SET LOCAL vacuum_cost_delay TO 50; SHOW vacuum_cost_delay; diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.s= ql index fdd3ff1d161..3f240bec7b0 100644 --- a/src/test/regress/sql/rules.sql +++ b/src/test/regress/sql/rules.sql @@ -1217,6 +1217,7 @@ CREATE FUNCTION func_with_set_params() RETURNS integ= er SET extra_float_digits TO 2 SET work_mem TO '4MB' SET datestyle to iso, mdy + SET temp_tablespaces to NULL SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '012= 34567890123456789012345678901234567890123456789012345678901234567890123456= 78901234567890123456789' IMMUTABLE STRICT; SELECT pg_get_functiondef('func_with_set_params()'::regprocedure); -- = 2.43.7 ------- =_aaaaaaaaaa0--