Thread

1. SSI patch version 14

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-25T03:30:58Z

   Attached.
    
   Dan and I have spent many, many hours desk-check and testing,
   including pounding for many hours in DBT-2 at high concurrency
   factors on a 16 processor box.  In doing so, we found and fixed a few
   issues.  Neither of us is aware of any bugs or deficiencies
   remaining, even after a solid day of pounding in DBT-2, other than
   the failure to extend any new functionality to hot standby.
    
   At Heikki's suggestion I have included a test to throw an error on an
   attempt to switch to serializable mode during recovery.  Anything
   more to address that issue can be a small follow-up patch -- probably
   mainly a few notes in the docs.
    
   -Kevin
   
   
   

2. Re: SSI patch version 14

   Jeff Davis <pgsql@j-davis.com> — 2011-01-25T09:07:39Z

   On Mon, 2011-01-24 at 21:30 -0600, Kevin Grittner wrote: 
   > Attached.
   >  
   > Dan and I have spent many, many hours desk-check and testing,
   > including pounding for many hours in DBT-2 at high concurrency
   > factors on a 16 processor box.  In doing so, we found and fixed a few
   > issues.  Neither of us is aware of any bugs or deficiencies
   > remaining, even after a solid day of pounding in DBT-2, other than
   > the failure to extend any new functionality to hot standby.
   >  
   > At Heikki's suggestion I have included a test to throw an error on an
   > attempt to switch to serializable mode during recovery.  Anything
   > more to address that issue can be a small follow-up patch -- probably
   > mainly a few notes in the docs.
   
   Here is my first crack at a review:
   
   First of all, I am very impressed with the patch. I was pleased to see
   that both READ ONLY DEFERRABLE and 2PC work! Also, I found the patch
   very usable and did not encounter any bugs or big surprises.
   
   Second, I do not understand this patch entirely, so the following
   statements can be considered questions as much as answers.
   
   At a high level, there is a nice conceptual simplicity. Let me try to
   summarize it as I understand it:
     * RW dependencies are detected using predicate locking.
     * RW dependencies are tracked from the reading transaction (as an
       "out") conflict; and from the writing transaction (as an "in"
       conflict).
     * Before committing a transaction, then by looking only at the RW 
       dependencies (and predicate locks) for current and past 
       transactions, you can determine if committing this transaction will
       result in a cycle (and therefore a serialization anomaly); and if
       so, abort it.
   
   That's where the simplicity ends, however ;)
   
   For starters, the above structures require unlimited memory, while we
   have fixed amounts of memory. The predicate locks are referenced by a
   global shared hash table as well as per-process SHMQueues. It can adapt
   memory usage downward in three ways:
     * increase lock granularity -- e.g. change N page locks into a
       table lock
     * "summarization" -- fold multiple locks on the same object across
       many old committed transactions into a single lock
     * use the SLRU
   
   Tracking of RW conflicts of current and past transactions is more
   complex. Obviously, it doesn't keep _all_ past transactions, but only
   ones that overlap with a currently-running transaction. It does all of
   this management using SHMQueue. There isn't much of an attempt to
   gracefully handle OOM here as far as I can tell, it just throws an error
   if there's not enough room to track a new transaction (which is
   reasonable, considering that it should be quite rare and can be
   mitigated by increasing max_connections). 
   
   The heavy use of SHMQueue is quite reasonable, but for some reason I
   find the API very difficult to read. I think it would help (at least me)
   quite a lot to annotate (i.e. with a comment in the struct) the various
   lists and links with the types of data being held.
   
   The actual checking of conflicts isn't quite so simple, either, because
   we want to detect problems before the victim transaction has done too
   much work. So, checking is done along the way in several places, and
   it's a little difficult to follow exactly how those smaller checks add
   up to the overall serialization-anomaly check (the third point in my
   simple model).
   
   There are also optimizations around transactions declared READ ONLY.
   Some of these are a little difficult to follow as well, and I haven't
   followed them all.
   
   There is READ ONLY DEFERRABLE, which is a nice feature that waits for a
   "safe" snapshot, so that the transaction will never be aborted.
   
   Now, on to my code comments (non-exhaustive):
   
   * I see that you use a union as well as using "outLinks" to also be a
   free list. I suppose you did this to conserve shared memory space,
   right?
   
   * In RegisterSerializableTransactionInt(), for a RO xact, it considers
   any concurrent RW xact a possible conflict. It seems like it would be
   possible to know whether a RW transaction may have overlapped with any
   committed RW transaction (in finishedLink queue), and only add those as
   potential conflicts. Would that work? If so, that would make more
   snapshots safe.
   
   * When a transaction finishes, then PID should probably be set to zero.
   You only use it for waking up a deferrable RO xact waiting for a
   snapshot, right?
   
   * Still some compiler warnings:
   twophase.c: In function ‘FinishPreparedTransaction’:
   twophase.c:1360: warning: implicit declaration of function
   ‘PredicateLockTwoPhaseFinish’
   
   * You have a function called CreatePredTran. We are already using
   "Transaction" and "Xact" -- we probably don't need "Tran" as well.
   
   * HS error has a typo:
   "ERROR:  cannot set transaction isolationn level to serializable during
   recovery"
   
   * I'm a little unclear on summarization and writing to the SLRU. I don't
   see where it's determining that the predicate locks can be safely
   released. Couldn't the oldest transaction still have relevant predicate
   locks?
   
   * In RegisterSerializableTransactionInt, if it doesn't get an sxact, it
   goes into summarization. But summarization assumes that it has at least
   one finished xact. Is that always true? If you have enough memory to
   hold a transaction for each connection, plus max_prepared_xacts, plus
   one, I think that's true. But maybe that could be made more clear?
   
   I'll keep working on this patch. I hope I can be of some help getting
   this committed, because I'm looking forward to this feature. And I
   certainly think that you and Dan have applied the amount of planning,
   documentation, and careful implementation necessary for a feature like
   this.
   
   Regards,
   	Jeff Davis
   
   
   
   

3. Re: SSI patch version 14

   Dan Ports <drkp@csail.mit.edu> — 2011-01-25T10:57:42Z

   Thanks for working your way through this patch. I'm certainly well
   aware that that's not a trivial task!
   
   I'm suffering through a bout of insomnia, so I'll respond to some of
   your high-level comments in hopes that serializability will help put me
   to sleep (as it often does). I'll leave the more detailed code comments
   for later when I'm actually looking at the code, or better yet Kevin
   will take care of them and I won't have to. ;-)
   
   On Tue, Jan 25, 2011 at 01:07:39AM -0800, Jeff Davis wrote:
   > At a high level, there is a nice conceptual simplicity. Let me try to
   > summarize it as I understand it:
   >   * RW dependencies are detected using predicate locking.
   >   * RW dependencies are tracked from the reading transaction (as an
   >     "out") conflict; and from the writing transaction (as an "in"
   >     conflict).
   >   * Before committing a transaction, then by looking only at the RW 
   >     dependencies (and predicate locks) for current and past 
   >     transactions, you can determine if committing this transaction will
   >     result in a cycle (and therefore a serialization anomaly); and if
   >     so, abort it.
   
   This summary is right on. I would add one additional detail or
   clarification to the last point, which is that rather than checking for
   a cycle, we're checking for a transaction with both "in" and "out"
   conflicts, which every cycle must contain.
   
   > That's where the simplicity ends, however ;)
   
   Indeed!
   
   > Tracking of RW conflicts of current and past transactions is more
   > complex. Obviously, it doesn't keep _all_ past transactions, but only
   > ones that overlap with a currently-running transaction. It does all of
   > this management using SHMQueue. There isn't much of an attempt to
   > gracefully handle OOM here as far as I can tell, it just throws an error
   > if there's not enough room to track a new transaction (which is
   > reasonable, considering that it should be quite rare and can be
   > mitigated by increasing max_connections). 
   
   If the OOM condition you're referring to is the same one from the
   following comment, then it can't happen: (Apologies if I've
   misunderstood what you're referring to.)
   
   > * In RegisterSerializableTransactionInt, if it doesn't get an sxact, it
   > goes into summarization. But summarization assumes that it has at least
   > one finished xact. Is that always true? If you have enough memory to
   > hold a transaction for each connection, plus max_prepared_xacts, plus
   > one, I think that's true. But maybe that could be made more clear?
   
   Yes -- the SerializableXact pool is allocated up front and it
   definitely has to be bigger than the number of possible active
   transactions. In fact, it's much larger: 10 * (MaxBackends +
   max_prepared_xacts) to allow some room for the committed transactions
   we still have to track.
   
   > * In RegisterSerializableTransactionInt(), for a RO xact, it considers
   > any concurrent RW xact a possible conflict. It seems like it would be
   > possible to know whether a RW transaction may have overlapped with any
   > committed RW transaction (in finishedLink queue), and only add those as
   > potential conflicts. Would that work? If so, that would make more
   > snapshots safe.
   
   Interesting idea. That's worth some careful thought. I think it's
   related to the condition that the RW xact needs to commit with a
   conflict out to a transaction earlier than the RO xact. My first guess
   is that this wouldn't make more transactions safe, but could detect
   safe snapshots faster.
   
   > * When a transaction finishes, then PID should probably be set to zero.
   > You only use it for waking up a deferrable RO xact waiting for a
   > snapshot, right?
   
   Correct. It probably wouldn't hurt to clear that field when releasing
   the transaction, but we don't use it after.
   
   > * I'm a little unclear on summarization and writing to the SLRU. I don't
   > see where it's determining that the predicate locks can be safely
   > released. Couldn't the oldest transaction still have relevant predicate
   > locks?
   
   When a SerializableXact gets summarized to the SLRU, its predicate
   locks aren't released; they're transferred to the dummy
   OldCommittedSxact.
   
   > I'll keep working on this patch. I hope I can be of some help getting
   > this committed, because I'm looking forward to this feature. And I
   > certainly think that you and Dan have applied the amount of planning,
   > documentation, and careful implementation necessary for a feature like
   > this.
   
   Hopefully my comments here will help clarify the patch. It's not lost
   on me that there's no shortage of complexity in the patch, so if you
   found anything particularly confusing we should probably add some
   documentation to README-SSI.
   
   Dan
   
   -- 
   Dan R. K. Ports              MIT CSAIL                http://drkp.net/
   
   

4. Re: SSI patch version 14

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-25T17:17:06Z

   Jeff Davis <pgsql@j-davis.com> wrote:
    
   > For starters, the above structures require unlimited memory, while
   > we have fixed amounts of memory. The predicate locks are
   > referenced by a global shared hash table as well as per-process
   > SHMQueues. It can adapt memory usage downward in three ways:
   >   * increase lock granularity -- e.g. change N page locks into a
   >     table lock
   >   * "summarization" -- fold multiple locks on the same object
   >     across many old committed transactions into a single lock
   >   * use the SLRU
    
   Those last two are related -- the summarization process takes the
   oldest committed-but-still-significant transaction and does two
   things with it:
    
   (1)  We move predicate locks to the "dummy" transaction, kept just
   for this purpose.  We combine locks against the same lock target,
   using the more recent commit point to determine when the resulting
   lock can be removed.
    
   (2)  We use SLRU to keep track of the top level xid of the old
   committed transaction, and the earliest commit point of any
   transactions to which it had an out-conflict.
    
   The above reduces the information available to avoid serialization
   failure in certain corner cases, and is more expensive to access
   than the other structures, but it keeps us running in pessimal
   circumstances, even if at a degraded level of performance.
    
   > The heavy use of SHMQueue is quite reasonable, but for some reason
   > I find the API very difficult to read. I think it would help (at
   > least me) quite a lot to annotate (i.e. with a comment in the
   > struct) the various lists and links with the types of data being
   > held.
    
   We've tried to comment enough, but when you have your head buried in
   code you don't always recognize how mysterious something can look. 
   Can you suggest some particular places where more comments would be
   helpful?
    
   > The actual checking of conflicts isn't quite so simple, either,
   > because we want to detect problems before the victim transaction
   > has done too much work. So, checking is done along the way in
   > several places, and it's a little difficult to follow exactly how
   > those smaller checks add up to the overall serialization-anomaly
   > check (the third point in my simple model).
   > 
   > There are also optimizations around transactions declared READ
   > ONLY. Some of these are a little difficult to follow as well, and
   > I haven't followed them all.
    
   Yeah.  After things were basically working correctly, in terms of
   not allowing any anomalies, we still had a lot of false positives. 
   Checks around the order and timing of commits, as well as read-only
   status, helped bring these down.  The infamous receipting example
   was my main guide here.  There are 210 permutations in the way the
   statements can be interleaved, of which only six can result in
   anomalies.  At first we only managed to commit a couple dozen
   permutations.  As we added code to cover optimizations for various
   special cases the false positive rate dropped a little at a time,
   until that test hit 204 commits, six rollbacks.  Although, all the
   tests in the dcheck target are useful -- if I made a mistake in
   implementing an optimization there would sometimes be just one or
   two of the other tests which would fail.  Looking at which
   permutations got it right and which didn't was a really good way to
   figure out what I did wrong.
    
   > There is READ ONLY DEFERRABLE, which is a nice feature that waits
   > for a "safe" snapshot, so that the transaction will never be
   > aborted.
    
   *And* will not contribute to causing any other transaction to be
   rolled back, *and* dodges the overhead of predicate locking and
   conflict checking.  Glad you like it!  ;-)
    
   > Now, on to my code comments (non-exhaustive):
   > 
   > * I see that you use a union as well as using "outLinks" to also
   > be a free list. I suppose you did this to conserve shared memory
   > space, right?
    
   Yeah, we tried to conserve shared memory space where we could do so
   without hurting performance.  Some of it gets to be a little bit-
   twiddly, but it seemed like a good idea at the time.  Does any of it
   seem like it creates a confusion factor which isn't worth it
   compared to the shared memory savings?
    
   > * Still some compiler warnings:
   > twophase.c: In function *FinishPreparedTransaction*:
   > twophase.c:1360: warning: implicit declaration of function
   > *PredicateLockTwoPhaseFinish*
    
   Ouch!  That could cause bugs, since the implicit declaration didn't
   actually *match* the actual definition.  Don't know how we missed
   that.  I strongly recommend that anyone who wants to test 2PC with
   the patch add this commit to it:
    
   http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=8e020d97bc7b8c72731688515b6d895f7e243e27
    
   > * You have a function called CreatePredTran. We are already using
   > "Transaction" and "Xact" -- we probably don't need "Tran" as well.
    
   OK.  Will rename if you like.  Since that creates the PredTran
   structure, I assume you want that renamed, too?
    
   > * HS error has a typo:
   > "ERROR:  cannot set transaction isolationn level to serializable
   > during recovery"
    
   Will fix.
    
   > I'll keep working on this patch. I hope I can be of some help
   > getting this committed, because I'm looking forward to this
   > feature. And I certainly think that you and Dan have applied the
   > amount of planning, documentation, and careful implementation
   > necessary for a feature like this.
    
   Thanks much!  This effort was driven, for my part, by the needs of
   my employer; but I have to admit it was kinda fun to do some serious
   coding on innovative ideas again.  It's been a while.  I'm ready to
   kick back and party a bit when this gets done, though.  ;-)
    
   -Kevin
   
   

