Re: FlexLocks

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