Thread

1. Re: SSI work for 9.1

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-09T12:06:18Z

   Dan Ports  wrote:
   > On Wed, Jun 08, 2011 at 09:17:04PM -0500, Kevin Grittner wrote:
    
   >> A patch is attached which just covers the predicate lock
   >> acquisition, where a snapshot is available without too much pain.
   >> There are two functions which acquire predicate locks where a
   >> snapshot was not readily available: _bt_search() and
   >> _bt_get_endpoint(). Not only was it not clear how to get a
   >> snapshot in, it was not entirely clear from reading the code that
   >> we need to acquire predicate locks here. Now, I suspect that we
   >> probably do, because I spent many long hours stepping through gdb
   >> to pick the spots where they are, but that was about a year ago
   >> and my memory of the details has faded.
   > 
   > For _bt_search(), the lock calls should move to _bt_first() where
   > the ScanDesc is available. This also keeps us from trying to take
   > locks during _bt_pagedel(), which is only called during vacuum and
   > recovery.
    
   Sounds reasonable, but why did you pass the snapshot to the
   PredicateLockPage() call but not the PredicateLockRelation() call? 
   Oversight?
    
   > The call in _bt_get_endpoint() seems unnecessary, because after it
   > returns, _bt_endpoint() takes the same lock. The only other callers
   > of _bt_get_endpoint() are _bt_pagedel() and _bt_insert_parent(),
   > neither of which should take predicate locks.
    
   That also sounds reasonable.
    
   > I've updated the patch, attached.
    
   I've confirmed that it passes the usual regression tests (including
   isolation tests and the normal regression tests at serializable). 
   I'll take a closer look once I wake up and get the caffeine going.
    
   Thanks for following up on this!
    
   -Kevin
   
   
   