5. Re: SSI patch version 14

   Jeff Davis <pgsql@j-davis.com> — 2011-01-25T18:37:32Z

   On Tue, 2011-01-25 at 11:17 -0600, Kevin Grittner wrote:
   > > The heavy use of SHMQueue is quite reasonable, but for some reason
   > > I find the API very difficult to read. I think it would help (at
   > > least me) quite a lot to annotate (i.e. with a comment in the
   > > struct) the various lists and links with the types of data being
   > > held.
   >  
   > We've tried to comment enough, but when you have your head buried in
   > code you don't always recognize how mysterious something can look. 
   > Can you suggest some particular places where more comments would be
   > helpful?
   
   I think just annotating RWConflict.in/outLink and
   PredTranList.available/activeList with the types of things they hold
   would be a help.
   
   Also, you say something about RWConflict and "when the structure is not
   in use". Can you elaborate on that a bit?
   
   I'll address the rest of your comments in a later email.
   
   Regards,
   	Jeff Davis
   
   
   

6. Re: SSI patch version 14

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-01-25T20:41:18Z

   On 25.01.2011 05:30, Kevin Grittner wrote:
   > Attached.
   
   The readme says this:
   > +   4. PostgreSQL supports subtransactions -- an issue not mentioned
   > +in the papers.
   
   But I don't see any mention anywhere else on how subtransactions are 
   handled. If a subtransaction aborts, are its predicate locks immediately 
   released?
   
   -- 
      Heikki Linnakangas
      EnterpriseDB   http://www.enterprisedb.com
   
   

7. Re: SSI patch version 14

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-25T20:53:02Z

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:
   > On 25.01.2011 05:30, Kevin Grittner wrote:
    
   > The readme says this:
   >> 4. PostgreSQL supports subtransactions -- an issue not mentioned
   >>    in the papers.
   > 
   > But I don't see any mention anywhere else on how subtransactions
   > are handled. If a subtransaction aborts, are its predicate locks
   > immediately released?
    
   No.  Here's the reasoning.  Within a top level transaction, you
   might start a subtransaction, read some data, and then decide based
   on what you read that the subtransaction should be rolled back.  If
   the decision as to what is part of the top level transaction can
   depend on what is read in the subtransaction, predicate locks taken
   by the subtransaction must survive rollback of the subtransaction.
    
   Does that make sense to you?  Is there somewhere you would like to
   see that argument documented?
    
   -Kevin
   
   

8. Re: SSI patch version 14

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-25T21:22:59Z

   Jeff Davis <pgsql@j-davis.com> wrote:
    
   > I think just annotating RWConflict.in/outLink and
   > PredTranList.available/activeList with the types of things they
   > hold would be a help.
   > 
   > Also, you say something about RWConflict and "when the structure
   > is not in use". Can you elaborate on that a bit?
    
   Please let me know whether this works for you:
    
   http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=325ec55e8c9e5179e2e16ff303af6afc1d6e732b
    
   -Kevin
   
   

9. Re: SSI patch version 14

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-01-26T07:20:39Z

   On 25.01.2011 22:53, Kevin Grittner wrote:
   > Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  wrote:
   >> On 25.01.2011 05:30, Kevin Grittner wrote:
   >
   >> The readme says this:
   >>> 4. PostgreSQL supports subtransactions -- an issue not mentioned
   >>>     in the papers.
   >>
   >> But I don't see any mention anywhere else on how subtransactions
   >> are handled. If a subtransaction aborts, are its predicate locks
   >> immediately released?
   >
   > No.  Here's the reasoning.  Within a top level transaction, you
   > might start a subtransaction, read some data, and then decide based
   > on what you read that the subtransaction should be rolled back.  If
   > the decision as to what is part of the top level transaction can
   > depend on what is read in the subtransaction, predicate locks taken
   > by the subtransaction must survive rollback of the subtransaction.
   >
   > Does that make sense to you?
   
   Yes, that's what I suspected. And I gather that all the data structures 
   in predicate.c work with top-level xids, not subxids. When looking at an 
   xid that comes from a tuple's xmin or xmax, for example, you always call 
   SubTransGetTopmostTransaction() before doing much else with it.
   
   >  Is there somewhere you would like to
   > see that argument documented?
   
   README-SSI .
   
   -- 
      Heikki Linnakangas
      EnterpriseDB   http://www.enterprisedb.com
   
   

10. Re: SSI patch version 14

    Simon Riggs <simon@2ndquadrant.com> — 2011-01-26T11:36:15Z

    On Mon, 2011-01-24 at 21:30 -0600, Kevin Grittner wrote:
    
    > Dan and I have spent many, many hours desk-check and testing,
    > including pounding for many hours in DBT-2 at high concurrency
    > factors on a 16 processor box. In doing so, we found and fixed a few
    > issues. Neither of us is aware of any bugs or deficiencies
    > remaining, even after a solid day of pounding in DBT-2, other than
    > the failure to extend any new functionality to hot standby.
    
    A couple of comments on this.
    
    I looked at the patch to begin a review and immediately saw "dtest".
    There's no docs to explain what it is, but a few comments fill me in a
    little more. Can we document that please? And/or explain why its an
    essential part of this commit? Could we keep it out of core, or if not,
    just commit that part separately? I notice the code is currently
    copyright someone else than PGDG.
    
    Pounding for hours on 16 CPU box sounds good. What diagnostics or
    instrumentation are included with the patch? How will we know whether
    pounding for hours is actually touching all relevant parts of code? I've
    done such things myself only to later realise I wasn't actually testing
    the right piece of code.
    
    When this runs in production, how will we know whether SSI is stuck or
    is consuming too much memory? e.g. Is there a dynamic view e.g.
    pg_prepared_xacts, or is there a log mode log_ssi_impact, etc??
    
    -- 
     Simon Riggs           http://www.2ndQuadrant.com/books/
     PostgreSQL Development, 24x7 Support, Training and Services
     
    
    
    

11. Re: SSI patch version 14

    Simon Riggs <simon@2ndquadrant.com> — 2011-01-26T13:01:22Z

    On Wed, 2011-01-26 at 11:36 +0000, Simon Riggs wrote:
    
    > Pounding for hours on 16 CPU box sounds good. What diagnostics or
    > instrumentation are included with the patch? How will we know whether
    > pounding for hours is actually touching all relevant parts of code? I've
    > done such things myself only to later realise I wasn't actually testing
    > the right piece of code.
    
    An example of this is the XIDCACHE_DEBUG code used in procarray.c to
    validate TransactionIdIsInProgress().
    
    -- 
     Simon Riggs           http://www.2ndQuadrant.com/books/
     PostgreSQL Development, 24x7 Support, Training and Services
     
    
    
    

12. Re: SSI patch version 14

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-26T15:22:12Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:
    > On 25.01.2011 22:53, Kevin Grittner wrote:
    >>  Is there somewhere you would like to
    >> see that argument documented?
    > 
    > README-SSI .
     
    Done (borrowing some of your language).
     
    http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=470abc51c5626cf3c7cbd734b1944342973d0d47
     
    Let me know if you think more is needed.
     
    -Kevin
    
    

