On Wed, Nov 16, 2011 at 11:14 AM, Greg Stark <st...@mit.edu> wrote: > On Wed, Nov 16, 2011 at 3:41 PM, Robert Haas <robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers