Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ocCDm-00008s-MU for pgsql-bugs@arkaria.postgresql.org; Sat, 24 Sep 2022 20:57:58 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1ocCDl-0002Qx-GL for pgsql-bugs@arkaria.postgresql.org; Sat, 24 Sep 2022 20:57:57 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ocCDk-0002Qe-Vb for pgsql-bugs@lists.postgresql.org; Sat, 24 Sep 2022 20:57:57 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ocCDi-0006XO-5U for pgsql-bugs@lists.postgresql.org; Sat, 24 Sep 2022 20:57:55 +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 28OKvp8s2604498; Sat, 24 Sep 2022 16:57:51 -0400 From: Tom Lane To: Masahiko Sawada cc: Michael Paquier , Justin Pryzby , Dmitry Koval , Dilip Kumar , andrewbille@gmail.com, pgsql-bugs@lists.postgresql.org, Kyotaro Horiguchi Subject: Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end In-reply-to: References: <20220628003720.GG19662@telsasoft.com> Comments: In-reply-to Masahiko Sawada message dated "Thu, 22 Sep 2022 11:03:24 +0900" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <2604432.1664053005.0@sss.pgh.pa.us> Date: Sat, 24 Sep 2022 16:57:51 -0400 Message-ID: <2604497.1664053071@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: <2604432.1664053005.1@sss.pgh.pa.us> Content-Transfer-Encoding: quoted-printable Masahiko Sawada writes: > I've attached patches. Per the cfbot, this incorrectly updates the plpgsql_transaction test. Here's an updated version that fixes that. The proposed error message for the GUC_ACTION_SAVE case, errmsg("parameter \"%s\" cannot be set temporarily or in functions loc= ally", does not seem like very good English to me. In the attached I changed it to errmsg("parameter \"%s\" cannot be set temporarily or locally in funct= ions", but that isn't great either: it seems redundant and confusing. I think we could just make it errmsg("parameter \"%s\" cannot be set locally in functions", because that's really the only case users should ever see. Other than the message-wording issue, I think v6-0001 is OK. > I did the reorganization of GUC flags in a separate patch (0002 patch) > since it's not relevant with the new flag GUC_NO_RESET. 0002 seems quite invasive and hard to review compared to what it accomplishes. I think we should just keep the flags in the same order, stick in GUC_NO_RESET in front of GUC_NO_RESET_ALL, and renumber the flag values as needed. I did not change 0002 below, though. regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="v6-0001-Introduce-GUC_NO_RESET-flag.patch"; charset="us-ascii" Content-ID: <2604432.1664053005.2@sss.pgh.pa.us> Content-Description: v6-0001-Introduce-GUC_NO_RESET-flag.patch Content-Transfer-Encoding: quoted-printable diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index e1fe4fec57..546213fa93 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -24353,6 +24353,12 @@ SELECT collation for ('foo' COLLATE "de_DE"); SHOW ALL commands. + + NO_RESET + Parameters with this flag do not support + RESET commands. + + NO_RESET_ALL Parameters with this flag are excluded from diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 8c2e08fad6..29ec6d886c 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -3243,6 +3243,26 @@ set_config_option_ext(const char *name, const char = *value, } } = + /* Disallow resetting and saving GUC_NO_RESET values */ + if (record->flags & GUC_NO_RESET) + { + if (value =3D=3D NULL) + { + ereport(elevel, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("parameter \"%s\" cannot be reset", name))); + return 0; + } + if (action =3D=3D GUC_ACTION_SAVE) + { + ereport(elevel, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("parameter \"%s\" cannot be set temporarily or locally in fu= nctions", + name))); + return 0; + } + } + /* * Should we set reset/stacked values? (If so, the behavior is not * transactional.) This is done either when we get a default value from diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/g= uc_funcs.c index 3d2df18659..ffc71726f9 100644 --- a/src/backend/utils/misc/guc_funcs.c +++ b/src/backend/utils/misc/guc_funcs.c @@ -141,9 +141,6 @@ ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopL= evel) WarnNoTransactionBlock(isTopLevel, "SET LOCAL"); /* fall through */ case VAR_RESET: - if (strcmp(stmt->name, "transaction_isolation") =3D=3D 0) - WarnNoTransactionBlock(isTopLevel, "RESET TRANSACTION"); - (void) set_config_option(stmt->name, NULL, (superuser() ? PGC_SUSET : PGC_USERSET), @@ -539,7 +536,7 @@ ShowAllGUCConfig(DestReceiver *dest) Datum pg_settings_get_flags(PG_FUNCTION_ARGS) { -#define MAX_GUC_FLAGS 5 +#define MAX_GUC_FLAGS 6 char *varname =3D TextDatumGetCString(PG_GETARG_DATUM(0)); struct config_generic *record; int cnt =3D 0; @@ -554,6 +551,8 @@ pg_settings_get_flags(PG_FUNCTION_ARGS) = if (record->flags & GUC_EXPLAIN) flags[cnt++] =3D CStringGetTextDatum("EXPLAIN"); + if (record->flags & GUC_NO_RESET) + flags[cnt++] =3D CStringGetTextDatum("NO_RESET"); if (record->flags & GUC_NO_RESET_ALL) flags[cnt++] =3D CStringGetTextDatum("NO_RESET_ALL"); if (record->flags & GUC_NO_SHOW_ALL) diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/= guc_tables.c index ab3140ff3a..fda3f9befb 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1505,7 +1505,7 @@ struct config_bool ConfigureNamesBool[] =3D {"transaction_read_only", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Sets the current transaction's read-only status."), NULL, - GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE + GUC_NO_RESET | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_= FILE }, &XactReadOnly, false, @@ -1524,7 +1524,7 @@ struct config_bool ConfigureNamesBool[] =3D {"transaction_deferrable", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Whether to defer a read-only serializable transaction un= til it can be executed with no possible serialization failures."), NULL, - GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE + GUC_NO_RESET | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_= FILE }, &XactDeferrable, false, @@ -3606,7 +3606,7 @@ struct config_real ConfigureNamesReal[] =3D {"seed", PGC_USERSET, UNGROUPED, gettext_noop("Sets the seed for random-number generation."), NULL, - GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_= IN_FILE + GUC_NO_SHOW_ALL | GUC_NO_RESET | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE = | GUC_DISALLOW_IN_FILE }, &phony_random_seed, 0.0, -1.0, 1.0, @@ -4557,7 +4557,7 @@ struct config_enum ConfigureNamesEnum[] =3D {"transaction_isolation", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Sets the current transaction's isolation level."), NULL, - GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE + GUC_NO_RESET | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_= FILE }, &XactIsoLevel, XACT_READ_COMMITTED, isolation_level_options, diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index e426dd757d..a846500bc5 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -238,6 +238,8 @@ typedef enum */ #define GUC_RUNTIME_COMPUTED 0x200000 = +#define GUC_NO_RESET 0x400000 /* not support RESET and save */ + #define GUC_UNIT (GUC_UNIT_MEMORY | GUC_UNIT_TIME) = = diff --git a/src/pl/plpgsql/src/expected/plpgsql_transaction.out b/src/pl/= plpgsql/src/expected/plpgsql_transaction.out index 254e5b7a70..adff10fa6d 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out +++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out @@ -576,8 +576,7 @@ BEGIN PERFORM 1; RAISE INFO '%', current_setting('transaction_isolation'); COMMIT; - SET TRANSACTION ISOLATION LEVEL REPEATABLE READ; - RESET TRANSACTION ISOLATION LEVEL; + SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; PERFORM 1; RAISE INFO '%', current_setting('transaction_isolation'); COMMIT; @@ -585,7 +584,7 @@ END; $$; INFO: read committed INFO: repeatable read -INFO: read committed +INFO: serializable -- error cases DO LANGUAGE plpgsql $$ BEGIN diff --git a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql b/src/pl/plpgs= ql/src/sql/plpgsql_transaction.sql index 8d76d00daa..c73fca7e03 100644 --- a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql +++ b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql @@ -481,8 +481,7 @@ BEGIN PERFORM 1; RAISE INFO '%', current_setting('transaction_isolation'); COMMIT; - SET TRANSACTION ISOLATION LEVEL REPEATABLE READ; - RESET TRANSACTION ISOLATION LEVEL; + SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; PERFORM 1; RAISE INFO '%', current_setting('transaction_isolation'); COMMIT; diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected= /guc.out index 3de6404ba5..2914b950b4 100644 --- a/src/test/regress/expected/guc.out +++ b/src/test/regress/expected/guc.out @@ -839,6 +839,7 @@ SELECT pg_settings_get_flags('does_not_exist'); = CREATE TABLE tab_settings_flags AS SELECT name, category, 'EXPLAIN' =3D ANY(flags) AS explain, + 'NO_RESET' =3D ANY(flags) AS no_reset, 'NO_RESET_ALL' =3D ANY(flags) AS no_reset_all, 'NO_SHOW_ALL' =3D ANY(flags) AS no_show_all, 'NOT_IN_SAMPLE' =3D ANY(flags) AS not_in_sample, @@ -906,4 +907,12 @@ SELECT name FROM tab_settings_flags ------ (0 rows) = +-- NO_RESET implies NO_RESET_ALL. +SELECT name FROM tab_settings_flags + WHERE no_reset AND NOT no_reset_all + ORDER BY 1; + name = +------ +(0 rows) + DROP TABLE tab_settings_flags; diff --git a/src/test/regress/expected/transactions.out b/src/test/regress= /expected/transactions.out index a46fa5d48a..9351d1d134 100644 --- a/src/test/regress/expected/transactions.out +++ b/src/test/regress/expected/transactions.out @@ -44,6 +44,40 @@ SELECT * FROM xacttest; 777 | 777.777 (5 rows) = +-- Test that transaction characteristics cannot be reset. +BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; +SELECT COUNT(*) FROM xacttest; + count = +------- + 5 +(1 row) + +RESET transaction_isolation; -- error +ERROR: parameter "transaction_isolation" cannot be reset +END; +BEGIN TRANSACTION READ ONLY; +SELECT COUNT(*) FROM xacttest; + count = +------- + 5 +(1 row) + +RESET transaction_read_only; -- error +ERROR: parameter "transaction_read_only" cannot be reset +END; +BEGIN TRANSACTION DEFERRABLE; +SELECT COUNT(*) FROM xacttest; + count = +------- + 5 +(1 row) + +RESET transaction_deferrable; -- error +ERROR: parameter "transaction_deferrable" cannot be reset +END; +CREATE FUNCTION errfunc() RETURNS int LANGUAGE SQL AS 'SELECT 1' +SET transaction_read_only =3D on; -- error +ERROR: parameter "transaction_read_only" cannot be set temporarily or lo= cally in functions -- Read-only tests CREATE TABLE writetest (a int); CREATE TEMPORARY TABLE temptest (a int); diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql index d5db101e48..d40d0c178f 100644 --- a/src/test/regress/sql/guc.sql +++ b/src/test/regress/sql/guc.sql @@ -324,6 +324,7 @@ SELECT pg_settings_get_flags(NULL); SELECT pg_settings_get_flags('does_not_exist'); CREATE TABLE tab_settings_flags AS SELECT name, category, 'EXPLAIN' =3D ANY(flags) AS explain, + 'NO_RESET' =3D ANY(flags) AS no_reset, 'NO_RESET_ALL' =3D ANY(flags) AS no_reset_all, 'NO_SHOW_ALL' =3D ANY(flags) AS no_show_all, 'NOT_IN_SAMPLE' =3D ANY(flags) AS not_in_sample, @@ -360,4 +361,8 @@ SELECT name FROM tab_settings_flags SELECT name FROM tab_settings_flags WHERE no_show_all AND NOT not_in_sample ORDER BY 1; +-- NO_RESET implies NO_RESET_ALL. +SELECT name FROM tab_settings_flags + WHERE no_reset AND NOT no_reset_all + ORDER BY 1; DROP TABLE tab_settings_flags; diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/= transactions.sql index d71c3ce93e..7ee5f6aaa5 100644 --- a/src/test/regress/sql/transactions.sql +++ b/src/test/regress/sql/transactions.sql @@ -35,6 +35,24 @@ SELECT oid FROM pg_class WHERE relname =3D 'disappear'; -- should have members again SELECT * FROM xacttest; = +-- Test that transaction characteristics cannot be reset. +BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; +SELECT COUNT(*) FROM xacttest; +RESET transaction_isolation; -- error +END; + +BEGIN TRANSACTION READ ONLY; +SELECT COUNT(*) FROM xacttest; +RESET transaction_read_only; -- error +END; + +BEGIN TRANSACTION DEFERRABLE; +SELECT COUNT(*) FROM xacttest; +RESET transaction_deferrable; -- error +END; + +CREATE FUNCTION errfunc() RETURNS int LANGUAGE SQL AS 'SELECT 1' +SET transaction_read_only =3D on; -- error = -- Read-only tests = ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="v6-0002-Reorganize-GUC_XXX-flag-values.patch"; charset="us-ascii" Content-ID: <2604432.1664053005.3@sss.pgh.pa.us> Content-Description: v6-0002-Reorganize-GUC_XXX-flag-values.patch Content-Transfer-Encoding: quoted-printable =46rom b207f4a2560a19435598d442a0ca3f5e43aba532 Mon Sep 17 00:00:00 2001 From: Masahiko Sawada Date: Wed, 21 Sep 2022 21:07:20 +0900 Subject: [PATCH v5 2/2] Reorganize GUC_XXX flag values. Previously, we defined GUC flags for units for memory and time followed by flags for properties. This change makes more room for properties by inverting the order. --- src/include/utils/guc.h | 56 ++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index a846500bc5..8daeb877ad 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -204,41 +204,39 @@ typedef enum /* * bit values in "flags" of a GUC variable */ -#define GUC_LIST_INPUT 0x0001 /* input can be list format */ -#define GUC_LIST_QUOTE 0x0002 /* double-quote list elements */ -#define GUC_NO_SHOW_ALL 0x0004 /* exclude from SHOW ALL */ -#define GUC_NO_RESET_ALL 0x0008 /* exclude from RESET ALL */ -#define GUC_REPORT 0x0010 /* auto-report changes to client */ -#define GUC_NOT_IN_SAMPLE 0x0020 /* not in postgresql.conf.sample */ -#define GUC_DISALLOW_IN_FILE 0x0040 /* can't set in postgresql.conf */ -#define GUC_CUSTOM_PLACEHOLDER 0x0080 /* placeholder for custom variable = */ -#define GUC_SUPERUSER_ONLY 0x0100 /* show only to superusers */ -#define GUC_IS_NAME 0x0200 /* limit string to NAMEDATALEN-1 */ -#define GUC_NOT_WHILE_SEC_REST 0x0400 /* can't set if security restricted= */ -#define GUC_DISALLOW_IN_AUTO_FILE 0x0800 /* can't set in +#define GUC_UNIT_KB 0x000001 /* value is in kilobytes */ +#define GUC_UNIT_BLOCKS 0x000002 /* value is in blocks */ +#define GUC_UNIT_XBLOCKS 0x000003 /* value is in xlog blocks */ +#define GUC_UNIT_MB 0x000004 /* value is in megabytes */ +#define GUC_UNIT_BYTE 0x000005 /* value is in bytes */ +#define GUC_UNIT_MEMORY 0x00000F /* mask for size-related units */ + +#define GUC_UNIT_MS 0x000010 /* value is in milliseconds */ +#define GUC_UNIT_S 0x000020 /* value is in seconds */ +#define GUC_UNIT_MIN 0x000030 /* value is in minutes */ +#define GUC_UNIT_TIME 0x0000F0 /* mask for time-related units */ + +#define GUC_LIST_INPUT 0x000100 /* input can be list format */ +#define GUC_LIST_QUOTE 0x000200 /* double-quote list elements */ +#define GUC_NO_SHOW_ALL 0x000400 /* exclude from SHOW ALL */ +#define GUC_NO_RESET 0x000800 /* not support RESET and save */ +#define GUC_NO_RESET_ALL 0x001000 /* exclude from RESET ALL */ +#define GUC_REPORT 0x002000 /* auto-report changes to client */ +#define GUC_NOT_IN_SAMPLE 0x004000 /* not in postgresql.conf.sample */ +#define GUC_DISALLOW_IN_FILE 0x008000 /* can't set in postgresql.conf */ +#define GUC_CUSTOM_PLACEHOLDER 0x010000 /* placeholder for custom variabl= e */ +#define GUC_SUPERUSER_ONLY 0x020000 /* show only to superusers */ +#define GUC_IS_NAME 0x040000 /* limit string to NAMEDATALEN-1 */ +#define GUC_NOT_WHILE_SEC_REST 0x080000 /* can't set if security restrict= ed */ +#define GUC_DISALLOW_IN_AUTO_FILE 0x100000 /* can't set in * PG_AUTOCONF_FILENAME */ - -#define GUC_UNIT_KB 0x1000 /* value is in kilobytes */ -#define GUC_UNIT_BLOCKS 0x2000 /* value is in blocks */ -#define GUC_UNIT_XBLOCKS 0x3000 /* value is in xlog blocks */ -#define GUC_UNIT_MB 0x4000 /* value is in megabytes */ -#define GUC_UNIT_BYTE 0x5000 /* value is in bytes */ -#define GUC_UNIT_MEMORY 0xF000 /* mask for size-related units */ - -#define GUC_UNIT_MS 0x10000 /* value is in milliseconds */ -#define GUC_UNIT_S 0x20000 /* value is in seconds */ -#define GUC_UNIT_MIN 0x30000 /* value is in minutes */ -#define GUC_UNIT_TIME 0xF0000 /* mask for time-related units */ - -#define GUC_EXPLAIN 0x100000 /* include in explain */ +#define GUC_EXPLAIN 0x200000 /* include in explain */ = /* * GUC_RUNTIME_COMPUTED is intended for runtime-computed GUCs that are on= ly * available via 'postgres -C' if the server is not running. */ -#define GUC_RUNTIME_COMPUTED 0x200000 - -#define GUC_NO_RESET 0x400000 /* not support RESET and save */ +#define GUC_RUNTIME_COMPUTED 0x400000 = #define GUC_UNIT (GUC_UNIT_MEMORY | GUC_UNIT_TIME) = -- = 2.31.1 ------- =_aaaaaaaaaa0--