From 6f59777c65d557485e933a383ebc4c3fdfc1a2b7 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Sat, 22 Jan 2011 20:51:32 -0500 Subject: [PATCH] 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. --- src/backend/commands/variable.c | 53 ++++++++++++++- src/backend/utils/misc/guc.c | 29 --------- src/include/commands/variable.h | 2 + src/test/regress/expected/transactions.out | 75 ++++++++++++++++++++++ src/test/regress/sql/transactions.sql | 42 ++++++++++++ 5 files changed, 170 insertions(+), 31 deletions(-) diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c index 1e9bdc3f21..139148eec2 100644 --- 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 73b9f1b01c..2c95ef80c4 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 7ac8ed8f9f..2fc144e1e1 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 84d14537f1..b91c9d8e23 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 17e830e7a4..7c9638f05e 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 -- 2.40.0