Code cleanup for assign_transaction_read_only.

As in commit fb4c5d2798730f60b102d775f22fb53c26a6445d on 2011-01-21,
this avoids spurious debug messages and allows idempotent changes at
any time.  Along the way, make assign_XactIsoLevel allow idempotent
changes even when not within a subtransaction, to be consistent with
the new coding of assign_transaction_read_only and because there's
no compelling reason to do otherwise.

Kevin Grittner, with some adjustments.

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 1e9bdc3f21833b01ebbf07bda48e1bb1f9bd7e52..139148eec2e6ff3431be039d90d306e717e68d93 100644 (file)
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -543,14 +543,63 @@ show_log_timezone(void)
 }
 
 
+/*
+ * SET TRANSACTION READ ONLY and SET TRANSACTION READ WRITE
+ *
+ * We allow idempotent changes (r/w -> r/w and r/o -> r/o) at any time, and
+ * we also always allow changes from read-write to read-only.  However,
+ * read-only to read-write may be changed only when source == PGC_S_OVERRIDE
+ * (i.e. we're aborting a read only transaction and restoring the previous
+ * setting) or in a top-level transaction that has not yet taken an initial
+ * snapshot.
+ */
+bool
+assign_transaction_read_only(bool newval, bool doit, GucSource source)
+{
+   if (source != PGC_S_OVERRIDE && newval == false && XactReadOnly)
+   {
+       /* Can't go to r/w mode inside a r/o transaction */
+       if (IsSubTransaction())
+       {
+           ereport(GUC_complaint_elevel(source),
+                   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                    errmsg("cannot set transaction read-write mode inside a read-only transaction")));
+           return false;
+       }
+       /* Top level transaction can't change to r/w after first snapshot. */
+       if (FirstSnapshotSet)
+       {
+           ereport(GUC_complaint_elevel(source),
+                   (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
+                    errmsg("transaction read-write mode must be set before any query")));
+           return false;
+       }
+       /* Can't go to r/w mode while recovery is still active */
+       if (RecoveryInProgress())
+       {
+           ereport(GUC_complaint_elevel(source),
+                   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                    errmsg("cannot set transaction read-write mode during recovery")));
+           return false;
+       }
+   }
+
+   return true;
+}
+
 /*
  * SET TRANSACTION ISOLATION LEVEL
+ *
+ * We allow idempotent changes at any time, but otherwise this can only be
+ * changed from a toplevel transaction that has not yet taken a snapshot, or
+ * when source == PGC_S_OVERRIDE (i.e. we're aborting a transaction and
+ * restoring the previously set value).
  */
 const char *
 assign_XactIsoLevel(const char *value, bool doit, GucSource source)
 {
    /* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
-   if (source != PGC_S_OVERRIDE)
+   if (source != PGC_S_OVERRIDE && strcmp(value, XactIsoLevel_string) != 0)
    {
        if (FirstSnapshotSet)
        {
@@ -560,7 +609,7 @@ assign_XactIsoLevel(const char *value, bool doit, GucSource source)
            return NULL;
        }
        /* We ignore a subtransaction setting it to the existing value. */
-       if (IsSubTransaction() && strcmp(value, XactIsoLevel_string) != 0)
+       if (IsSubTransaction())
        {
            ereport(GUC_complaint_elevel(source),
                    (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 73b9f1b01ca7ddfdd7b31a29d4e939d2d91e911b..2c95ef80c458f82922eeb280c82199d0e63a8996 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -168,7 +168,6 @@ static bool assign_bonjour(bool newval, bool doit, GucSource source);
 static bool assign_ssl(bool newval, bool doit, GucSource source);
 static bool assign_stage_log_stats(bool newval, bool doit, GucSource source);
 static bool assign_log_stats(bool newval, bool doit, GucSource source);
-static bool assign_transaction_read_only(bool newval, bool doit, GucSource source);
 static const char *assign_canonical_path(const char *newval, bool doit, GucSource source);
 static const char *assign_timezone_abbreviations(const char *newval, bool doit, GucSource source);
 static const char *show_archive_command(void);
@@ -7843,34 +7842,6 @@ assign_log_stats(bool newval, bool doit, GucSource source)
    return true;
 }
 
-static bool
-assign_transaction_read_only(bool newval, bool doit, GucSource source)
-{
-   /* Can't go to r/w mode inside a r/o transaction */
-   if (newval == false && XactReadOnly && IsSubTransaction())
-   {
-       ereport(GUC_complaint_elevel(source),
-               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                errmsg("cannot set transaction read-write mode inside a read-only transaction")));
-       /* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
-       if (source != PGC_S_OVERRIDE)
-           return false;
-   }
-
-   /* Can't go to r/w mode while recovery is still active */
-   if (newval == false && XactReadOnly && RecoveryInProgress())
-   {
-       ereport(GUC_complaint_elevel(source),
-               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-         errmsg("cannot set transaction read-write mode during recovery")));
-       /* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
-       if (source != PGC_S_OVERRIDE)
-           return false;
-   }
-
-   return true;
-}
-
 static const char *
 assign_canonical_path(const char *newval, bool doit, GucSource source)
 {

diff --git a/src/include/commands/variable.h b/src/include/commands/variable.h
index 7ac8ed8f9f7a9e0e9dc9a8deb7e470440e8f4306..2fc144e1e1276e235b66d58acf9bb639c24522e5 100644 (file)
--- a/src/include/commands/variable.h
+++ b/src/include/commands/variable.h
@@ -21,6 +21,8 @@ extern const char *show_timezone(void);
 extern const char *assign_log_timezone(const char *value,
                    bool doit, GucSource source);
 extern const char *show_log_timezone(void);
+extern bool assign_transaction_read_only(bool value,
+                   bool doit, GucSource source);
 extern const char *assign_XactIsoLevel(const char *value,
                    bool doit, GucSource source);
 extern const char *show_XactIsoLevel(void);

diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index 84d14537f14ba575a5b57666bf37231576595c14..b91c9d8e23716fe66eb693f5c34eec85b757f0a9 100644 (file)
--- a/src/test/regress/expected/transactions.out
+++ b/src/test/regress/expected/transactions.out
@@ -43,6 +43,81 @@ SELECT * FROM aggtest;
 -- Read-only tests
 CREATE TABLE writetest (a int);
 CREATE TEMPORARY TABLE temptest (a int);
+BEGIN;
+SET TRANSACTION READ ONLY; -- ok
+SELECT * FROM writetest; -- ok
+ a 
+---
+(0 rows)
+
+SET TRANSACTION READ WRITE; --fail
+ERROR:  transaction read-write mode must be set before any query
+COMMIT;
+BEGIN;
+SET TRANSACTION READ ONLY; -- ok
+SET TRANSACTION READ WRITE; -- ok
+SET TRANSACTION READ ONLY; -- ok
+SELECT * FROM writetest; -- ok
+ a 
+---
+(0 rows)
+
+SAVEPOINT x;
+SET TRANSACTION READ ONLY; -- ok
+SELECT * FROM writetest; -- ok
+ a 
+---
+(0 rows)
+
+SET TRANSACTION READ ONLY; -- ok
+SET TRANSACTION READ WRITE; --fail
+ERROR:  cannot set transaction read-write mode inside a read-only transaction
+COMMIT;
+BEGIN;
+SET TRANSACTION READ WRITE; -- ok
+SAVEPOINT x;
+SET TRANSACTION READ WRITE; -- ok
+SET TRANSACTION READ ONLY; -- ok
+SELECT * FROM writetest; -- ok
+ a 
+---
+(0 rows)
+
+SET TRANSACTION READ ONLY; -- ok
+SET TRANSACTION READ WRITE; --fail
+ERROR:  cannot set transaction read-write mode inside a read-only transaction
+COMMIT;
+BEGIN;
+SET TRANSACTION READ WRITE; -- ok
+SAVEPOINT x;
+SET TRANSACTION READ ONLY; -- ok
+SELECT * FROM writetest; -- ok
+ a 
+---
+(0 rows)
+
+ROLLBACK TO SAVEPOINT x;
+SHOW transaction_read_only;  -- off
+ transaction_read_only 
+-----------------------
+ off
+(1 row)
+
+SAVEPOINT y;
+SET TRANSACTION READ ONLY; -- ok
+SELECT * FROM writetest; -- ok
+ a 
+---
+(0 rows)
+
+RELEASE SAVEPOINT y;
+SHOW transaction_read_only;  -- off
+ transaction_read_only 
+-----------------------
+ off
+(1 row)
+
+COMMIT;
 SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY;
 DROP TABLE writetest; -- fail
 ERROR:  cannot execute DROP TABLE in a read-only transaction

diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql
index 17e830e7a4cb7c4d96c62250dde4de22d88bd006..7c9638f05e26f2d9341ed5ad274736cbf417d229 100644 (file)
--- a/src/test/regress/sql/transactions.sql
+++ b/src/test/regress/sql/transactions.sql
@@ -39,6 +39,48 @@ SELECT * FROM aggtest;
 CREATE TABLE writetest (a int);
 CREATE TEMPORARY TABLE temptest (a int);
 
+BEGIN;
+SET TRANSACTION READ ONLY; -- ok
+SELECT * FROM writetest; -- ok
+SET TRANSACTION READ WRITE; --fail
+COMMIT;
+
+BEGIN;
+SET TRANSACTION READ ONLY; -- ok
+SET TRANSACTION READ WRITE; -- ok
+SET TRANSACTION READ ONLY; -- ok
+SELECT * FROM writetest; -- ok
+SAVEPOINT x;
+SET TRANSACTION READ ONLY; -- ok
+SELECT * FROM writetest; -- ok
+SET TRANSACTION READ ONLY; -- ok
+SET TRANSACTION READ WRITE; --fail
+COMMIT;
+
+BEGIN;
+SET TRANSACTION READ WRITE; -- ok
+SAVEPOINT x;
+SET TRANSACTION READ WRITE; -- ok
+SET TRANSACTION READ ONLY; -- ok
+SELECT * FROM writetest; -- ok
+SET TRANSACTION READ ONLY; -- ok
+SET TRANSACTION READ WRITE; --fail
+COMMIT;
+
+BEGIN;
+SET TRANSACTION READ WRITE; -- ok
+SAVEPOINT x;
+SET TRANSACTION READ ONLY; -- ok
+SELECT * FROM writetest; -- ok
+ROLLBACK TO SAVEPOINT x;
+SHOW transaction_read_only;  -- off
+SAVEPOINT y;
+SET TRANSACTION READ ONLY; -- ok
+SELECT * FROM writetest; -- ok
+RELEASE SAVEPOINT y;
+SHOW transaction_read_only;  -- off
+COMMIT;
+
 SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY;
 
 DROP TABLE writetest; -- fail