On 2014-10-09 16:52:46 -0500, Jim Nasby wrote: > On 10/8/14, 8:35 AM, Andres Freund wrote: > >+#define EXCLUSIVE_LOCK (((uint32) 1) << (31 - 1)) > >+ > >+/* Must be greater than MAX_BACKENDS - which is 2^23-1, so we're fine. */ > >+#define SHARED_LOCK_MASK (~EXCLUSIVE_LOCK) > > There should at least be a comment where we define MAX_BACKENDS about the > relationship here... or better yet, validate that MAX_BACKENDS > > SHARED_LOCK_MASK during postmaster startup. (For those that think that's too > pedantic, I'll argue that it's no worse than the patch verifying that MyProc > != NULL in LWLockQueueSelf()).
If you modify either, you better grep for them... I don't think that's going to happen anyway. Requiring it during startup would mean exposing SHARED_LOCK_MASK outside of lwlock.c which'd be ugly. We could possibly stick a StaticAssert() someplace in lwlock.c. And no, it's not comparable at all to MyProc != NULL - the lwlock code initially *does* run when MyProc isn't setup. We just better not conflict against any other lockers at that stage. > >+/* > >+ * Internal function that tries to atomically acquire the lwlock in the > >passed > >+ * in mode. > >+ * > >+ * This function will not block waiting for a lock to become free - that's > >the > >+ * callers job. > >+ * > >+ * Returns true if the lock isn't free and we need to wait. > >+ * > >+ * When acquiring shared locks it's possible that we disturb an exclusive > >+ * waiter. If that's a problem for the specific user, pass in a valid > >pointer > >+ * for 'potentially_spurious'. Its value will be set to true if we possibly > >+ * did so. The caller then has to handle that scenario. > >+ */ > >+static bool > >+LWLockAttemptLock(LWLock* lock, LWLockMode mode, bool *potentially_spurious) > > We should invert the return of this function. Current code returns > true if the lock is actually acquired (see below), and I think that's > true of other locking code as well. IMHO it makes more sense that way, > plus consistency is good. I don't think so. I've wondered about it as well, but the way the function is used its more consistent imo if it returns whether we must wait. Note that it's not an exported function. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers