On Fri, Jan 21, 2011 at 7:08 PM, Kevin Grittner
<kevin.gritt...@wicourts.gov> wrote:
> Robert Haas <robertmh...@gmail.com> wrote:
>> Jeff Janes <jeff.ja...@gmail.com> wrote:
>>> I found the following message somewhat confusing:
>>> ERROR: read-only property must be set before any query
>>
>> I think what we need here is two messages, this one and a similar
>> one that starts with "read-write property...".
>
> Done. I started out by being cute with plugging "only" or "write"
> into a single message, but then figured that might be hard on
> translators; so I went with two separate messages.
Make sense.
I committed the part of this that applies to SET TRANSACTION ISOLATION
LEVEL; the remainder is attached.
Upon further review, I am wondering if it wouldn't be simpler and more
logical to allow idempotent changes of these settings at any time, and
to restrict only changes that actually change something. It feels
really weird to allow changing these properties to their own values at
any time within a subtransaction, but not in a top-level transaction.
Why not:
if (source != PGC_S_OVERRIDE && newval && XactReadOnly)
{
if (IsSubTransaction())
cannot set transaction read-write mode inside a read-only transaction;
else if (FirstSnapshotSet)
transaction read-write mode must be set before any query;
else if (RecoveryInProgress())
cannot set transaction read-write mode during recovery;
}
That seems a lot more straightforward than this logic, and it saves
one translatable message, too.
I'm not bent on this route if people feel strongly otherwise, but it
seems like it'd be simpler without really losing anything.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 1e9bdc3..02825c2 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -544,6 +544,49 @@ show_log_timezone(void)
/*
+ * SET TRANSACTION READ ONLY and SET TRANSACTION READ WRITE
+ *
+ * These should be transaction properties which can be set in exactly the
+ * same points in time that transaction isolation may be set.
+ */
+bool
+assign_transaction_read_only(bool newval, bool doit, GucSource source)
+{
+ /* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
+ if (source != PGC_S_OVERRIDE)
+ {
+ /* 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")));
+ return false;
+ }
+ /* Top level transaction can't change this after first snapshot. */
+ if (FirstSnapshotSet && !IsSubTransaction())
+ {
+ ereport(GUC_complaint_elevel(source),
+ (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
+ errmsg(newval
+ ? "transaction read-only mode must be set before any query"
+ : "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 (newval == false && XactReadOnly && 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
*/
const char *
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ffff601..a7616df 100644
--- 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 7ac8ed8..2fc144e 100644
--- 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 84d1453..b91c9d8 100644
--- 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 17e830e..7c9638f 100644
--- 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
