FlexLocks

Robert Haas <robertmhaas@gmail.com> wrote:

I'm not necessarily saying that any of these particular
things are what we want to do, just throwing out the idea that we
may want a variety of lock types that are similar to lightweight
locks but with subtly different behavior, yet with common
infrastructure for error handling and wait queue management.

The locking in the clog area is pretty funky. I bet we could craft
a special flavor of FlexLock to make that cleaner. And I would be
surprised if some creative thinking didn't yield a far better FL
scheme for SSI than we can manage with existing LW locks.

Your description makes sense to me, and your numbers prove the value
of the concept. Whether there's anything in the implementation I
would quibble about will take some review time.

-Kevin

On Tue, Nov 15, 2011 at 1:50 PM, Robert Haas <robertmhaas@gmail.com> wrote:

It basically
works like a regular LWLock, except that it has a special operation to
optimize ProcArrayEndTransaction().  In the uncontended case, instead
of acquiring and releasing the lock, it just grabs the lock, observes
that there is no contention, clears the critical PGPROC fields (which
isn't noticeably slower than updating the state of the lock would be)
and releases the spin lock.  There's then no need to reacquire the
spinlock to "release" the lock; we're done.  In the contended case,
the backend wishing to end adds itself to a queue of ending
transactions.  When ProcArrayLock is released, the last person out
clears the PGPROC structures for all the waiters and wakes them all
up; they don't need to reacquire the lock, because the work they
wished to perform while holding it is already done.  Thus, in the
*worst* case, ending transactions only need to acquire the spinlock
protecting ProcArrayLock half as often (once instead of twice), and in
the best case (where backends have to keep retrying only to repeatedly
fail to get the lock) it's far better than that.

Which is the same locking avoidance technique we already use for sync
rep and for the new group commit patch.

I've been saying for some time that we should use the same technique
for ProcArray and clog also, so we only need to queue once rather than
queue three times at end of each transaction.

I'm not really enthused by the idea of completely rewriting lwlocks
for this. Seems like specialised code is likely to be best, as well as
having less collateral damage.

With that in mind, should we try to fuse the group commit with the
procarraylock approach, so we just queue once and get woken when all
the activities have been handled? If the first woken proc performs the
actions then wakes people further down the queue it could work quite
well.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Nov 15, 2011 at 1:40 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

Which is the same locking avoidance technique we already use for sync
rep and for the new group commit patch.

Yep...

I've been saying for some time that we should use the same technique
for ProcArray and clog also, so we only need to queue once rather than
queue three times at end of each transaction.

I'm not really enthused by the idea of completely rewriting lwlocks
for this. Seems like specialised code is likely to be best, as well as
having less collateral damage.

Well, the problem that I have with that is that we're going to end up
with a lot of specialized code, particularly around error recovery.
This code doesn't remove the need for ProcArrayLock to be taken in
exclusive mode, and I don't think there IS any easy way to remove the
need for that to happen sometimes. So we have to deal with the
possibility that an ERROR might occur while we hold the lock, which
means we have to release the lock and clean up our state. That means
every place that has a call to LWLockReleaseAll() will now also need
to cleanup ProperlyCleanUpProcArrayLockStuff(). And the next time we
need to add some kind of specialized lock, we'll need to do the same
thing again. It seems to me that that rapidly gets unmanageable, not
to mention *slow*. We need some kind of common infrastructure for
releasing locks, and this is an attempt to create such a thing. I'm
not opposed to doing it some other way, but I think doing each one as
a one-off isn't going to work out very well.

Also, in this particular case, I really do want shared and exclusive
locks on ProcArrayLock to stick around; I just want one additional
operation as well. It's a lot less duplicated code to do that this
way than it is to write something from scratch. The FlexLock patch
may look big, but it's mostly renaming and rearranging; it's really
not adding much code.

With that in mind, should we try to fuse the group commit with the
procarraylock approach, so we just queue once and get woken when all
the activities have been handled? If the first woken proc performs the
actions then wakes people further down the queue it could work quite
well.

Well, there's too much work there to use the same approach I took
here: we can't very well hold onto the LWLock spinlock while flushing
WAL or waiting for synchronous replication. Fusing together some
parts of the commit sequence might be the right approach (I don't
know), but honestly my gut feeling is that the first thing we need to
do is go in the opposite direction and break up WALInsertLock into
multiple locks that allow better parallelization of WAL insertion. Of
course if someone finds a way to fuse the whole commit sequence
together in some way that improves performance, fantastic, but having
tried a lot of things before I came up with this approach, I'm a bit
reluctant to abandon it in favor of an approach that hasn't been coded
or tested yet. I think we should pursue this approach for now, and we
can always revise it later if someone comes up with something even
better. As a practical matter, the test results show that with these
patches, ProcArrayLock is NOT a bottleneck at 32 cores, which seems
like enough reason to be pretty happy with it, modulo implementation
details.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Robert Haas's message of mar nov 15 17:16:31 -0300 2011:

On Tue, Nov 15, 2011 at 1:40 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

Which is the same locking avoidance technique we already use for sync
rep and for the new group commit patch.

Yep...

I've been saying for some time that we should use the same technique
for ProcArray and clog also, so we only need to queue once rather than
queue three times at end of each transaction.

I'm not really enthused by the idea of completely rewriting lwlocks
for this. Seems like specialised code is likely to be best, as well as
having less collateral damage.

Well, the problem that I have with that is that we're going to end up
with a lot of specialized code, particularly around error recovery.
This code doesn't remove the need for ProcArrayLock to be taken in
exclusive mode, and I don't think there IS any easy way to remove the
need for that to happen sometimes. So we have to deal with the
possibility that an ERROR might occur while we hold the lock, which
means we have to release the lock and clean up our state. That means
every place that has a call to LWLockReleaseAll() will now also need
to cleanup ProperlyCleanUpProcArrayLockStuff(). And the next time we
need to add some kind of specialized lock, we'll need to do the same
thing again. It seems to me that that rapidly gets unmanageable, not
to mention *slow*. We need some kind of common infrastructure for
releasing locks, and this is an attempt to create such a thing. I'm
not opposed to doing it some other way, but I think doing each one as
a one-off isn't going to work out very well.

I agree. In fact, I would think that we should look into rewriting the
sync rep locking (and group commit) on top of flexlocks, not the other
way around. As Kevin says nearby it's likely that we could find some
way to rewrite the SLRU (clog and such) locking protocol using these new
things too.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> wrote:

As Kevin says nearby it's likely that we could find some way to
rewrite the SLRU (clog and such) locking protocol using these new
things too.

Yeah, I really meant all SLRU, not just clog. And having seen what
Robert has done here, I'm kinda glad I haven't gotten around to
trying to reduce LW lock contention yet, even though we're getting
dangerously far into the release cycle -- I think it can be done
much better with the, er, flexibility offered by the FlexLock patch.

-Kevin

On Tue, Nov 15, 2011 at 3:47 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:

Alvaro Herrera <alvherre@commandprompt.com> wrote:

As Kevin says nearby it's likely that we could find some way to
rewrite the SLRU (clog and such) locking protocol using these new
things too.

Yeah, I really meant all SLRU, not just clog.  And having seen what
Robert has done here, I'm kinda glad I haven't gotten around to
trying to reduce LW lock contention yet, even though we're getting
dangerously far into the release cycle -- I think it can be done
much better with the, er, flexibility offered by the FlexLock patch.

I've had a thought that the SLRU machinery could benefit from having
the concept of a "pin", which it currently doesn't. I'm not certain
whether that thought is correct.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Nov 15, 2011 at 10:55:49AM -0600, Kevin Grittner wrote:

And I would be
surprised if some creative thinking didn't yield a far better FL
scheme for SSI than we can manage with existing LW locks.

One place I could see it being useful is for
SerializableFinishedListLock, which protects the queue of committed
sxacts that can't yet be cleaned up. When committing a transaction, it
gets added to the list, and then scans the queue to find and clean up
any sxacts that are no longer needed. If there's contention, we don't
need multiple backends doing that scan; it's enough for one to complete
it on everybody's behalf.

I haven't thought it through, but it may also help with the other
contention bottleneck on that lock: that every transaction needs to add
itself to the cleanup list when it commits.

Mostly unrelatedly, the other thing that's looking like it would be really
useful would be some support for atomic integer operations. This would
be useful for some SSI things like writableSxactCount, and some things
elsewhere like the strong lock count in the regular lock manager.
I've been toying with the idea of creating an AtomicInteger type with a
few operations like increment-and-get, compare-and-set, swap, etc. This
would be implemented using the appropriate hardware operations on
platforms that support them (x86_64, perhaps others) and fall back on a
spinlock implementation on other platforms. I'll probably give it a try
and see what it looks like, but if anyone has any thoughts, let me know.

Dan

--
Dan R. K. Ports MIT CSAIL http://drkp.net/

On Tue, Nov 15, 2011 at 8:33 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

I'm not really enthused by the idea of completely rewriting lwlocks
for this. Seems like specialised code is likely to be best, as well as
having less collateral damage.

Well, the problem that I have with that is that we're going to end up
with a lot of specialized code, particularly around error recovery.
This code doesn't remove the need for ProcArrayLock to be taken in
exclusive mode, and I don't think there IS any easy way to remove the
need for that to happen sometimes.  So we have to deal with the
possibility that an ERROR might occur while we hold the lock, which
means we have to release the lock and clean up our state.  That means
every place that has a call to LWLockReleaseAll() will now also need
to cleanup ProperlyCleanUpProcArrayLockStuff().  And the next time we
need to add some kind of specialized lock, we'll need to do the same
thing again.  It seems to me that that rapidly gets unmanageable, not
to mention *slow*.  We need some kind of common infrastructure for
releasing locks, and this is an attempt to create such a thing.  I'm
not opposed to doing it some other way, but I think doing each one as
a one-off isn't going to work out very well.

I agree.  In fact, I would think that we should look into rewriting the
sync rep locking (and group commit) on top of flexlocks, not the other
way around.  As Kevin says nearby it's likely that we could find some
way to rewrite the SLRU (clog and such) locking protocol using these new
things too.

I see the commonality between ProcArray locking and Sync Rep/ Group
Commit locking. It's basically the same design, so although it wasn't
my first thought, I agree.

I did originally write that using spinlocks, but that was objected to.
Presumably the same objection would hold here also, but if it doesn't
that's good.

Mixing the above 3 things together is enough for me; I just don't see
the reason to do a global search and replace on the lwlock name in
order to do that. This is 2 patches at same time, 1 we clearly need, 1
I'm not sure about. Perhaps some more explanation about the flexlocks
structure and design will smooth that unease.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon@2ndQuadrant.com> wrote:

I just don't see the reason to do a global search and replace on
the lwlock name

I was going to review further before commenting on that, but since
it has now come up -- it seems odd that a source file which uses
only LW locks needs to change so much for the FlexLock
implementation. I'm not sure source code which uses the next layer
up from FlexLock should need to be aware of it quite so much. I
assume that this was done to avoid adding a layer to some code where
it could cause an unnecessary performance hit, but maybe it would be
worth just wrapping with a macro to isolate the levels where that's
all it takes.

For example, if these two macros were defined, predicate.c wouldn't
have needed any modifications, and I suspect that is true of many
other files (although possibly needing a few other macros):

#define LWLockId FlexLockId
#define LWLockHeldByMe(lock) FlexLockHeldByMe(lock)

Particularly with the function call it seems like it's a mistake to
assume that test will always be the same between LW locks and flex
locks. There may be a better way to do it than the above, but I
think a worthy goal would be to impose zero source code changes on
code which continues to use "traditional" lightweight locks.

-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Simon Riggs <simon@2ndQuadrant.com> wrote:

I just don't see the reason to do a global search and replace on
the lwlock name

I was going to review further before commenting on that, but since
it has now come up -- it seems odd that a source file which uses
only LW locks needs to change so much for the FlexLock
implementation.

Yeah, -1 on wideranging source changes for me too. There is no reason
that the current LWLock API need change. (I'm not saying that it has
to be same ABI though --- macro wrappers would be fine.)

regards, tom lane

On Wed, Nov 16, 2011 at 10:26 AM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:

For example, if these two macros were defined, predicate.c wouldn't
have needed any modifications, and I suspect that is true of many
other files (although possibly needing a few other macros):

#define LWLockId FlexLockId
#define LWLockHeldByMe(lock) FlexLockHeldByMe(lock)

Particularly with the function call it seems like it's a mistake to
assume that test will always be the same between LW locks and flex
locks.  There may be a better way to do it than the above, but I
think a worthy goal would be to impose zero source code changes on
code which continues to use "traditional" lightweight locks.

Well, it would certainly be easy enough to add those macros, and I'm
not necessarily opposed to it, but I fear it could end up being a bit
confusing in the long run. If we adopt this infrastructure, then I
expect knowledge of different types of FlexLocks to gradually
propagate through the system. Now, you're always going to use
LWLockAcquire() and LWLockRelease() to acquire and release an LWLock,
but a FlexLockId isn't guaranteed to be an LWLockId - any given value
might also refer to a FlexLock of some other type. If we let everyone
continue to refer to those things as LWLockIds, then it seems like
only a matter of time before someone has a variable that's declared as
LWLockId but actually doesn't refer to an LWLock at all. I think it's
better to bite the bullet and do the renaming up front, rather than
having to think about it every time you modify some code that uses
LWLockId or LWLockHeldByMe and say to yourself, "oh, wait a minute, is
this really guaranteed to be an LWLock?"

For LWLockHeldByMe, a sensible compromise might be to add a function
that asserts that the FlexLockId passed as an argument is in fact
pointing to an LWLock, and then calls FlexLockHeldByMe() and returns
the result. That way you'd presumably noticed if you used the more
specific function when you needed the more general one (because,
hopefully, the regression tests would fail). But I'm not seeing any
obvious way of providing a similar degree of insulation against
abusing LWLockId.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

Well, it would certainly be easy enough to add those macros, and I'm
not necessarily opposed to it, but I fear it could end up being a bit
confusing in the long run. If we adopt this infrastructure, then I
expect knowledge of different types of FlexLocks to gradually
propagate through the system. Now, you're always going to use
LWLockAcquire() and LWLockRelease() to acquire and release an LWLock,
but a FlexLockId isn't guaranteed to be an LWLockId - any given value
might also refer to a FlexLock of some other type. If we let everyone
continue to refer to those things as LWLockIds, then it seems like
only a matter of time before someone has a variable that's declared as
LWLockId but actually doesn't refer to an LWLock at all. I think it's
better to bite the bullet and do the renaming up front, rather than
having to think about it every time you modify some code that uses
LWLockId or LWLockHeldByMe and say to yourself, "oh, wait a minute, is
this really guaranteed to be an LWLock?"

In that case, I think you've chosen an unfortunate naming convention
and should rethink it. There is not any benefit to be gained from a
global search and replace here, and as somebody who spends quite enough
time dealing with cross-branch coding differences already, I'm going to
put my foot down about introducing a useless one.

Perhaps it would be better to think of this as "they're all lightweight
locks, but some have different locking policies". Or "we're taking a
different type of lock on this particular lock" --- that would match up
rather better with the way we think about heavyweight locks.

regards, tom lane

On Wed, Nov 16, 2011 at 10:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Well, it would certainly be easy enough to add those macros, and I'm
not necessarily opposed to it, but I fear it could end up being a bit
confusing in the long run.  If we adopt this infrastructure, then I
expect knowledge of different types of FlexLocks to gradually
propagate through the system.  Now, you're always going to use
LWLockAcquire() and LWLockRelease() to acquire and release an LWLock,
but a FlexLockId isn't guaranteed to be an LWLockId - any given value
might also refer to a FlexLock of some other type.  If we let everyone
continue to refer to those things as LWLockIds, then it seems like
only a matter of time before someone has a variable that's declared as
LWLockId but actually doesn't refer to an LWLock at all.  I think it's
better to bite the bullet and do the renaming up front, rather than
having to think about it every time you modify some code that uses
LWLockId or LWLockHeldByMe and say to yourself, "oh, wait a minute, is
this really guaranteed to be an LWLock?"

In that case, I think you've chosen an unfortunate naming convention
and should rethink it.  There is not any benefit to be gained from a
global search and replace here, and as somebody who spends quite enough
time dealing with cross-branch coding differences already, I'm going to
put my foot down about introducing a useless one.

Perhaps it would be better to think of this as "they're all lightweight
locks, but some have different locking policies".  Or "we're taking a
different type of lock on this particular lock" --- that would match up
rather better with the way we think about heavyweight locks.

I struggled a lot with how to name this, and I'm not going to pretend
that what I came up with is necessarily ideal. But the basic idea
here is that all FlexLocks share the following properties in common:

- they are identified by a FlexLockId
- they are released by FlexLockReleaseAll
- they make use of the lwlock-related fields (renamed in the patch) in
PGPROC for sleep and wakeup handling
- they have a type indicator, a mutex, a retry flag, and a wait queue

But the following things are different per-type:

- acquire, conditional acquire (if any), and release functions
- available lock modes
- additional data fields that are part of the lock

Now, it seemed to me that if I was going to split the LWLock facililty
into two layers, either the upper layer could be LWLocks, or the lower
layer could be LWLocks, but they couldn't both be LWLocks. Since we
use LWLockAcquire() and LWLockRelease() all over the place but only
make reference to LWLockId in comparatively few places, it seemed to
me to be by far the less invasive renaming to make the upper layer be
LWLocks and the lower layer be something else.

Now maybe there is some better way to do this, but at the moment, I'm
not seeing it. If we call them all LWLocks, but only some of them
support LWLockAcquire(), then that's going to be pretty weird. The
situation is not really analagous to heavy-weight locks, where every
lock supports every lock mode, but in practice only table locks make
use of them all. In this particular case, we do not want to clutter
up the vanilla LWLock implementation with a series of special cases
that are only useful for a minority of locks in the system. That will
cause them to stop being lightweight, which misses the point; and it
will be ugly as hell, because the exact frammishes needed will
doubtless vary from one lock to another, and having just one lock type
that supports every single one of those frammishes is certainly a bad
idea.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 16, 2011 at 3:41 PM, Robert Haas <robertmhaas@gmail.com> wrote:

 Now, you're always going to use
LWLockAcquire() and LWLockRelease() to acquire and release an LWLock,
but a FlexLockId isn't guaranteed to be an LWLockId - any given value
might also refer to a FlexLock of some other type.  If we let everyone
continue to refer to those things as LWLockIds, then it seems like
only a matter of time before someone has a variable that's declared as
LWLockId but actually doesn't refer to an LWLock at all.  I think it's
better to bite the bullet and do the renaming up front, rather than
having to think about it every time you modify some code that uses
LWLockId or LWLockHeldByMe and say to yourself, "oh, wait a minute, is
this really guaranteed to be an LWLock?"

But that's an advantage to having a distinct API layer for LWLock
instead of having callers directly call FlexLock methods. The LWLock
macros can AssertMacro that the lockid they were passed are actually
LWLocks and not some other type of lock. That would require assigning
FlexLockIds that are recognizably LWLocks but that's not implausible
is it?

--
greg

Robert Haas <robertmhaas@gmail.com> wrote:

Now maybe there is some better way to do this, but at the moment,
I'm not seeing it. If we call them all LWLocks, but only some of
them support LWLockAcquire(), then that's going to be pretty
weird.

Is there any way to typedef our way out of it, such that a LWLock
*is a* FlexLock, but a FlexLock isn't a LWLock? If we could do
that, you couldn't use just a plain old FlexLock in LWLockAcquire(),
but you could do the cleanups, etc., that you want.

-Kevin

On Wed, Nov 16, 2011 at 11:14 AM, Greg Stark <stark@mit.edu> wrote:

On Wed, Nov 16, 2011 at 3:41 PM, Robert Haas <robertmhaas@gmail.com> wrote:

 Now, you're always going to use
LWLockAcquire() and LWLockRelease() to acquire and release an LWLock,
but a FlexLockId isn't guaranteed to be an LWLockId - any given value
might also refer to a FlexLock of some other type.  If we let everyone
continue to refer to those things as LWLockIds, then it seems like
only a matter of time before someone has a variable that's declared as
LWLockId but actually doesn't refer to an LWLock at all.  I think it's
better to bite the bullet and do the renaming up front, rather than
having to think about it every time you modify some code that uses
LWLockId or LWLockHeldByMe and say to yourself, "oh, wait a minute, is
this really guaranteed to be an LWLock?"

But that's an advantage to having a distinct API layer for LWLock
instead of having callers directly call FlexLock methods. The LWLock
macros can AssertMacro that the lockid they were passed are actually
LWLocks and not some other type of lock. That would require assigning
FlexLockIds that are recognizably LWLocks but that's not implausible
is it?

Well, that works for the most part. You still need a few generic
functions, like FlexLockReleaseAll(), which releases all FlexLocks of
all types, not just those of some particular type. And it doesn't
solve the problem with FlexLockId, which can potentially refer to a
FlexLock of any type, not just a LWLock.

I think we might be getting slightly more excited about this problem
than it actually deserves. Excluding lwlock.{c,h}, the new files
added by this patch, and the documentation changes, this patch adds
103 lines and removes 101. We can uncontroversially reduce each
numbers by 14 by adding a separate LWLockHeldByMe() function that does
the same thing as FlexLockHeldByMe() but also asserts the lock type.
That would leave us adding 89 lines of code and removing 87.

If we (against my better judgement) take the position that we must
continue to use LWLockId rather than FlexLockId as the type name in
any place that only treats with LWLocks we could reduce each of those
numbers by an additional 34, giving new totals of 55 and 53 lines of
changes outside the lwlock/flexlock code itself rather than 89 and 87.
I humbly submit that this is not really enough to get excited about.
We've make far more sweeping notational changes than that more than
once - even, I think, with some regularity.

This may seem invasive because it's touching LWLocks, and we use those
everywhere, but in practice the code footprint is very small because
typical usage is just LWLockAcquire(BlahLock) and then
LWLockRelease(BlahLock). And I'm not proposing to change that usage
in any way; avoiding any change in that area was, in fact, one of my
main design goals.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 16, 2011 at 11:17 AM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:

Robert Haas <robertmhaas@gmail.com> wrote:

Now maybe there is some better way to do this, but at the moment,
I'm not seeing it.  If we call them all LWLocks, but only some of
them support LWLockAcquire(), then that's going to be pretty
weird.

Is there any way to typedef our way out of it, such that a LWLock
*is a* FlexLock, but a FlexLock isn't a LWLock?  If we could do
that, you couldn't use just a plain old FlexLock in LWLockAcquire(),
but you could do the cleanups, etc., that you want.

Well, if we just say:

typedef FlexLockId LWLockId;

...that's about equivalent to the #define from the compiler's point of
view. We could alternatively change one or the other of them to be a
struct with one member, but I think the cure might be worse than the
disease. By my count, we are talking about saving perhaps as many as
34 lines of code changes here, and that's only if complicating the
type handling doesn't require any changes to places that are untouched
at present, which I suspect it would.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> wrote:

Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote:

Is there any way to typedef our way out of it [?]

Well, if we just say:

typedef FlexLockId LWLockId;

...that's about equivalent to the #define from the compiler's
point of view.

Bummer -- I was hoping there was some equivalent to "subclassing"
that I just didn't know about. :-(

We could alternatively change one or the other of them to be a
struct with one member, but I think the cure might be worse than
the disease. By my count, we are talking about saving perhaps as
many as 34 lines of code changes here, and that's only if
complicating the type handling doesn't require any changes to
places that are untouched at present, which I suspect it would.

So I stepped through all the changes of this type, and I notice that
most of them are in areas where we've talked about likely benefits
of creating new FlexLock variants instead of staying with LWLocks;
if any of that is done (as seems likely), it further reduces the
impact from 34 lines. If we take care of LWLockHeldByMe() as you
describe, I'll concede the FlexLockId changes.

-Kevin

On Wed, Nov 16, 2011 at 12:25 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:

We could alternatively change one or the other of them to be a
struct with one member, but I think the cure might be worse than
the disease.  By my count, we are talking about saving perhaps as
many as 34 lines of code changes here, and that's only if
complicating the type handling doesn't require any changes to
places that are untouched at present, which I suspect it would.

So I stepped through all the changes of this type, and I notice that
most of them are in areas where we've talked about likely benefits
of creating new FlexLock variants instead of staying with LWLocks;
if any of that is done (as seems likely), it further reduces the
impact from 34 lines.  If we take care of LWLockHeldByMe() as you
describe, I'll concede the FlexLockId changes.

Updated patches attached.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company