READ ONLY fixes
Attached is a rebased roll-up of the 3 and 3a patches from last month.
-Kevin
Attachments:
read-only-4.patchtext/plain; name=read-only-4.patchDownload
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -544,29 +544,72 @@ 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("read-only property 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
*/
+extern char *XactIsoLevel_string; /* in guc.c */
const char *
assign_XactIsoLevel(const char *value, bool doit, GucSource source)
{
- if (FirstSnapshotSet)
+ /* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
+ if (source != PGC_S_OVERRIDE)
{
- ereport(GUC_complaint_elevel(source),
- (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
- errmsg("SET TRANSACTION ISOLATION LEVEL must be called before any query")));
- /* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
- if (source != PGC_S_OVERRIDE)
+ if (FirstSnapshotSet)
+ {
+ ereport(GUC_complaint_elevel(source),
+ (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
+ errmsg("SET TRANSACTION ISOLATION LEVEL must be called before any query")));
return NULL;
- }
- else if (IsSubTransaction())
- {
- ereport(GUC_complaint_elevel(source),
- (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
- errmsg("SET TRANSACTION ISOLATION LEVEL must not be called in a subtransaction")));
- /* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
- if (source != PGC_S_OVERRIDE)
+ }
+ /* We ignore a subtransaction setting it to the existing value. */
+ if (IsSubTransaction() && strcmp(value, XactIsoLevel_string) != 0)
+ {
+ ereport(GUC_complaint_elevel(source),
+ (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
+ errmsg("SET TRANSACTION ISOLATION LEVEL must not be called in a subtransaction")));
return NULL;
+ }
}
if (strcmp(value, "serializable") == 0)
--- 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);
@@ -425,7 +424,6 @@ static int server_version_num;
static char *timezone_string;
static char *log_timezone_string;
static char *timezone_abbreviations_string;
-static char *XactIsoLevel_string;
static char *custom_variable_classes;
static int max_function_args;
static int max_index_keys;
@@ -440,6 +438,7 @@ static int effective_io_concurrency;
/* should be static, but commands/variable.c needs to get at these */
char *role_string;
char *session_authorization_string;
+char *XactIsoLevel_string;
/*
@@ -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)
{
--- 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);
--- a/src/test/regress/expected/transactions.out
+++ b/src/test/regress/expected/transactions.out
@@ -43,6 +43,50 @@ 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: read-only property 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;
SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY;
DROP TABLE writetest; -- fail
ERROR: cannot execute DROP TABLE in a read-only transaction
--- a/src/test/regress/sql/transactions.sql
+++ b/src/test/regress/sql/transactions.sql
@@ -39,6 +39,34 @@ 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;
+
SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY;
DROP TABLE writetest; -- fail
On Mon, Jan 10, 2011 at 8:27 AM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
Attached is a rebased roll-up of the 3 and 3a patches from last month.
-Kevin
Hi Kevin,
A review:
The main motivation for the patch is to allow future optimization of
read-only transactions, by preventing them from changing back to read
write once their read-onliness has been noticed. This will also
probably bring closer compliance with SQL standards.
The patch was mostly reviewed by others at the time it was proposed and altered.
The patch applies and builds cleanly, and passes make check. It does
what it says.
I found the following message somewhat confusing:
ERROR: read-only property must be set before any query
I was not setting read-only, but rather READ WRITE. This message is
understandable from the perspective of the code (and the "SET
transaction_read_only=..." command), but I think it should be framed
in the context of the SET TRANSACTION command in which read-only is
not the name of a boolean, but one label of a binary switch. Maybe
something like:
ERROR: transaction modes READ WRITE or READ ONLY must be set before any query.
It seems a bit strange that you can do dummy changes (setting the mode
to the same value it currently has) as much as you want in a
subtransaction, but not in a top-level transaction. But this was
discussed previously and not objected to.
The old behavior was not described in the docs. This patch does not
include a doc change, but considering the parallelism between this and
ISOLATION LEVEL, perhaps a parallel sentence should be added to the
docs about this aspect as well.
There are probably many people around who are abusing the current
laxity, so a mention in the release notes is probably warranted.
When a subtransaction has set the mode more stringent than the
top-level transaction did, that setting is reversed when the
subtransaction ends (whether by success or by rollback), which was
discussed as the desired behavior. But the included regression tests
do not exercise that case by testing the case where a SAVEPOINT is
either rolled back or released. Should those tests be included?
The changes made to the isolation level code to get rid of some
spurious warnings are not tested in the regression test--if I excise
that part of the patch, the code still passes make check. Just
looking at that code, it appears to do what it is supposed to, but I
can't figure out how to test it myself.
I poked at the patched code a bit and I could not break it, but I
don't know enough about this part of the system to design truly
devilish tests to apply to it.
I did not do any performance testing, as I don't see how this patch
could have performance implications.
None of the issues I raise above are severe. Does that mean I should
change the status to "ready for committer"?
Cheers,
Jeff
On Sun, Jan 16, 2011 at 6:58 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
None of the issues I raise above are severe. Does that mean I should
change the status to "ready for committer"?
Sounds right to me.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Jeff Janes wrote:
A review:
Thanks! Very thorough!
None of the issues I raise above are severe. Does that mean I
should change the status to "ready for committer"?
I see that notion was endorsed by Robert, so I'll leave it alone for
now. If a committer asks me to do something about any of those
issues (or others, of course), I'll happily do so.
-Kevin
Import Notes
Resolved by subject fallback
On Mon, Jan 10, 2011 at 11:27 AM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
Attached is a rebased roll-up of the 3 and 3a patches from last month.
Sorry to be a dweeb, but do you have a link to previous discussion?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Sun, Jan 16, 2011 at 6:58 PM, Jeff Janes <jeff.janes@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...".
When a subtransaction has set the mode more stringent than the
top-level transaction did, that setting is reversed when the
subtransaction ends (whether by success or by rollback), which was
discussed as the desired behavior. But the included regression tests
do not exercise that case by testing the case where a SAVEPOINT is
either rolled back or released. Should those tests be included?
+1.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas wrote:
Kevin Grittner wrote:
Attached is a rebased roll-up of the 3 and 3a patches from last
month.
do you have a link to previous discussion?
http://archives.postgresql.org/pgsql-hackers/2010-12/msg00582.php
That thread seems to break, but if you look at the references and
follow-up here, you've got the important points, I think:
http://archives.postgresql.org/message-id/4CFFB70302000025000384AF@gw.wicourts.gov
-Kevin
Import Notes
Resolved by subject fallback
Robert Haas wrote:
Jeff Janes wrote:I found the following message somewhat confusing:
ERROR: read-only property must be set before any queryI think what we need here is two messages, this one and a similar one
that starts with "read-write property...".When a subtransaction has set the mode more stringent than the
top-level transaction did, that setting is reversed when the
subtransaction ends (whether by success or by rollback), which was
discussed as the desired behavior. But the included regression tests
do not exercise that case by testing the case where a SAVEPOINT is
either rolled back or released. Should those tests be included?+1.
OK, will put something together.
-Kevin
Import Notes
Resolved by subject fallback
Robert Haas <robertmhaas@gmail.com> wrote:
Jeff Janes <jeff.janes@gmail.com> wrote:
I found the following message somewhat confusing:
ERROR: read-only property must be set before any queryI 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.
Also, I noticed we seemed to be using "transaction read-only mode"
and "transaction read-write mode" elsewhere, so I made this
consistent with the others while I was at it. Hopefully that was a
good idea.
When a subtransaction has set the mode more stringent than the
top-level transaction did, that setting is reversed when the
subtransaction ends (whether by success or by rollback), which
was discussed as the desired behavior. But the included
regression tests do not exercise that case by testing the case
where a SAVEPOINT is either rolled back or released. Should
those tests be included?+1.
Done.
-Kevin
Attachments:
read-only-5.patchtext/plain; name=read-only-5.patchDownload
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
***************
*** 544,572 **** show_log_timezone(void)
/*
* SET TRANSACTION ISOLATION LEVEL
*/
const char *
assign_XactIsoLevel(const char *value, bool doit, GucSource source)
{
! if (FirstSnapshotSet)
{
! ereport(GUC_complaint_elevel(source),
! (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
! errmsg("SET TRANSACTION ISOLATION LEVEL must be called before any query")));
! /* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
! if (source != PGC_S_OVERRIDE)
return NULL;
! }
! else if (IsSubTransaction())
! {
! ereport(GUC_complaint_elevel(source),
! (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
! errmsg("SET TRANSACTION ISOLATION LEVEL must not be called in a subtransaction")));
! /* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
! if (source != PGC_S_OVERRIDE)
return NULL;
}
if (strcmp(value, "serializable") == 0)
--- 544,617 ----
/*
+ * 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
*/
+ extern char *XactIsoLevel_string; /* in guc.c */
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 (FirstSnapshotSet)
! {
! ereport(GUC_complaint_elevel(source),
! (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
! errmsg("SET TRANSACTION ISOLATION LEVEL must be called before any query")));
return NULL;
! }
! /* We ignore a subtransaction setting it to the existing value. */
! if (IsSubTransaction() && strcmp(value, XactIsoLevel_string) != 0)
! {
! ereport(GUC_complaint_elevel(source),
! (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
! errmsg("SET TRANSACTION ISOLATION LEVEL must not be called in a subtransaction")));
return NULL;
+ }
}
if (strcmp(value, "serializable") == 0)
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 168,174 **** 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);
--- 168,173 ----
***************
*** 425,431 **** static int server_version_num;
static char *timezone_string;
static char *log_timezone_string;
static char *timezone_abbreviations_string;
- static char *XactIsoLevel_string;
static char *custom_variable_classes;
static int max_function_args;
static int max_index_keys;
--- 424,429 ----
***************
*** 440,445 **** static int effective_io_concurrency;
--- 438,444 ----
/* should be static, but commands/variable.c needs to get at these */
char *role_string;
char *session_authorization_string;
+ char *XactIsoLevel_string;
/*
***************
*** 7843,7876 **** 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)
{
--- 7842,7847 ----
*** a/src/include/commands/variable.h
--- b/src/include/commands/variable.h
***************
*** 21,26 **** extern const char *show_timezone(void);
--- 21,28 ----
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);
*** a/src/test/regress/expected/transactions.out
--- b/src/test/regress/expected/transactions.out
***************
*** 43,48 **** SELECT * FROM aggtest;
--- 43,123 ----
-- 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
*** a/src/test/regress/sql/transactions.sql
--- b/src/test/regress/sql/transactions.sql
***************
*** 39,44 **** SELECT * FROM aggtest;
--- 39,86 ----
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
On Fri, Jan 21, 2011 at 7:08 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
Robert Haas <robertmhaas@gmail.com> wrote:
Jeff Janes <jeff.janes@gmail.com> wrote:
I found the following message somewhat confusing:
ERROR: read-only property must be set before any queryI 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
Attachments:
read-only-5a.patchtext/x-diff; charset=US-ASCII; name=read-only-5a.patchDownload
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
Robert Haas wrote:
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.
I don't care a lot about that either -- if I remember correctly, we
got here based largely on my somewhat tentative interpretation of the
standard. Even if my reading was right (of which I'm far from sure),
it would just mean that we have an extension to the standard in
allowing the benign declarations. I'm sure not going to lose any
sleep over that.
I'll do whatever people want in this regard with no reservations.
-Kevin
Import Notes
Resolved by subject fallback
On Fri, Jan 21, 2011 at 10:21 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
Robert Haas wrote:
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.I don't care a lot about that either -- if I remember correctly, we
got here based largely on my somewhat tentative interpretation of the
standard. Even if my reading was right (of which I'm far from sure),
it would just mean that we have an extension to the standard in
allowing the benign declarations. I'm sure not going to lose any
sleep over that.I'll do whatever people want in this regard with no reservations.
OK, I've committed this as proposed above.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> wrote:
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.
I just looked at the committed code, and saw that it not only
changed things in this regard, but also allows a change from READ
WRITE to READ ONLY at any point in a transaction. (I see now that
your pseudo-code did the same, but I didn't pick up on it at the
time.)
That part of it seems a little weird to me. I think I can live with
it since it doesn't open up any routes to break SSI (now that I
reviewed our use of XactReadOnly and tweaked a function), or to
subvert security except for the unlikely scenario that something is
checking RO state and depending on there having been no writes
earlier in the transaction -- in which case they'd still need to be
very careful about subtransactions.
In short, I'm OK with it but wanted to make sure the community was
aware of the change to what this patch was doing, because I don't
think the discussion made that entirely clear.
-Kevin
On Mon, Jan 24, 2011 at 2:21 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
Robert Haas <robertmhaas@gmail.com> wrote:
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.I just looked at the committed code, and saw that it not only
changed things in this regard, but also allows a change from READ
WRITE to READ ONLY at any point in a transaction. (I see now that
your pseudo-code did the same, but I didn't pick up on it at the
time.)That part of it seems a little weird to me. I think I can live with
it since it doesn't open up any routes to break SSI (now that I
reviewed our use of XactReadOnly and tweaked a function), or to
subvert security except for the unlikely scenario that something is
checking RO state and depending on there having been no writes
earlier in the transaction -- in which case they'd still need to be
very careful about subtransactions.In short, I'm OK with it but wanted to make sure the community was
aware of the change to what this patch was doing, because I don't
think the discussion made that entirely clear.
Hmm, sorry, I thought that had been made clear. I guess the issue is
that within a subtransaction we can't really prohibit that anyway, so
spending extra code to do it in a toplevel transaction seems like
making the code more complicated just for the heck of it. I wasn't
intending to do anything not agreed...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company