From 602255e0479ffbd22e0a661f43dc71b005712e90 Mon Sep 17 00:00:00 2001 From: Masahiko Sawada Date: Tue, 28 Jun 2022 11:30:02 +0900 Subject: [PATCH v4] Introduce GUC_NOT_RESET flag. Previously, GUCs having GUC_NO_RESET_ALL flag were ignored when the RESET ALL command but were reset when the RESET command. So the transaction-property GUCs such as transaction_isolation could be reset after starting the transaction, which led to an assertion failure as per reported in bug #17385. This change adds a new flag GUC_NO_RESET and marks transaction-property GUCs GUC_NO_RESET. GUCs having GUC_NO_RESET are forbidden to be reset by the RESET command as well as be saved and restored. Although it's uncommon that the transaction-property GUCs are reset after BEGIN TRANSACTION, there may be functions that have any optimization that depends on transaction_read_only as follow: CREATE FUNCTION foo() RETURNS ... SET transaction_read_only = 1; Since this change would break them we don't back-patch it. --- doc/src/sgml/func.sgml | 4 +++ src/backend/utils/misc/guc.c | 21 +++++++++--- src/include/utils/guc.h | 2 ++ .../src/expected/plpgsql_transaction.out | 6 ---- .../plpgsql/src/sql/plpgsql_transaction.sql | 5 --- src/test/regress/expected/guc.out | 9 +++++ src/test/regress/expected/transactions.out | 34 +++++++++++++++++++ src/test/regress/sql/guc.sql | 5 +++ src/test/regress/sql/transactions.sql | 18 ++++++++++ 9 files changed, 89 insertions(+), 15 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 7b652460a1..e0f82f19e1 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -24965,6 +24965,10 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id')); NO_SHOW_ALL: parameters excluded from SHOW ALL commands. + + NO_RESET: parameters do not support + RESET commands. + NO_RESET_ALL: parameters excluded from RESET ALL commands. diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index a7cc49898b..ae3eed571d 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1752,7 +1752,7 @@ static struct config_bool ConfigureNamesBool[] = {"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, @@ -1771,7 +1771,7 @@ static struct config_bool ConfigureNamesBool[] = {"transaction_deferrable", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Whether to defer a read-only serializable transaction until 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, @@ -4808,7 +4808,7 @@ static struct config_enum ConfigureNamesEnum[] = {"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, @@ -7791,6 +7791,17 @@ set_config_option(const char *name, const char *value, } } + /* Disallow resetting and saving (and restoring) GUC_NO_RESET values */ + if ((record->flags & GUC_NO_RESET) && + (value == NULL || action == GUC_ACTION_SAVE)) + { + ereport(elevel, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("parameter \"%s\" cannot be reset", + 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 @@ -9947,7 +9958,7 @@ GetConfigOptionByName(const char *name, const char **varname, bool missing_ok) Datum pg_settings_get_flags(PG_FUNCTION_ARGS) { -#define MAX_GUC_FLAGS 5 +#define MAX_GUC_FLAGS 6 char *varname = TextDatumGetCString(PG_GETARG_DATUM(0)); struct config_generic *record; int cnt = 0; @@ -9962,6 +9973,8 @@ pg_settings_get_flags(PG_FUNCTION_ARGS) if (record->flags & GUC_EXPLAIN) flags[cnt++] = CStringGetTextDatum("EXPLAIN"); + if (record->flags & GUC_NO_RESET) + flags[cnt++] = CStringGetTextDatum("NO_RESET"); if (record->flags & GUC_NO_RESET_ALL) flags[cnt++] = CStringGetTextDatum("NO_RESET_ALL"); if (record->flags & GUC_NO_SHOW_ALL) diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index d5976ecbfb..9c626130ff 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -239,6 +239,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..759daf69be 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out +++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out @@ -576,16 +576,10 @@ BEGIN PERFORM 1; RAISE INFO '%', current_setting('transaction_isolation'); COMMIT; - SET TRANSACTION ISOLATION LEVEL REPEATABLE READ; - RESET TRANSACTION ISOLATION LEVEL; - PERFORM 1; - RAISE INFO '%', current_setting('transaction_isolation'); - COMMIT; END; $$; INFO: read committed INFO: repeatable read -INFO: read committed -- error cases DO LANGUAGE plpgsql $$ BEGIN diff --git a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql index 8d76d00daa..f8efdb01b6 100644 --- a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql +++ b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql @@ -481,11 +481,6 @@ BEGIN PERFORM 1; RAISE INFO '%', current_setting('transaction_isolation'); COMMIT; - SET TRANSACTION ISOLATION LEVEL REPEATABLE READ; - RESET TRANSACTION ISOLATION LEVEL; - PERFORM 1; - RAISE INFO '%', current_setting('transaction_isolation'); - COMMIT; END; $$; diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out index 3de6404ba5..b092ceebf1 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' = ANY(flags) AS explain, + 'NO_RESET' = ANY(flags) AS no_reset, 'NO_RESET_ALL' = ANY(flags) AS no_reset_all, 'NO_SHOW_ALL' = ANY(flags) AS no_show_all, 'NOT_IN_SAMPLE' = ANY(flags) AS not_in_sample, @@ -898,6 +899,14 @@ SELECT name FROM tab_settings_flags transaction_read_only (3 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) + -- NO_SHOW_ALL implies NOT_IN_SAMPLE. SELECT name FROM tab_settings_flags WHERE no_show_all AND NOT not_in_sample diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out index a46fa5d48a..852aee095f 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 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 = on; -- error +ERROR: parameter "transaction_read_only" cannot be reset -- 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..7af6c0ce24 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' = ANY(flags) AS explain, + 'NO_RESET' = ANY(flags) AS no_reset, 'NO_RESET_ALL' = ANY(flags) AS no_reset_all, 'NO_SHOW_ALL' = ANY(flags) AS no_show_all, 'NOT_IN_SAMPLE' = ANY(flags) AS not_in_sample, @@ -356,6 +357,10 @@ SELECT name FROM tab_settings_flags SELECT name FROM tab_settings_flags WHERE NOT no_show_all AND no_reset_all 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; -- NO_SHOW_ALL implies NOT_IN_SAMPLE. SELECT name FROM tab_settings_flags WHERE no_show_all AND NOT not_in_sample diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql index d71c3ce93e..f4246b14e7 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 = 'disappear'; -- should have members again SELECT * FROM xacttest; +-- Test that transaction characteristics cannot 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 = on; -- error -- Read-only tests -- 2.24.3 (Apple Git-128)