Re: FlexLocks

On Wed, Nov 16, 2011 at 11:14 AM, Greg Stark <stark(at)mit(dot)edu> wrote:
> On Wed, Nov 16, 2011 at 3:41 PM, Robert Haas <robertmhaas(at)gmail(dot)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