Re: Wait free LW_SHARED acquisition - v0.9

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