13. Re: SSI patch version 14

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-26T16:01:28Z

    Simon Riggs <simon@2ndQuadrant.com> wrote:
    > On Wed, 2011-01-26 at 11:36 +0000, Simon Riggs wrote:
    > 
    >> Pounding for hours on 16 CPU box sounds good. What diagnostics or
    >> instrumentation are included with the patch? How will we know
    >> whether pounding for hours is actually touching all relevant
    >> parts of code? I've done such things myself only to later realise
    >> I wasn't actually testing the right piece of code.
    > 
    > An example of this is the XIDCACHE_DEBUG code used in procarray.c
    > to validate TransactionIdIsInProgress().
     
    It isn't exactly equivalent, but on a conceptually similar note some
    of the hours of DBT-2 pounding were done with #ifdef statements to
    force code into code paths which are normally rarely used.  We left
    one of them in the codebase with the #define commented out, although
    I know that's not strictly necessary.  (It does provide a convenient
    place to put a comment about what it's for, though.)
     
    In looking at it just now, I noticed that after trying it in a
    couple different places what was left in the repository was not the
    optimal version for code coverage.  I've put this back to the
    version which did a better job, for reasons described in the commit
    comment:
     
    http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=8af1bc84318923ba0ec3d4413f374a3beb10bc70
     
    Dan, did you have some others which should maybe be included?
     
    I'm not sure I see any counts we could get from SSI which would be
    useful beyond what we might get from a code coverage tool or
    profiling, but I'm open to suggestions.
     
    -Kevin
    
    

14. Re: SSI patch version 14

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-26T17:07:18Z

    Simon Riggs <simon@2ndQuadrant.com> wrote:
     
    > I looked at the patch to begin a review and immediately saw
    > "dtest". There's no docs to explain what it is, but a few comments
    > fill me in a little more. Can we document that please? And/or
    > explain why its an essential part of this commit? Could we keep it
    > out of core, or if not, just commit that part separately? I notice
    > the code is currently copyright someone else than PGDG.
     
    I'm including Markus on this reply, because he's the only one who
    can address the copyright issue.
     
    I will say that while the dcheck make target is not required for a
    proper build, and the tests run for too long to consider including
    this in the check target, I would not recommend that anybody hack on
    SSI without regularly running these tests or some equivalent..
     
    When I suggested breaking this out of the patch, everybody who spoke
    up said not to do so.  How the eventual committer commits it is of
    course up to that person.
     
    > Pounding for hours on 16 CPU box sounds good. What diagnostics or
    > instrumentation are included with the patch? How will we know
    > whether pounding for hours is actually touching all relevant parts
    > of code? I've done such things myself only to later realise I
    > wasn't actually testing the right piece of code.
     
    We've looked at distributions of failed transactions by ereport
    statement.  This confirms that we are indeed exercising the vast
    majority of the code.  See separate post for how we pushed execution
    into the summarization path to test code related to that.
     
    I do have some concern that the 2PC testing hasn't yet covered all
    code paths.  I don't see how the problem found by Jeff during review
    could have survived thorough testing there.
     
    > When this runs in production, how will we know whether SSI is
    > stuck
     
    Stuck?  I'm not sure what you mean by that.  Other than LW locking
    (which I believe is always appropriately brief, with rules for order
    of acquisition which should prevent deadlocks), the only blocking
    introduced by SSI is when there is an explicit request for
    DEFERRABLE READ ONLY transactions.  Such a transaction may take a
    while to start.  Is that what you're talking about?
     
    > or is consuming too much memory?
     
    Relevant shared memory is allocated at startup, with strategies for
    living within that as suggested by Heikki and summarized in a recent
    post by Jeff.  It's theoretically possible to run out of certain
    objects, in which case there is an ereport, but we haven't seen
    anything like that since the mitigation and graceful degradation
    changes were implemented.
     
    > e.g. Is there a dynamic view e.g. pg_prepared_xacts,
     
    We show predicate locks in the pg_locks view with mode SIReadLock.
     
    > is there a log mode log_ssi_impact, etc??
     
    Don't have that.  What would you expect that to show?
     
    -Kevin
    
    

15. Re: SSI patch version 14

    Markus Wanner <markus@bluegap.ch> — 2011-01-26T18:52:27Z

    Kevin,
    
    thanks for your heads-up.
    
    On 01/26/2011 06:07 PM, Kevin Grittner wrote:
    > Simon Riggs <simon@2ndQuadrant.com> wrote:
    >  
    >> I looked at the patch to begin a review and immediately saw
    >> "dtest". There's no docs to explain what it is, but a few comments
    >> fill me in a little more. Can we document that please? And/or
    >> explain why its an essential part of this commit? Could we keep it
    >> out of core, or if not, just commit that part separately? I notice
    >> the code is currently copyright someone else than PGDG.
    >  
    > I'm including Markus on this reply, because he's the only one who
    > can address the copyright issue.
    
    I certainly agree to change the copyright notice for my parts of the
    code in Kevin's SSI to say "Copyright ... Postgres Global Development
    Group".
    
    However, it's also worth mentioning that 'make dcheck' requires my
    dtester python package to be installed.  See [1].  I put that under the
    Boost license, which seems very similar to the Postgres license.
    
    > When I suggested breaking this out of the patch, everybody who spoke
    > up said not to do so.  How the eventual committer commits it is of
    > course up to that person.
    
    If the committer decides not to commit the dtests, I'm happy to add
    these dtests to the "official" postgres-dtests repository [2].  There I
    could let it follow the development of dtester.
    
    If Kevin's dtests gets committed, either dtester needs to be backwards
    compatible or the Postgres sources need to follow development of
    dtester, which I'm not satisfied with, yet.  (However, note that I
    didn't have any time to work on dtester, recently, so that is somewhat
    hypothetical anyway).
    
    If you have any more questions, I'm happy to help.
    
    Regards
    
    Markus Wanner
    
    
    [1]: dtester project site:
    http://www.bluegap.ch/projects/dtester/
    
    [2]: postgres dtests:
    http://git.postgres-r.org/?p=postgres-dtests;a=summary
    
    

16. Re: SSI patch version 14

    Simon Riggs <simon@2ndquadrant.com> — 2011-01-26T19:08:35Z

    On Wed, 2011-01-26 at 11:07 -0600, Kevin Grittner wrote:
    
    > > When this runs in production, how will we know whether SSI is
    > > stuck
    >  
    > Stuck?  I'm not sure what you mean by that.  Other than LW locking
    > (which I believe is always appropriately brief, with rules for order
    > of acquisition which should prevent deadlocks), the only blocking
    > introduced by SSI is when there is an explicit request for
    > DEFERRABLE READ ONLY transactions.  Such a transaction may take a
    > while to start.  Is that what you're talking about?
    
    I'm thinking of a general requirement for diagnostics.
    
    What has been done so far looks great to me, so talking about this
    subject is in no way meant to be negative. Everything has bugs and we
    find them quicker if there are some ways of getting out more information
    about what is happening in the guts of the system.
    
    For HS, I put in a special debug mode and various information functions.
    For HOT, I produced a set of page inspection functions.
     
    I'm keen to have some ways where we can find out various metrics about
    what is happening, so we can report back to you to check if there are
    bugs. I foresee that some applications will be more likely to generate
    serialization errors than others. People will ask questions and they may
    claim there are bugs. I would like to be able to say "there is no bug -
    look at XYZ and see that the errors you are getting are because of ABC".
    
    > > or is consuming too much memory?
    >  
    > Relevant shared memory is allocated at startup, with strategies for
    > living within that as suggested by Heikki and summarized in a recent
    > post by Jeff.  It's theoretically possible to run out of certain
    > objects, in which case there is an ereport, but we haven't seen
    > anything like that since the mitigation and graceful degradation
    > changes were implemented.
    >  
    > > e.g. Is there a dynamic view e.g. pg_prepared_xacts,
    >  
    > We show predicate locks in the pg_locks view with mode SIReadLock.
    
    OK, that's good.
    
    > > is there a log mode log_ssi_impact, etc??
    >  
    > Don't have that.  What would you expect that to show?
    >  
    > -Kevin
    
    -- 
     Simon Riggs           http://www.2ndQuadrant.com/books/
     PostgreSQL Development, 24x7 Support, Training and Services
     
    
    
    

17. Re: SSI patch version 14

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-01-26T19:25:27Z

    Excerpts from Kevin Grittner's message of mié ene 26 14:07:18 -0300 2011:
    > Simon Riggs <simon@2ndQuadrant.com> wrote:
    
    > > Pounding for hours on 16 CPU box sounds good. What diagnostics or
    > > instrumentation are included with the patch? How will we know
    > > whether pounding for hours is actually touching all relevant parts
    > > of code? I've done such things myself only to later realise I
    > > wasn't actually testing the right piece of code.
    >  
    > We've looked at distributions of failed transactions by ereport
    > statement.  This confirms that we are indeed exercising the vast
    > majority of the code.  See separate post for how we pushed execution
    > into the summarization path to test code related to that.
    
    BTW did you try "make coverage" and friends?  See
    http://www.postgresql.org/docs/9.0/static/regress-coverage.html
    and
    http://developer.postgresql.org/~petere/coverage/
    
    -- 
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
    
    

18. Re: SSI patch version 14

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-26T19:42:23Z

    Alvaro Herrera <alvherre@commandprompt.com> wrote:
     
    > BTW did you try "make coverage" and friends?  See
    > http://www.postgresql.org/docs/9.0/static/regress-coverage.html
    > and
    > http://developer.postgresql.org/~petere/coverage/
     
    I had missed that; thanks for pointing it out!
     
    I'm doing a coverage build now, to see what coverage we get from
    `make check` (probably not much) and `make dcheck`.
     
    Dan, do you still have access to that machine you were using for the
    DBT-2 runs?  Could we get a coverage run with and without
    TEST_OLDSERXID defined?
     
    -Kevin
    
    

19. Re: SSI patch version 14

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-26T20:10:23Z

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote: 
    > Alvaro Herrera <alvherre@commandprompt.com> wrote:
    >  
    >> BTW did you try "make coverage" and friends?  See
    >> http://www.postgresql.org/docs/9.0/static/regress-coverage.html
    >> and
    >> http://developer.postgresql.org/~petere/coverage/
    >  
    > I had missed that; thanks for pointing it out!
    >  
    > I'm doing a coverage build now, to see what coverage we get from
    > `make check` (probably not much) and `make dcheck`.
     
    Well, that was a bit better than I expected.  While the overall code
    coverage for PostgreSQL source code is:
     
    Overall coverage rate:
      lines......: 64.8% (130296 of 201140 lines)
      functions..: 72.0% (7997 of 11109 functions)
     
    The coverage for predicate.c, after running both check and dcheck,
    was (formatted to match above):
     
      lines......: 69.8% (925 of 1325 lines)
      functions..: 85.7% (48 of 56 functions)
     
    Most of what was missed was in the SLRU or 2PC code, which is
    expected.  I'll bet that the DBT-2 runs, between the "normal"
    and TEST_OLDSERXID flavors, would get us about 2/3 of the way from
    those numbers toward 100%, with almost all the residual being in
    2PC.
     
    Does anyone have suggestions for automated 2PC tests?
     
    -Kevin
    
    

20. Re: SSI patch version 14

    Dan Ports <drkp@csail.mit.edu> — 2011-01-26T20:16:23Z

    On Wed, Jan 26, 2011 at 10:01:28AM -0600, Kevin Grittner wrote:
    > In looking at it just now, I noticed that after trying it in a
    > couple different places what was left in the repository was not the
    > optimal version for code coverage.  I've put this back to the
    > version which did a better job, for reasons described in the commit
    > comment:
    
    Isn't this placement the same as the version we had before that didn't
    work?
    
    Specifically, aren't we going to have problems running with
    TEST_OLDSERXID enabled because CreatePredTran succeeds and links a new
    SerializableXact into the active list, but we don't initialize it
    before we drop SerializableXactHashLock to call
    SummarizeOldestCommittedSxact?
    
    I seem to recall SummarizeOldestCommittedSxact failing before because
    of the uninitialized entry, but more generally since we drop the lock
    something else might scan the list.
    
    (This isn't a problem in the non-test case because we'd only do that if
    CreatePredTran fails.)
    
    Dan
    
    -- 
    Dan R. K. Ports              MIT CSAIL                http://drkp.net/
    
    

21. Re: SSI patch version 14

    Dan Ports <drkp@csail.mit.edu> — 2011-01-26T20:20:09Z

    On Wed, Jan 26, 2011 at 01:42:23PM -0600, Kevin Grittner wrote:
    > Dan, do you still have access to that machine you were using for the
    > DBT-2 runs?  Could we get a coverage run with and without
    > TEST_OLDSERXID defined?
    
    Sure, I'll give it a shot (once I figure out how to enable coverage...)
    
    Dan
    
    -- 
    Dan R. K. Ports              MIT CSAIL                http://drkp.net/
    
    

22. Re: SSI patch version 14

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-26T20:36:25Z

    Dan Ports <drkp@csail.mit.edu> wrote: 
     
    > Isn't this placement the same as the version we had before that
    > didn't work?
     
    You're right.  How about this?:
     
    http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=86b839291e2588e59875fb87d05432f8049575f6
     
    Same benefit in terms of exercising more lines of code, but
    *without* exposing the uninitialized structure to other threads.
     
    -Kevin
    
    

23. Re: SSI patch version 14

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-26T20:43:26Z

    I wrote:
     
    > You're right.  How about this?:
     
    That's even worse.  I'm putting back to where you had it and taking
    a break before I do anything else that dumb.
     
    -Kevin
    
    

24. Re: SSI patch version 14

    Dan Ports <drkp@csail.mit.edu> — 2011-01-26T20:46:22Z

    On Wed, Jan 26, 2011 at 02:36:25PM -0600, Kevin Grittner wrote:
    > Same benefit in terms of exercising more lines of code, but
    > *without* exposing the uninitialized structure to other threads.
    
    Won't this cause a deadlock because locks are being acquired out of
    order?
    
    Dan
    
    -- 
    Dan R. K. Ports              MIT CSAIL                http://drkp.net/
    
    

25. Re: SSI patch version 14

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-27T15:44:32Z

    I wrote:
     
    > While the overall code coverage for PostgreSQL source code is:
    >  
    > Overall coverage rate:
    >   lines......: 64.8% (130296 of 201140 lines)
    >   functions..: 72.0% (7997 of 11109 functions)
     
    By the way, I discovered that the above is lower if you just run the
    check target.  The dcheck target helps with overall PostgreSQL code
    coverage, even though it was targeted at the SSI code.
     
    > The coverage for predicate.c, after running both check and dcheck,
    > was (formatted to match above):
    >  
    >   lines......: 69.8% (925 of 1325 lines)
    >   functions..: 85.7% (48 of 56 functions)
     
    Some minor tweaks to the regression tests boosts that to:
     
      lines......: 73.1% (968 of 1325 lines)
      functions..: 89.3% (50 of 56 functions)
     
    Most of the code not covered in the regular build (above) is in the
    OldSerXidXxxxx functions, which are covered well in a build with
    TEST_OLDSERXID defined.  The 2PC code is very well covered now,
    except for the recovery-time function.  We don't have a way to test
    that in the `make check` process, do we?
     
    There is some code which is not covered just because it is so hard
    to hit -- for example, code which is only executed if vacuum cleans
    up an index page when we are right at the edge of running out of the
    memory used to track predicate locks.  It would be hard to include a
    test for that in the normal regression tests.
     
    The regression test changes are here:
     
    http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=d4c1005d731c81049cc2622e50b7a2ebb99bbcac
     
    -Kevin
    
    

26. Re: SSI patch version 14

    Jeff Davis <pgsql@j-davis.com> — 2011-01-27T17:18:23Z

    On Tue, 2011-01-25 at 05:57 -0500, Dan Ports wrote:
    > This summary is right on. I would add one additional detail or
    > clarification to the last point, which is that rather than checking for
    > a cycle, we're checking for a transaction with both "in" and "out"
    > conflicts, which every cycle must contain.
    
    To clarify, this means that it will get some false positives, right?
    
    For instance:
    
    T1:
      get snapshot
    
    T2:
      get snapshot
      insert R1
      commit
    
    T1:
      read R1
      write R2
    
    T3:
      get snapshot
      read R2
    
    T3:
      commit
    
    T1:
      commit -- throws error
    
    
    T1 has a conflict out to T2, and T1 has a conflict in from T3.
    T2 has a conflict in from T1.
    T3 has a conflict out to T1.
    
    T1 is canceled because it has both a conflict in and a conflict out. But
    the results are the same as a serial order of execution: T3, T1, T2.
    
    Is there a reason we can't check for a real cycle, which would let T1
    succeed?
    
    Regards,
    	Jeff Davis
    
    
    

27. Re: SSI patch version 14

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-27T18:22:45Z

    Jeff Davis <pgsql@j-davis.com> wrote:
     
    > To clarify, this means that it will get some false positives,
    > right?
     
    Yes.  But the example you're about to get into isn't one of them.
     
    > For instance:
    > 
    > T1:
    >   get snapshot
    > 
    > T2:
    >   get snapshot
    >   insert R1
    >   commit
    > 
    > T1:
    >   read R1
    >   write R2
    > 
    > T3:
    >   get snapshot
    >   read R2
    > 
    > T3:
    >   commit
    > 
    > T1:
    >   commit -- throws error
    > 
    > 
    > T1 has a conflict out to T2, and T1 has a conflict in from T3.
    > T2 has a conflict in from T1.
    > T3 has a conflict out to T1.
    > 
    > T1 is canceled because it has both a conflict in and a conflict
    > out. But the results are the same as a serial order of execution:
    > T3, T1, T2.
    > 
    > Is there a reason we can't check for a real cycle, which would let
    > T1 succeed?
     
    Yes.  Because T2 committed before T3 started, it's entirely possible
    that there is knowledge outside the database server that the work of
    T2 was done and committed before the start of T3, which makes the
    order of execution: T2, T3, T1, T2.  So you can have anomalies.  Let
    me give you a practical example.
     
    Pretend there are receipting terminals in public places for the
    organization.  In most financial systems, those receipts are
    assigned to batches of some type.  Let's say that's done by an
    insert for the new batch ID, which closes the old batch.  Receipts
    are always added with the maximum batch ID, reflecting the open
    batch.
     
    Your above example could be:
     
    -- setup
    test=# create table ctl (batch_id int not null primary key);
    NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index
    "ctl_pkey" for table "ctl"
    CREATE TABLE
    test=# create table receipt (batch_id int not null, amt
    numeric(13,2) not null);
    CREATE TABLE
    test=# insert into ctl values (1),(2),(3);
    INSERT 0 3
    test=# insert into receipt values ((select max(batch_id) from ctl),
    50),((select max(batch_id) from ctl), 100);
    INSERT 0 2
     
    -- receipting workstation
    -- T1 starts working on receipt insert transaction
    test=# begin transaction isolation level repeatable read;
    BEGIN
    test=# select 1; -- to grab snapshot, per above
     ?column?
    ----------
            1
    (1 row)
    
    -- accounting workstation
    -- T2 closes old receipt batch; opens new
    test=# begin transaction isolation level repeatable read;
    BEGIN
    test=# insert into ctl values (4);
    INSERT 0 1
    test=# commit;
    COMMIT
    
    -- receipting workstation
    -- T1 continues work on receipt
    test=# select max(batch_id) from ctl;
     max
    -----
       3
    (1 row)
    
    test=# insert into receipt values (3, 1000);
    INSERT 0 1
    
    -- accounting workstation
    -- T3 lists receipts from the closed batch
    -- (Hey, we inserted a new batch_id and successfully
    -- committed, right?  The old batch is closed.)
    test=# begin transaction isolation level repeatable read;
    BEGIN
    test=# select * from receipt where batch_id = 3;
     batch_id |  amt
    ----------+--------
            3 |  50.00
            3 | 100.00
    (2 rows)
    
    test=# commit;
    COMMIT
    
    -- receipting workstation
    -- T1 receipt insert transaction commits
    test=# commit;
    COMMIT
     
    Now, with serializable transactions, as you saw, T1 will be rolled
    back.  With a decent software framework, it will be automatically
    retried, without any user interaction.  It will select max(batch_id)
    of 4 this time, and the insert will succeed and be committed. 
    Accounting's list is accurate.
     
    -Kevin
    
    

28. Re: SSI patch version 14

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-27T18:39:35Z

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:
     
    > Now, with serializable transactions, as you saw, T1 will be rolled
    > back.
     
    I should probably have mentioned, that if all the transactions were
    SERIALIZABLE and the report of transactions from the batch was run
    as SERIALIZABLE READ ONLY DEFERRABLE, the start of the report would
    block until it was certain that it had a snapshot which could not
    lead to an anomaly, so the BEGIN for T3 would wait until the COMMIT
    of T1, get a new snapshot which it would determine to be safe, and
    proceed.  This would allow that last receipt to land in batch 3 and
    show up on accounting's receipt list with no rollbacks *or*
    anomalies.
     
    -Kevin
    
    

29. Re: SSI patch version 14

    Jeff Davis <pgsql@j-davis.com> — 2011-01-28T16:54:34Z

    On Tue, 2011-01-25 at 15:22 -0600, Kevin Grittner wrote:
    > Jeff Davis <pgsql@j-davis.com> wrote:
    >  
    > > I think just annotating RWConflict.in/outLink and
    > > PredTranList.available/activeList with the types of things they
    > > hold would be a help.
    > > 
    > > Also, you say something about RWConflict and "when the structure
    > > is not in use". Can you elaborate on that a bit?
    >  
    > Please let me know whether this works for you:
    >  
    > http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=325ec55e8c9e5179e2e16ff303af6afc1d6e732b
    
    Looks good.
    
    	Jeff
    
    
    

