Re: FlexLocks

"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

On Wed, Nov 23, 2011 at 7:18 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)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

On Wed, Nov 30, 2011 at 7:01 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)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

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)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

On Fri, Dec 2, 2011 at 4:11 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)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