Thread

1. Re: FlexLocks

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-11-24T00:18:16Z

   "Kevin Grittner"  wrote:
   > Robert Haas  wrote:
    
   >> Updated patches attached.
   > 
   > I have to admit I don't have my head around the extraWaits issue,
   > so I can't personally vouch for that code, although I have no
   > reason to doubt it, either. Everything else was something that I at
   > least *think* I understand, and it looked good to me.
   > 
   > I'm not changing the status until I get through the other patch
   > file, which should be tomorrow.
    
   Most of the procarraylock-v1.patch file was pretty straightforward,
   but I have a few concerns.
    
   Why is it OK to drop these lines from the else condition in
   ProcArrayEndTransaction()?:
    
           /* must be cleared with xid/xmin: */
           proc->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
    
   The extraWaits code still looks like black magic to me, so unless
   someone can point me in the right direction to really understand
   that, I can't address whether it's OK.
    
   The need to modify flexlock_internals.h and flexlock.c seems to me to
   indicate a lack of desirable modularity here.  The lower level object
   type shouldn't need to know about each and every implementation of a
   higher level type which uses it, particularly not compiled in like
   that.  It would be really nice if each of the higher level types
   "registered" with flexlock at runtime, so that the areas modified at
   the flexlock level in this patch file could be generic.  Among other
   things, this could allow extensions to use specialized types, which
   seems possibly useful.  Does that (or some other technique to address
   the concern) seem feasible?
    
   -Kevin
   
   

2. Re: FlexLocks

   Robert Haas <robertmhaas@gmail.com> — 2011-11-30T20:26:54Z

   On Wed, Nov 23, 2011 at 7:18 PM, Kevin Grittner
   <Kevin.Grittner@wicourts.gov> wrote:
   > Why is it OK to drop these lines from the else condition in
   > ProcArrayEndTransaction()?:
   >
   >        /* must be cleared with xid/xmin: */
   >        proc->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
   
   It's probably not.  Oops.
   
   I believe the attached patch versions address your comments regarding
   the flexlock patch as well; it is also rebased over the PGXACT patch,
   which has since been committed.
   
   > The extraWaits code still looks like black magic to me, so unless
   > someone can point me in the right direction to really understand
   > that, I can't address whether it's OK.
   
   I don't think I've changed the behavior, so it should be fine.  The
   idea is that something like this can happen:
   
   1. Backend #1 does some action which will eventually cause some other
   process to send it a wakeup (like adding itself to the wait-queue for
   a heavyweight lock).
   2. Before actually going to sleep, backend #1 tries to acquire an
   LWLock.  The LWLock is not immediately available, so it sleeps on its
   process semaphore.
   3. Backend #2 sees the shared memory state created in step one and
   decides sends a wakeup to backend #1 (for example, because the lock is
   now available).
   4. Backend #1 receives the wakeup.  It duly reacquires the spinlock
   protecting the LWLock, sees that the LWLock is not available, releases
   the spinlock, and goes back to sleep.
   5. Backend #3 now releases the LWLock that backend #1 is trying to
   acquire and, as backend #1 is first in line, it sends backend #1 a
   wakeup.
   6. Backend #1 now wakes up again, reacquires the spinlock, gets the
   lwlock, releases the spinlock, does some stuff, and releases the
   lwlock.
   7. Backend #1, having now finished what it needed to do while holding
   the lwlock, is ready to go to sleep and wait for the event that it
   queued up for back in step #1.  However, the wakeup for that event
   *has already arrived* and was consumed by the LWLock machinery.  So
   when backend #1 goes to sleep, it's waiting for a wakeup that will
   never arrive, because it already did arrive, and got eaten.
   
   The solution is the "extraWaits" thing; in step #6, we remember that
   we received an extra, useless wakeup in step #4 that we threw away.
   To make up for having thrown away a wakeup someone else sent us in
   step #3, we send ourselves a wakeup in step #6.  That way, when we go
   to sleep in step #7, we wake up immediately, just as we should.
   
   > The need to modify flexlock_internals.h and flexlock.c seems to me to
   > indicate a lack of desirable modularity here.  The lower level object
   > type shouldn't need to know about each and every implementation of a
   > higher level type which uses it, particularly not compiled in like
   > that.  It would be really nice if each of the higher level types
   > "registered" with flexlock at runtime, so that the areas modified at
   > the flexlock level in this patch file could be generic.  Among other
   > things, this could allow extensions to use specialized types, which
   > seems possibly useful.  Does that (or some other technique to address
   > the concern) seem feasible?
   
   Possibly; let me think about that.  I haven't addressed that in this version.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   

3. Re: FlexLocks

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-11-30T23:57:51Z

   Robert Haas <robertmhaas@gmail.com> wrote:
   > Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote:
   >> Why is it OK to drop these lines from the else condition in
   >> ProcArrayEndTransaction()?:
   >>
   >>        /* must be cleared with xid/xmin: */
   >>        proc->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
   > 
   > It's probably not.  Oops.
    
   OK.  I see that's back now.
    
   > I believe the attached patch versions address your comments
   > regarding the flexlock patch as well; it is also rebased over the
   > PGXACT patch, which has since been committed.
    
   Applies cleanly again.
    
   >> The extraWaits code still looks like black magic to me, so unless
   >> someone can point me in the right direction to really understand
   >> that, I can't address whether it's OK.
   > 
   > I don't think I've changed the behavior, so it should be fine. 
   > The idea is that something like this can happen:
   > 
   > [explanation of the extraWaits behavior]
    
   Thanks.  I'll spend some time reviewing this part.  There is some
   rearrangement of related code, and this should arm me with enough of
   a grasp to review that.
    
   >> [gripes about modularity compromise and lack of pluggability]
    
   > let me think about that.  I haven't addressed that in this
   > version.
    
   OK.  There are a few things I found in this pass which missed in the
   last.  One contrib module was missed, I found another typo in a
   comment, and I think we can reduce the include files a bit.  Rather
   than describe it, I'm attaching a patch file over the top of your
   patches with what I think might be a good idea.  I don't think
   there's anything here to merit a new round of benchmarking.
    
   -Kevin
   
   

4. Re: FlexLocks

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-12-01T00:01:50Z

   "Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:
    
   > OK.  There are a few things I found in this pass which missed in the
   > last.  One contrib module was missed, I found another typo in a
   > comment, and I think we can reduce the include files a bit.  Rather
   > than describe it, I'm attaching a patch file over the top of your
   > patches with what I think might be a good idea.
    
   This time with it really attached.
    
   -Kevin
   
   

5. Re: FlexLocks

   Robert Haas <robertmhaas@gmail.com> — 2011-12-01T18:00:04Z

   On Wed, Nov 30, 2011 at 7:01 PM, Kevin Grittner
   <Kevin.Grittner@wicourts.gov> wrote:
   > "Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:
   >
   >> OK.  There are a few things I found in this pass which missed in the
   >> last.  One contrib module was missed, I found another typo in a
   >> comment, and I think we can reduce the include files a bit.  Rather
   >> than describe it, I'm attaching a patch file over the top of your
   >> patches with what I think might be a good idea.
   >
   > This time with it really attached.
   
   Thanks, I've merged those in.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

6. Re: FlexLocks

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-12-02T21:11:04Z

   "Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:
   > Robert Haas <robertmhaas@gmail.com> wrote:
   >> Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote:
    
   >>> The extraWaits code still looks like black magic to me
    
   >> [explanation of the extraWaits behavior]
   > 
   > Thanks.  I'll spend some time reviewing this part.  There is some
   > rearrangement of related code, and this should arm me with enough
   > of a grasp to review that.
    
   I got through that without spotting any significant problems,
   although I offer the attached micro-optimizations for your
   consideration.  (Applies over the top of your patches.)
    
   As far as I'm concerned it looks great and "Ready for Committer"
   except for the modularity/pluggability question.  Perhaps that could
   be done as a follow-on patch (if deemed a good idea)?
    
   -Kevin
   
   

7. Re: FlexLocks

   Robert Haas <robertmhaas@gmail.com> — 2011-12-15T19:02:59Z

   On Fri, Dec 2, 2011 at 4:11 PM, Kevin Grittner
   <Kevin.Grittner@wicourts.gov> wrote:
   > As far as I'm concerned it looks great and "Ready for Committer"
   > except for the modularity/pluggability question.  Perhaps that could
   > be done as a follow-on patch (if deemed a good idea)?
   
   I investigated the performance issues with the previous version of the
   patch and found that turning some of the FlexLock support functions
   into macros seems to help quite a bit, so I've done that in the
   attached versions.  I've also incorporated Kevin's incremental patch
   from his previous version.
   
   That having been said, I'm leaning away from applying any of this for
   the time being.  For one thing, Pavan's PGXACT stuff has greatly
   eroded the benefit of this patch.  I'm fairly optimistic about the
   prospects of finding other good uses for the FlexLock machinery down
   the road, but I don't feel like that's enough reason to apply it now.
   Also, there are several other good ideas kicking around out there for
   further reducing ProcArrayLock contention, some of which are
   lower-impact than this and others of which would obsolete the entire
   approach.  So it seems like I should probably let the dust settle on
   those things before deciding whether this even makes sense.  In
   particular, I'm starting to think that resolving the contention
   between GetSnapshotData() and ProcArrayEndTransaction() is basically a
   layup at this point, and I really want to go for a three-pointer,
   namely also eliminating the spinlock contention between different
   backends all trying to acquire ProcArrayLock in shared mode during
   read-only operation.
   
   So, I'm going to mark this Returned with Feedback for now and keep
   working on the problem.  Thanks for the review and positive comments.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company