Re: BUG #17385: "RESET transaction_isolation" inside serializable transaction causes Assert at the transaction end

Masahiko Sawada <sawada.mshk@gmail.com> 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 locally",

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 functions",

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

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..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 == 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 locally in functions",
+                            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 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[] =
         {"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
possibleserialization 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..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/plpgsql/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'          = 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..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 = on; -- error
+ERROR:  parameter "transaction_read_only" cannot be set temporarily or locally 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'          = 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

From b207f4a2560a19435598d442a0ca3f5e43aba532 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
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 variable */
+#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 restricted */
+#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 only
  * 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