30. Re: SSI patch version 14

    Dan Ports <drkp@csail.mit.edu> — 2011-01-29T04:36:40Z

    On Thu, Jan 27, 2011 at 09:18:23AM -0800, Jeff Davis wrote:
    > On Tue, 2011-01-25 at 05:57 -0500, Dan Ports wrote:
    > > This summary is right on. I would add one additional detail or
    > > clarification to the last point, which is that rather than checking for
    > > a cycle, we're checking for a transaction with both "in" and "out"
    > > conflicts, which every cycle must contain.
    > 
    > To clarify, this means that it will get some false positives, right?
    
    Yes, this is correct.
    
    > Is there a reason we can't check for a real cycle, which would let T1
    > succeed?
    
    I talked today with someone who experimented with doing exactly that in
    MySQL as part of a research project (Perfectly Serializable Snapshot
    Isolation, paper forthcoming in ICDE)
    
    It confirmed my intuition that this is possible but not as
    straightforward as it sounds. Some complications I thought of in
    adapting that to what we're doing:
    
    1) it requires tracking all edges in the serialization graph; besides
       the rw-conflicts we track there are also the more mundane
       rw-dependencies (if T2 sees an update made by T1, then T2 has to
       come after T1) and ww-dependencies (if T2's write modifies a tuple
       created by T1, then T2 has to come after T1).
       
       We are taking advantage of the fact that any cycle must have two
       adjacent rw-conflict edges, but the rest of the cycle can be
       composed of other types. It would certainly be possible to track the
       others, but it would add a bit more complexity and use more memory.
    
    2) it requires doing a depth-first search to check for a cycle, which
       is more expensive than just detecting two edges. That seems OK if
       you only want to check it on commit, but maybe not if you want to
       detect serialization failures at the time of the conflicting
       read/write (as we do).
    
    3) it doesn't seem to interact very well with our memory mitigation
       efforts, wherein we discard lots of data about committed
       transactions that we know we won't need. If we were checking for
       cycles, we'd need to keep more of it. I suspect summarization (when
       we combine two transactions' predicate locks to save memory) would
       also cause problems.
    
    None of these seem particularly insurmountable, but they suggest it
    isn't a clear win to try to find an entire cycle.
    
    Dan
    
    -- 
    Dan R. K. Ports              MIT CSAIL                http://drkp.net/
    
    

31. Re: SSI patch version 14

    Jeff Davis <pgsql@j-davis.com> — 2011-01-31T03:33:37Z

    1. In CheckForSerializableConflictIn(), I think the comment above may be
    out of date. It says:
    
    "A tuple insert is in conflict only if there is a predicate lock against
    the entire relation."
    
    That doesn't appear to be true if, for example, there's a predicate lock
    on the index page that the tuple goes into. I examined it with gdb, and
    it calls the function, and the function does identify the conflict.
    
    2. Also in the comment above CheckForSerializableConflictIn(), I see:
    
    "The call to this function also indicates that we need an entry in the
    serializable transaction hash table, so that this write's conflicts can
    be detected for the proper lifetime, which is until this transaction and
    all overlapping serializable transactions have completed."
    
    which doesn't make sense to me. The transaction should already have an
    entry in the hash table at this point, right?
    
    3. The comment above CheckForSerializableConflictOut() seems to trail
    off, as though you may have meant to write more. It also seems to be out
    of date.
    
    And why are you reading the infomask directly? Do the existing
    visibility functions not suffice?
    
    
    
    I have made it through predicate.c, and I have a much better
    understanding of what it's actually doing. I can't claim that I have a
    clear understanding of everything involved, but the code looks like it's
    in good shape (given the complexity involved) and well-commented.
    
    I am marking the patch Ready For Committer, because any committer will
    need time to go through the patch; and the authors have clearly applied
    the thought, care, and testing required for something of this
    complexity. I will continue to work on it, though.
    
    The biggest issue on my mind is what to do about Hot Standby. The
    authors have a plan, but there is also some resistance to it:
    
    http://archives.postgresql.org/message-id/23698.1295566621@sss.pgh.pa.us
    
    We don't need a perfect solution for 9.1, but it would be nice if we had
    a viable plan for 9.2.
    
    Regards,
    	Jeff Davis
    
    
    

32. SSI patch version 15

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-31T17:31:54Z

    Jeff Davis <pgsql@j-davis.com> wrote:
     
    > 1. [& 2.] In CheckForSerializableConflictIn()
     
    All of that was obsolete and could just be deleted.  I did.
     
    > 3. The comment above CheckForSerializableConflictOut()
     
    I reworked the comment there.  Hopefully it is now more clear.
     
    > I am marking the patch Ready For Committer
     
    Patch v15 is attached with fixes for all issues identified in v14. 
    There was one (two-line) bug fix.  No other logic changes.  We had
    an addition to the README-SSI file, comment changes, doc changes,
    changes to the text of a few messages, and some structure/field
    renames to avoid using Tran as an abbreviation for transaction in
    addition to the use of Xact as an abbreviation.
     
    Pretty minimal differences from V14, but I figured it would save the
    committer some work if I rolled them all up here.
     
    -Kevin
    

33. Re: SSI patch version 15

    Robert Haas <robertmhaas@gmail.com> — 2011-01-31T18:05:28Z

    On Mon, Jan 31, 2011 at 12:31 PM, Kevin Grittner
    <Kevin.Grittner@wicourts.gov> wrote:
    > Pretty minimal differences from V14, but I figured it would save the
    > committer some work if I rolled them all up here.
    
    Sounds good.  I believe Heikki is planning to work on this one.
    Hopefully that will happen soon, since we are running short on time.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    

34. Re: SSI patch version 15

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-01-31T21:35:53Z

    On 31.01.2011 20:05, Robert Haas wrote:
    > On Mon, Jan 31, 2011 at 12:31 PM, Kevin Grittner
    > <Kevin.Grittner@wicourts.gov>  wrote:
    >> Pretty minimal differences from V14, but I figured it would save the
    >> committer some work if I rolled them all up here.
    >
    > Sounds good.  I believe Heikki is planning to work on this one.
    > Hopefully that will happen soon, since we are running short on time.
    
    Yeah, I can commit this. Jeff, are you satisfied with this patch now? 
    I'm glad you're reviewing this, more eyeballs helps a lot with a big 
    patch like this.
    
    -- 
       Heikki Linnakangas
       EnterpriseDB   http://www.enterprisedb.com
    
    

35. Re: SSI patch version 15

    Jeff Davis <pgsql@j-davis.com> — 2011-01-31T22:46:48Z

    On Mon, 2011-01-31 at 23:35 +0200, Heikki Linnakangas wrote:
    > Yeah, I can commit this. Jeff, are you satisfied with this patch now? 
    > I'm glad you're reviewing this, more eyeballs helps a lot with a big 
    > patch like this.
    
    I think the patch is very close. I am doing my best in my free time to
    complete a thorough review. If you have other patches to review/commit
    then I will still be making progress reviewing SSI.
    
    However, I would recommend leaving yourself some time to think on this
    one if you don't already understand the design well.
    
    Regards,
    	Jeff Davis
    
    
    

36. Re: SSI patch version 14

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-02-02T10:20:53Z

    On 26.01.2011 13:36, Simon Riggs wrote:
    > I looked at the patch to begin a review and immediately saw "dtest".
    > There's no docs to explain what it is, but a few comments fill me in a
    > little more. Can we document that please? And/or explain why its an
    > essential part of this commit? Could we keep it out of core, or if not,
    > just commit that part separately? I notice the code is currently
    > copyright someone else than PGDG.
    
    We still haven't really resolved this..
    
    Looking at the dtest suite, I'd really like to have those tests in the 
    PostgreSQL repository. However, I'd really like not to require python to 
    run them, and even less dtester (no offense Markus) and the dependencies 
    it has. I couldn't get it to run on my basic Debian system, clearly I'm 
    doing something wrong, but it will be even harder for people on more 
    exotic platforms.
    
    The tests don't seem very complicated, and they don't seem to depend 
    much on the dtester features. How hard would it be to rewrite the test 
    engine in C or perl?
    
    I'm thinking of defining each test in a simple text file, and write a 
    small test engine to parse that.
    
    -- 
       Heikki Linnakangas
       EnterpriseDB   http://www.enterprisedb.com
    
    

