On Wed, Nov 16, 2011 at 10:51 AM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Robert Haas <robertmh...@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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers