Re: Wait free LW_SHARED acquisition - v0.9

On 10/9/14, 4:57 PM, Andres Freund wrote:
> 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.

Ahh, yeah, exposing it would be ugly.

I just get the heeby-jeebies when I see assumptions like this though. I fear there's a bunch of cases where changing
somethingwill break a completely unrelated part of the system with no warning.
 

Maybe add an assert() to check it?

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

Ahh, can you maybe add that detail to the comment? That wasn't clear to me.

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

ISTM that a function attempting a lock would return success, not failure. Even though it's internal now it could
certainlybe made external at some point in the future. But I suppose it's ultimately a matter of preference...
 
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com