37. Re: SSI patch version 14

    Markus Wanner <markus@bluegap.ch> — 2011-02-02T12:48:54Z

    Heikki,
    
    On 02/02/2011 11:20 AM, Heikki Linnakangas wrote:
    > I couldn't get it to run on my basic Debian system, clearly I'm
    > doing something wrong, but it will be even harder for people on more
    > exotic platforms.
    
    On Debian you only need 'python-twisted' and the dtester sources [1],
    IIRC.  What issue did you run into?
    
    > The tests don't seem very complicated, and they don't seem to depend
    > much on the dtester features. How hard would it be to rewrite the test
    > engine in C or perl?
    
    I've taken a quick look at a perl framework similar to twisted (POE).
    Doesn't seem too complicated, but I don't see the benefit of requiring
    one over the other.  Another async, event-based framework that I've been
    playing with is asio (C++).
    
    I don't think it's feasible to write anything similar without basing on
    such a framework.  (And twisted seems more mature and widespread (in
    terms of supported platforms) than POE, but that's probably just my
    subjective impression).
    
    However, the question here probably is whether or not you need all of
    the bells and whistles of dtester (as if there were that many) [2].
    
    > I'm thinking of defining each test in a simple text file, and write a
    > small test engine to parse that.
    
    AFAIUI there are lots of possible permutations, so you don't want to
    have to edit files for each manually (several hundreds, IIRC).  So using
    a scripting language to generate the permutations makes sense, IMO.
    
    Pretty much everything else that Kevin currently uses dtester for (i.e.
    initdb, running the postmaster, connecting with psql) is covered by the
    existing regression testing infrastructure already, I think.  So it
    might be a matter of integrating the permutation generation and test
    running code into the existing infrastructure.  Kevin?
    
    Regards
    
    Markus Wanner
    
    
    [1]: dtester project site:
    http://www.bluegap.ch/projects/dtester/
    
    [2]: side note:
    Dtester mainly serves me to test Postgres-R, where the current
    regression testing infrastructure simply doesn't suffice.  With dtester
    I tried to better structure the processes and services, their
    dependencies and the test environment.
    
    

38. Re: SSI patch version 14

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-02-02T12:56:55Z

    On 02.02.2011 12:20, Heikki Linnakangas wrote:
    > On 26.01.2011 13:36, Simon Riggs wrote:
    >> I looked at the patch to begin a review and immediately saw "dtest".
    >> There's no docs to explain what it is, but a few comments fill me in a
    >> little more. Can we document that please? And/or explain why its an
    >> essential part of this commit? Could we keep it out of core, or if not,
    >> just commit that part separately? I notice the code is currently
    >> copyright someone else than PGDG.
    >
    > We still haven't really resolved this..
    >
    > Looking at the dtest suite, I'd really like to have those tests in the
    > PostgreSQL repository. However, I'd really like not to require python to
    > run them, and even less dtester (no offense Markus) and the dependencies
    > it has. I couldn't get it to run on my basic Debian system, clearly I'm
    > doing something wrong, but it will be even harder for people on more
    > exotic platforms.
    >
    > The tests don't seem very complicated, and they don't seem to depend
    > much on the dtester features. How hard would it be to rewrite the test
    > engine in C or perl?
    >
    > I'm thinking of defining each test in a simple text file, and write a
    > small test engine to parse that.
    
    I took a stab at doing that. Attached, untar in the top source tree, and 
    do "cd src/test/isolation; make check". It's like "installcheck", so it 
    needs a postgres server to be running. Also available in my git 
    repository at git://git.postgresql.org/git/users/heikki/postgres.git, 
    branch "serializable".
    
    I think this will work. This is a rough first version, but it already 
    works. We can extend the test framework later if we need more features, 
    but I believe this is enough for all the existing tests.
    
    -- 
       Heikki Linnakangas
       EnterpriseDB   http://www.enterprisedb.com
    

39. Re: SSI patch version 14

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-02-02T13:04:28Z

    On 02.02.2011 14:48, Markus Wanner wrote:
    > Heikki,
    >
    > On 02/02/2011 11:20 AM, Heikki Linnakangas wrote:
    >> I couldn't get it to run on my basic Debian system, clearly I'm
    >> doing something wrong, but it will be even harder for people on more
    >> exotic platforms.
    >
    > On Debian you only need 'python-twisted' and the dtester sources [1],
    > IIRC.  What issue did you run into?
    
    I installed dtester with:
    
    PYTHONPATH=~/pythonpath/lib/python2.6/site-packages/ python ./setup.py 
    install --prefix=/home/heikki/pythonpath/
    
    And that worked. But when I try to run Kevin's test suite, I get this:
    
    ~/git-sandbox/postgresql/src/test/regress (serializable)$ 
    PYTHONPATH=~/pythonpath/lib/python2.6/site-packages/ make dcheck
    ./pg_dtester.py --temp-install --top-builddir=../../.. \
             --multibyte=
    Postgres dtester suite                Copyright (c) 2004-2010, by Markus 
    Wanner
    
    Traceback (most recent call last):
       File "./pg_dtester.py", line 1376, in <module>
         main(sys.argv[1:])
       File "./pg_dtester.py", line 1370, in main
         runner = Runner(reporter=TapReporter(sys.stdout, sys.stderr, 
    showTimingInfo=True),
    TypeError: __init__() got an unexpected keyword argument 'showTimingInfo'
    make: *** [dcheck] Virhe 1
    
    At that point I had no idea what's wrong.
    
    PS. I tried "python ./setup.py dtest", mentioned in the README, and it 
    just said "invalid command 'dtest'". But I presume the installation 
    worked anyway.
    
    > Pretty much everything else that Kevin currently uses dtester for (i.e.
    > initdb, running the postmaster, connecting with psql) is covered by the
    > existing regression testing infrastructure already, I think.  So it
    > might be a matter of integrating the permutation generation and test
    > running code into the existing infrastructure.  Kevin?
    
    Yeah, that was my impression too. (see my other post with WIP 
    implementation of that)
    
    -- 
       Heikki Linnakangas
       EnterpriseDB   http://www.enterprisedb.com
    
    

40. Re: SSI patch version 14

    Markus Wanner <markus@bluegap.ch> — 2011-02-02T13:22:24Z

    Heikki,
    
    On 02/02/2011 02:04 PM, Heikki Linnakangas wrote:
    > TypeError: __init__() got an unexpected keyword argument 'showTimingInfo'
    > make: *** [dcheck] Virhe 1
    > 
    > At that point I had no idea what's wrong.
    
    Hm.. looks like Kevin already uses the latest dtester code from git.
    You can either do that as well, or try to drop the 'showTimingInfo'
    argument from the call to Runner() in pg_dtester.py:1370.
    
    > PS. I tried "python ./setup.py dtest", mentioned in the README, and it
    > just said "invalid command 'dtest'". But I presume the installation
    > worked anyway.
    
    Yeah, that's a gotcha in dtester.  setup.py dtest requires dtester to be
    installed - a classic chicken and egg problem.  I certainly need to look
    into this.
    
    > Yeah, that was my impression too. (see my other post with WIP
    > implementation of that)
    
    I'd like to get Kevin's opinion on that, but it certainly sounds like a
    pragmatic approach for now.
    
    Regards
    
    Markus Wanner