Thread

1. Re: SSI work for 9.1

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-09T02:17:04Z

   > Dan Ports  wrote:
   > On Wed, Jun 08, 2011 at 05:48:26PM -0500, Kevin Grittner wrote:
   >> (1) Pass snapshot in to some predicate.c functions. The particular
   >> functions have yet to be determined, but certainly any which
   >> acquire predicate locks, and probably all which are guarded by the
   >> SkipSerialization() macro. Skip processing for non-MVCC snapshots.
   >> The goal here is to reduce false positive serialization failures
   >> and avoid confusion about locks showing in the pg_locks view which
   >> are hard to explain.
   > 
   > I assume you've already started on this one; let me know if you
   > have a patch I should take a look at or hit any snags.
    
   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.
    
   FWIW, this patch not only passes the usual battery of regression
   tests that I run, but the test which was showing REINDEX acquiring
   predicate locks on the heap runs without that happening.
    
   I'm posting this because I think it's the most important area to
   cover, and in a pinch might satisfy people for 9.1; but more
   importantly to give me a good place to step back to in case I muck
   things up moving forward from this point.
    
   >> (2) Check on heap truncation from vacuum. On the face of it this
   >> seems unlikely to be a problem since we make every effort to clean
   >> up predicate locks as soon as there is no transaction which can
   >> update what a transaction has read, but it merits a re-check. Once
   >> confirmed, add a note to lazy_truncate_heap about why it's not an
   >> issue.
   > 
   > I assume you are worried here that there may be SIREAD locks
   > remaining on truncated pages/tuples, and these would cause false
   > positives if those pages are reused?
   > 
   > I don't believe this can happen, because a vacuum will only delete
   > a formerly-visible dead tuple if its xmax is earlier than
   > OldestXmin. We remove all SIREAD locks on anything older than
   > GlobalSxactXmin, which won't be less than OldestXmin.
    
   That was my first thought, too.  But, the question being raised, I
   thought a quick double-check that there weren't any corner cases
   where this could occur was reasonable.
    
   >> (4) Add a comment to the docs about how querying tuples by TID
   >> doesn't lock not-found "gaps" the way an indexed access would.
   > 
   > I can take care of this one and some other README-SSI changes.
    
   OK, I'll stay away from any doc items for now.  Those are all yours
   until we agree otherwise. :-)  I really don't think I'm going to get
   past the snapshot guard issue tonight.  :-/
    
   -Kevin
   
   
   

2. Re: SSI work for 9.1

   Dan Ports <drkp@csail.mit.edu> — 2011-06-09T03:59:43Z

   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.
   
   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.
   
   I've updated the patch, attached.
   
   Dan
   
   -- 
   Dan R. K. Ports              MIT CSAIL                http://drkp.net/