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

Search Discussions

  • Robert Haas at Sep 1, 2010 at 11:26 pm

    On Sep 1, 2010, at 11:02 AM, "Kevin Grittner" 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
  • Kevin Grittner at Sep 2, 2010 at 3:41 pm

    Robert Haas 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
  • Robert Haas at Sep 2, 2010 at 5:55 pm

    On Thu, Sep 2, 2010 at 11:41 AM, Kevin Grittner 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
  • Kevin Grittner at Sep 2, 2010 at 6:23 pm

    Robert Haas 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
  • Tom Lane at Sep 2, 2010 at 7:07 pm

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
    Robert Haas 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
  • Kevin Grittner at Sep 2, 2010 at 7:14 pm

    Tom Lane 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
  • Kevin Grittner at Sep 3, 2010 at 11:06 pm

    Tom Lane 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
  • Alvaro Herrera at Sep 8, 2010 at 4:05 pm

    Excerpts from Kevin Grittner's message of vie sep 03 19:06:17 -0400 2010:
    Tom Lane 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
  • Tom Lane at Sep 8, 2010 at 4:12 pm

    Alvaro Herrera 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
  • Alvaro Herrera at Sep 8, 2010 at 4:41 pm

    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 at Sep 8, 2010 at 5:02 pm

    Alvaro Herrera 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
  • Joe Conway at Sep 9, 2010 at 4:11 pm

    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
  • Joe Conway at Sep 11, 2010 at 6:41 pm

    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
  • Kevin Grittner at Sep 13, 2010 at 8:59 pm

    Joe Conway 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
  • Bruce Momjian at Sep 8, 2010 at 4:51 pm

    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. +

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedSep 1, '10 at 3:03p
activeSep 13, '10 at 8:59p
posts16
users6
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase