From f2729d1d851dca3099d3f59372473ffddb28147a Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Wed, 21 Sep 2022 21:06:14 +0900
Subject: [PATCH v5 1/2] Introduce GUC_NO_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 and seed 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;

This change prohibits this case too.

Since this change would break the compatibility of functions we don't
back-patch it.
---
 doc/src/sgml/func.sgml                        |  6 ++++
 src/backend/utils/misc/guc.c                  | 20 +++++++++++
 src/backend/utils/misc/guc_funcs.c            |  7 ++--
 src/backend/utils/misc/guc_tables.c           |  8 ++---
 src/include/utils/guc.h                       |  2 ++
 .../src/expected/plpgsql_transaction.out      |  5 ---
 .../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 ++++++++++
 11 files changed, 101 insertions(+), 18 deletions(-)

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");
        <command>SHOW ALL</command> commands.
       </entry>
      </row>
+     <row>
+      <entry><literal>NO_RESET</literal></entry>
+      <entry>Parameters with this flag do not support
+      <command>RESET</command> commands.
+      </entry>
+     </row>
      <row>
       <entry><literal>NO_RESET_ALL</literal></entry>
       <entry>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..fba6cada6a 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 == NULL)
+		{
+			ereport(elevel,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("parameter \"%s\" cannot be reset", name)));
+			return 0;
+		}
+		if (action == GUC_ACTION_SAVE)
+		{
+			ereport(elevel,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("parameter \"%s\" cannot be set temporarily or in functions locally",
+							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/guc_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 isTopLevel)
 				WarnNoTransactionBlock(isTopLevel, "SET LOCAL");
 			/* fall through */
 		case VAR_RESET:
-			if (strcmp(stmt->name, "transaction_isolation") == 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 = TextDatumGetCString(PG_GETARG_DATUM(0));
 	struct config_generic *record;
 	int			cnt = 0;
@@ -554,6 +551,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/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 550e95056c..7a80c68673 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[] =
 		{"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[] =
 		{"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,
@@ -3606,7 +3606,7 @@ struct config_real ConfigureNamesReal[] =
 		{"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[] =
 		{"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..b4cd88ea46 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
@@ -576,11 +576,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;
 $$;
 INFO:  read committed
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..d11e0cd48f 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,
@@ -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..03b3c23377 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 = on; -- error
+ERROR:  parameter "transaction_read_only" cannot be set temporarily or in functions locally
 -- 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..d0146a7cf1 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,
@@ -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 = '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 = on; -- error
 
 -- Read-only tests
 
-- 
2.31.1