2. Re: SSI work for 9.1

   Dan Ports <drkp@csail.mit.edu> — 2011-06-09T17:30:27Z

   On Thu, Jun 09, 2011 at 07:06:18AM -0500, Kevin Grittner wrote:
   > Sounds reasonable, but why did you pass the snapshot to the
   > PredicateLockPage() call but not the PredicateLockRelation() call? 
   > Oversight?
   
   Yep, just an oversight; long day yesterday. I'll fix the patch shortly
   (unless you can get to it first, in which case I wouldn't complain)
   
   Dan
   
   -- 
   Dan R. K. Ports              MIT CSAIL                http://drkp.net/
   
   

3. Re: SSI work for 9.1

   Dan Ports <drkp@csail.mit.edu> — 2011-06-09T18:03:20Z

   On Thu, Jun 09, 2011 at 01:30:27PM -0400, Dan Ports wrote:
   > On Thu, Jun 09, 2011 at 07:06:18AM -0500, Kevin Grittner wrote:
   > > Sounds reasonable, but why did you pass the snapshot to the
   > > PredicateLockPage() call but not the PredicateLockRelation() call? 
   > > Oversight?
   > 
   > Yep, just an oversight; long day yesterday. I'll fix the patch shortly
   
   Attached. Only change is the missing snapshot argument to that one
   call.
   
   Dan
   
   -- 
   Dan R. K. Ports              MIT CSAIL                http://drkp.net/
   

4. Re: SSI work for 9.1

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-09T20:33:25Z

   Dan Ports <drkp@csail.mit.edu> wrote:
   > On Thu, Jun 09, 2011 at 01:30:27PM -0400, Dan Ports wrote:
   >> On Thu, Jun 09, 2011 at 07:06:18AM -0500, Kevin Grittner wrote:
   >>> Sounds reasonable, but why did you pass the snapshot to the
   >>> PredicateLockPage() call but not the PredicateLockRelation()
   >>> call?  Oversight?
   >> 
   >> Yep, just an oversight; long day yesterday. I'll fix the patch
   >> shortly
   > 
   > Attached. Only change is the missing snapshot argument to that one
   > call.
    
   I am in full agreement with this patch.
    
   It's an interesting coincidence that the two predicate locking calls
   which were at the wrong level to have access to the snapshot
   information were also at the wrong level to be able to fire them at
   only the needed times.
    
   -Kevin
   
   

5. Re: SSI work for 9.1

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-10T16:05:38Z

   "Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:
    
   > I am in full agreement with this patch.
    
   I found that pgindent would like to tweak whitespace in three places
   in that patch, and I found an unnecessary include that I would like
   to remove.  Normally, I would post a new version of the patch with
   those adjustments, but there's been a disquieting tendency for
   people to not look at what's actually happening with revisions of
   this patch and act like the sky is falling with each refinement.
    
   Let me be clear: each posted version of this patch has been safe and
   has been an improvement on what came before.  Dan and I didn't
   disagree about what to do at any point; Dan figured out what to do
   with two calls I left alone because I faded before I could work out
   how to deal with them.  Essentially we collaborated on-list rather
   than discussing things off-list and posting the end result.  Perhaps
   that was a bad choice, but time was short and I had hopes that a
   change people had requested could be included in beta2.
    
   Here are the tweaks which should be applied after Dan's last version
   of the patch.  If people prefer, I'll post a roll-up including them.
    
   http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=0258af4168a130af0d1e52294b248d54715b5f72
    
   http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=bb951bacd9700cdbddb0beba338a63bd301b200d
    
   -Kevin
   
   

6. Re: SSI work for 9.1

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-06-14T11:03:05Z

   On 10.06.2011 19:05, Kevin Grittner wrote:
   > I found that pgindent would like to tweak whitespace in three places
   > in that patch, and I found an unnecessary include that I would like
   > to remove.  Normally, I would post a new version of the patch with
   > those adjustments, but there's been a disquieting tendency for
   > people to not look at what's actually happening with revisions of
   > this patch and act like the sky is falling with each refinement.
   >
   > Let me be clear: each posted version of this patch has been safe and
   > has been an improvement on what came before.  Dan and I didn't
   > disagree about what to do at any point; Dan figured out what to do
   > with two calls I left alone because I faded before I could work out
   > how to deal with them.  Essentially we collaborated on-list rather
   > than discussing things off-list and posting the end result.  Perhaps
   > that was a bad choice, but time was short and I had hopes that a
   > change people had requested could be included in beta2.
   
   I did some further changes, refactoring SkipSerialization so that it's 
   hopefully more readable, and added a comment about the side-effects. See 
   attached. Let me know if I'm missing something.
   
   -- 
      Heikki Linnakangas
      EnterpriseDB   http://www.enterprisedb.com
   

7. Re: SSI work for 9.1

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-14T14:57:57Z

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:
    
   > I did some further changes, refactoring SkipSerialization so that
   > it's hopefully more readable, and added a comment about the
   > side-effects. See attached. Let me know if I'm missing something.
    
   I do think the changes improve readability.  I don't see anything
   missing, but there's something we can drop.  Now that you've split
   the read and write tests, this part can be dropped from the
   SerializationNeededForWrite function:
    
   +
   +	/* Check if we have just become "RO-safe". */
   +	if (SxactIsROSafe(MySerializableXact))
   +	{
   +		ReleasePredicateLocks(false);
   +		return false;
   +	}
    
   If it's doing a write, it can't be a read-only transaction....
    
   -Kevin
   
   

8. Re: SSI work for 9.1

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-06-15T09:10:30Z

   On 14.06.2011 17:57, Kevin Grittner wrote:
   > Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  wrote:
   >
   >> I did some further changes, refactoring SkipSerialization so that
   >> it's hopefully more readable, and added a comment about the
   >> side-effects. See attached. Let me know if I'm missing something.
   >
   > I do think the changes improve readability.  I don't see anything
   > missing, but there's something we can drop.  Now that you've split
   > the read and write tests, this part can be dropped from the
   > SerializationNeededForWrite function:
   >
   > +
   > +	/* Check if we have just become "RO-safe". */
   > +	if (SxactIsROSafe(MySerializableXact))
   > +	{
   > +		ReleasePredicateLocks(false);
   > +		return false;
   > +	}
   >
   > If it's doing a write, it can't be a read-only transaction....
   
   Ah, good point. Committed with that removed.
   
   -- 
      Heikki Linnakangas
      EnterpriseDB   http://www.enterprisedb.com
   
   

9. Re: SSI work for 9.1

   Robert Haas <robertmhaas@gmail.com> — 2011-06-17T03:49:48Z

   On Wed, Jun 15, 2011 at 5:10 AM, Heikki Linnakangas
   <heikki.linnakangas@enterprisedb.com> wrote:
   > On 14.06.2011 17:57, Kevin Grittner wrote:
   >>
   >> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  wrote:
   >>
   >>> I did some further changes, refactoring SkipSerialization so that
   >>> it's hopefully more readable, and added a comment about the
   >>> side-effects. See attached. Let me know if I'm missing something.
   >>
   >> I do think the changes improve readability.  I don't see anything
   >> missing, but there's something we can drop.  Now that you've split
   >> the read and write tests, this part can be dropped from the
   >> SerializationNeededForWrite function:
   >>
   >> +
   >> +       /* Check if we have just become "RO-safe". */
   >> +       if (SxactIsROSafe(MySerializableXact))
   >> +       {
   >> +               ReleasePredicateLocks(false);
   >> +               return false;
   >> +       }
   >>
   >> If it's doing a write, it can't be a read-only transaction....
   >
   > Ah, good point. Committed with that removed.
   
   Does this mean that the open item "more SSI loose ends" can now be
   marked resolved?
   
   http://wiki.postgresql.org/wiki/PostgreSQL_9.1_Open_Items
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

10. Re: SSI work for 9.1

    Dan Ports <drkp@csail.mit.edu> — 2011-06-17T04:30:16Z

    On Thu, Jun 16, 2011 at 11:49:48PM -0400, Robert Haas wrote:
    > Does this mean that the open item "more SSI loose ends" can now be
    > marked resolved?
    
    I was just looking at it and contemplating moving it to the non-blockers
    list. Of the five items:
     - (1) and (4) are resolved
     - (2) isn't an issue -- maybe we want to add a comment, someplace but
       I'm not convinced even that is necessary
     - (3) is a regression test, and is already on the list separately
     - (5) is a doc issue only
    
    There are no open issues with the code that I'm aware of.
    
    Dan
    
    -- 
    Dan R. K. Ports              MIT CSAIL                http://drkp.net/
    
    

11. Re: SSI work for 9.1

    Robert Haas <robertmhaas@gmail.com> — 2011-06-17T04:32:46Z

    On Fri, Jun 17, 2011 at 12:30 AM, Dan Ports <drkp@csail.mit.edu> wrote:
    > On Thu, Jun 16, 2011 at 11:49:48PM -0400, Robert Haas wrote:
    >> Does this mean that the open item "more SSI loose ends" can now be
    >> marked resolved?
    >
    > I was just looking at it and contemplating moving it to the non-blockers
    > list. Of the five items:
    >  - (1) and (4) are resolved
    >  - (2) isn't an issue -- maybe we want to add a comment, someplace but
    >   I'm not convinced even that is necessary
    >  - (3) is a regression test, and is already on the list separately
    >  - (5) is a doc issue only
    >
    > There are no open issues with the code that I'm aware of.
    
    Perhaps it would be best to remove the general item and replace it
    with a list of more specific things that need doing - which might just
    mean #5.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    

12. Re: SSI work for 9.1

    Dan Ports <drkp@csail.mit.edu> — 2011-06-17T04:47:25Z

    On Fri, Jun 17, 2011 at 12:32:46AM -0400, Robert Haas wrote:
    > Perhaps it would be best to remove the general item and replace it
    > with a list of more specific things that need doing - which might just
    > mean #5.
    
    Done.
    
    -- 
    Dan R. K. Ports              MIT CSAIL                http://drkp.net/