Обсуждение: READ ONLY fixes
Attached is a rebased roll-up of the 3 and 3a patches from last month. -Kevin
Вложения
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
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
> 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
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 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
Вложения
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 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
Вложения
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
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