Re: Wait free LW_SHARED acquisition - v0.9

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()).

> +/*
> + * 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.

(From 9.3)
* LWLockConditionalAcquire - acquire a lightweight lock in the specified mode
*
* If the lock is not available, return FALSE with no side-effects.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com