Обсуждение: "serializable" in comments and names
There are many comments in the code which use "serializable transaction" to mean "transaction snapshot based transaction". This doesn't matter much as long as REPEATABLE READ and SERIALIZABLE transaction isolation levels behave identically, but will be confusing and inaccurate when there is any difference between the two. In a similar way, the static bool registered_serializable in snapmgr.c will become misleading, and the IsXactIsoLevelSerializable macro is bound to lead to confusion. The patch to implement true serializable transactions using SSI renames/rewords these things to avoid confusion. However, there are seven files which are changed only for this reason, and another where there is one "real" change of two lines hidden in the midst of dozens of lines of such wording changes. I find it distracting to have all this mixed in, and I fear that it will be a time-waster for anyone reviewing the meat of the patch. I'd like to submit a "no-op" patch to cover these issues in advance of the CF, to get them out of the way. Joe suggested that I pose the issue to the -hackers list. I could knock out a couple other files from the main patch if people considered it acceptable to enable the SHMQueueIsDetached function now, which the patch uses in several places within asserts. I would remove the #ifdef NOT_USED from around the (very short) function, and add it to the .h file. The changes to the comments and local variables seem pretty safe. The change of IsXactIsoLevelSerializable to IsXactIsoLevelXactSnapshotBased (or whatever name the community prefers) could break the compile of external code; but the fix would be pretty obvious. It's hard to see how the change of a local variable name would present a lot of risk. So I see this as an extremely low-risk no-op patch to lay the groundwork for the main patch, so that it is easier to read. Thoughts? -Kevin
On Sep 1, 2010, at 11:02 AM, "Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote: > The patch to implement true serializable transactions using SSI > renames/rewords these things to avoid confusion. However, there are > seven files which are changed only for this reason, and another > where there is one "real" change of two lines hidden in the midst of > dozens of lines of such wording changes. I find it distracting to > have all this mixed in, and I fear that it will be a time-waster for > anyone reviewing the meat of the patch. I'd like to submit a > "no-op" patch to cover these issues in advance of the CF, to get > them out of the way. Joe suggested that I pose the issue to the > -hackers list. +1. > I could knock out a couple other files from the main patch if people > considered it acceptable to enable the SHMQueueIsDetached function > now, which the patch uses in several places within asserts. I would > remove the #ifdef NOT_USED from around the (very short) function, > and add it to the .h file. -1. > The changes to the comments and local variables seem pretty safe. > The change of IsXactIsoLevelSerializable to > IsXactIsoLevelXactSnapshotBased (or whatever name the community > prefers) How about IsXactIsoLevelSnapshot? Just to be a bit shorter. ...Robert
Robert Haas <robertmhaas@gmail.com> wrote: >> I could knock out a couple other files from the main patch if >> people considered it acceptable to enable the SHMQueueIsDetached >> function now, which the patch uses in several places within >> asserts. I would remove the #ifdef NOT_USED from around the >> (very short) function, and add it to the .h file. > > -1. OK, I'll leave that part out. >> The changes to the comments and local variables seem pretty >> safe. The change of IsXactIsoLevelSerializable to >> IsXactIsoLevelXactSnapshotBased (or whatever name the community >> prefers) > > How about IsXactIsoLevelSnapshot? Just to be a bit shorter. I need two macros -- one which has the same definition as the current IsXactIsoLevelSerializable, to be used everywhere the old macro name currently is used, which conveys that it is an isolation level which is based on a transaction snapshot rather than statement snapshots (i.e., REPEATABLE READ or SERIALIZABLE) and a new macro (which I was planning to call IsXactIsoLevelFullySerializable) which conveys that it is the SERIALIZABLE isolation level. Do you feel that IsXactIsoLevelSnapshot works with IsXactIsoLevelFullySerializable to convey the right semantics? If not, what would you suggest? I'm not attached to any particular names; what matters is that when people see them, they get the right meanings from them. I have some concern that IsXactIsoLevelSnapshot might suggest that it excludes the fully serializable transaction isolation level. -Kevin
On Thu, Sep 2, 2010 at 11:41 AM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: >> How about IsXactIsoLevelSnapshot? Just to be a bit shorter. > > I need two macros -- one which has the same definition as the > current IsXactIsoLevelSerializable, to be used everywhere the old > macro name currently is used, which conveys that it is an isolation > level which is based on a transaction snapshot rather than statement > snapshots (i.e., REPEATABLE READ or SERIALIZABLE) and a new macro > (which I was planning to call IsXactIsoLevelFullySerializable) which > conveys that it is the SERIALIZABLE isolation level. Do you feel > that IsXactIsoLevelSnapshot works with > IsXactIsoLevelFullySerializable to convey the right semantics? If > not, what would you suggest? OK, I see what you were going for. The current definition is: #define IsXactIsoLevelSerializable (XactIsoLevel >= XACT_REPEATABLE_READ) ...which is certainly a bit odd, since you'd think it would be comparing against XACT_SERIALIZABLE given the name. IsXactIsoLevelRepeatableRead()? XactUsesPerXactSnapshot()? Or, inverting the sense of it, XactUsesPerStatementSnapshot()? Just brainstorming... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> wrote: > The current definition is: > > #define IsXactIsoLevelSerializable (XactIsoLevel >= > XACT_REPEATABLE_READ) > > ...which is certainly a bit odd, since you'd think it would be > comparing against XACT_SERIALIZABLE given the name. Precisely why I want to rename it. ;-) > IsXactIsoLevelRepeatableRead()? Since the SSI implementation of a fully serializable transaction isolation level needs to do everything that the snapshot isolation of REPEATABLE READ does, plus a wee bit more, it is convenient to have a macro with the same semantics; just a less confusing name. I don't see anywhere in the code where there's a need to test for *just* REPEATABLE READ -- anything done for that also needs to be done for SERIALIZABLE. > XactUsesPerXactSnapshot()? That seems unambiguous. I think I prefer it to IsXactIsoLevelXactSnapshotBased, so if there are no objections, I'll switch to XactUsesPerXactSnapshot. The current code uses a macro without parentheses; are you suggesting that the new code add those? > Or, inverting the sense of it, XactUsesPerStatementSnapshot()? I don't see anywhere that the code is throwing an exclamation point in front of the macro name, so inverting it seems like a bad idea. I'd rather go from: if (IsXactIsoLevelSerializable) to: if (XactUsesPerXactSnapshot) than: if (!XactUsesPerStatementSnapshot) Given the suggested name above, IsXactIsoLevelFullySerializable no longer seems, well, symmetrical. How do you feel about XactIsFullySerializable? Names starting with IsXactIsoLevel seem more technically correct, but the names get long enough that it seems to me that the meaning gets a bit lost in the jumble of words -- which is why I like the shorter suggested name. Any other opinions out there? -Kevin
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: > Robert Haas <robertmhaas@gmail.com> wrote: >> XactUsesPerXactSnapshot()? > That seems unambiguous. I think I prefer it to > IsXactIsoLevelXactSnapshotBased, so if there are no objections, I'll > switch to XactUsesPerXactSnapshot. The current code uses a macro > without parentheses; are you suggesting that the new code add those? +1 for adding parens; we might want to make a function of it someday. > Names starting with IsXactIsoLevel seem more technically correct, > but the names get long enough that it seems to me that the meaning > gets a bit lost in the jumble of words -- which is why I like the > shorter suggested name. Any other opinions out there? I don't much like the "XactUses..." aspect of it; that's just about meaningless, because almost everything in PG could be said to be "used" by a transaction. How about IsolationUsesXactSnapshot (versus IsolationUsesStmtSnapshot)? regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote: > +1 for adding parens; we might want to make a function of it > someday. Makes sense; will do. > I don't much like the "XactUses..." aspect of it; that's just > about meaningless, because almost everything in PG could be said > to be "used" by a transaction. How about > IsolationUsesXactSnapshot (versus IsolationUsesStmtSnapshot)? And IsolationIsSerializable to make that test symmetrical? Any objections to this plan? -Kevin
Tom Lane <tgl@sss.pgh.pa.us> wrote: > +1 for adding parens; we might want to make a function of it > someday. > How about IsolationUsesXactSnapshot Patch attached. Joe said that he'll review this weekend and probably commit in a day or two if there are no objections. -Kevin
Вложения
Excerpts from Kevin Grittner's message of vie sep 03 19:06:17 -0400 2010: > Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > +1 for adding parens; we might want to make a function of it > > someday. > > > How about IsolationUsesXactSnapshot > > Patch attached. I find this name confusing :-( Doesn't a READ COMMITTED transaction use transaction snapshots as well? -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes: > Excerpts from Kevin Grittner's message of vie sep 03 19:06:17 -0400 2010: >>> How about IsolationUsesXactSnapshot > I find this name confusing :-( Doesn't a READ COMMITTED transaction use > transaction snapshots as well? AFAIR it doesn't keep the first snapshot around. If it did, most of your work on snapshot list trimming would have been useless, no? regards, tom lane
Excerpts from Tom Lane's message of mié sep 08 12:12:31 -0400 2010: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Excerpts from Kevin Grittner's message of vie sep 03 19:06:17 -0400 2010: > >>> How about IsolationUsesXactSnapshot > > > I find this name confusing :-( Doesn't a READ COMMITTED transaction use > > transaction snapshots as well? > > AFAIR it doesn't keep the first snapshot around. If it did, most of > your work on snapshot list trimming would have been useless, no? That's my point precisely. The name "IsolationUsesXactSnapshot" makes it sound like it applies to any transaction that uses snapshots for isolation, doesn't it? How about IsolationUses1stXactSnapshot, or something else that makes it clearer that there's a difference between that and read committed transactions? -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Excerpts from Kevin Grittner's message of vie sep 03 19:06:17 -0400 2010: > >>> How about IsolationUsesXactSnapshot > > > I find this name confusing :-( Doesn't a READ COMMITTED transaction use > > transaction snapshots as well? > > AFAIR it doesn't keep the first snapshot around. If it did, most of > your work on snapshot list trimming would have been useless, no? Technically, serializable uses a single transaction snapshot and read committed uses statement snapshots. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Alvaro Herrera <alvherre@commandprompt.com> writes: > Excerpts from Tom Lane's message of mié sep 08 12:12:31 -0400 2010: >> AFAIR it doesn't keep the first snapshot around. If it did, most of >> your work on snapshot list trimming would have been useless, no? > That's my point precisely. The name "IsolationUsesXactSnapshot" makes > it sound like it applies to any transaction that uses snapshots for > isolation, doesn't it? I don't think so, at least not when compared to the alternative IsolationUsesStmtSnapshot. > How about IsolationUses1stXactSnapshot This just seems longer, not really better. In particular, we have *always* adhered to the phraseology that a "transaction snapshot" is the first one taken in a transaction, so I don't see exactly why it's confusing you now. regards, tom lane
On 09/08/2010 10:02 AM, Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: >> Excerpts from Tom Lane's message of mié sep 08 12:12:31 -0400 2010: >>> AFAIR it doesn't keep the first snapshot around. If it did, most of >>> your work on snapshot list trimming would have been useless, no? > >> That's my point precisely. The name "IsolationUsesXactSnapshot" makes >> it sound like it applies to any transaction that uses snapshots for >> isolation, doesn't it? > > I don't think so, at least not when compared to the alternative > IsolationUsesStmtSnapshot. The attached patch is updated for the various comments, as well as some wording tweaks by me. If there are no objections I'd like to commit this in a day or two. Joe -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, & 24x7 Support
Вложения
On 09/09/2010 09:11 AM, Joe Conway wrote: > The attached patch is updated for the various comments, as well as some > wording tweaks by me. If there are no objections I'd like to commit this > in a day or two. Committed. Joe -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, & 24x7 Support
>Joe Conway <mail@joeconway.com> wrote: > Committed. Thanks! I'm pulling together a new version of the main patch, and it is almost 300 lines shorter and touches five fewer files than the last version because this went in. It should be easier for people to scan to understand the substance of the changes without the noop changes interleaved with "real" ones. -Kevin