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
thatMAX_BACKENDS > SHARED_LOCK_MASK during postmaster startup. (For those that think that's too pedantic, I'll argue
thatit'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),
andI 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