Re: [HACKERS] READ ONLY fixes
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] READ ONLY fixes
On Mon, Jan 24, 2011 at 2:21 PM, Kevin Grittner kevin.gritt...@wicourts.gov wrote: Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] READ ONLY fixes
On Fri, Jan 21, 2011 at 10:21 PM, Kevin Grittner kevin.gritt...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] READ ONLY fixes
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.
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
*** 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),
!
Re: [HACKERS] READ ONLY fixes
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 601..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
Re: [HACKERS] READ ONLY fixes
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] READ ONLY fixes
On Mon, Jan 10, 2011 at 11:27 AM, Kevin Grittner kevin.gritt...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] READ ONLY fixes
On Sun, Jan 16, 2011 at 6:58 PM, 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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] READ ONLY fixes
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/4cffb703022500038...@gw.wicourts.gov -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] READ ONLY fixes
Robert Haas wrote: Jeff Janes 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. OK, will put something together. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] READ ONLY fixes
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] READ ONLY fixes
On Mon, Jan 10, 2011 at 8:27 AM, Kevin Grittner kevin.gritt...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] READ ONLY fixes
On Sun, Jan 16, 2011 at 6:58 PM, Jeff Janes jeff.ja...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] READ ONLY fixes
Attached is a rebased roll-up of the 3 and 3a patches from last month.
-Kevin
--- 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;
