Re: Group commit, revised

On 26.01.2012 04:10, Robert Haas wrote:
> On Wed, Jan 25, 2012 at 3:11 AM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> Attached is a patch to do that. It adds a new mode to
>> LWLockConditionalAcquire(), LW_EXCLUSIVE_BUT_WAIT. If the lock is free, it
>> is acquired and the function returns immediately. However, unlike normal
>> LWLockConditionalAcquire(), if the lock is not free it goes to sleep until
>> it is released. But unlike normal LWLockAcquire(), when the lock is
>> released, the function returns *without* acquiring the lock.
>> ...
>
> I think you should break this off into a new function,
> LWLockWaitUntilFree(), rather than treating it as a new LWLockMode.
> Also, instead of adding lwWaitOnly, I would suggest that we generalize
> lwWaiting and lwExclusive into a field lwWaitRequest, which can be set
> to 1 for exclusive, 2 for shared, 3 for wait-for-free, etc. That way
> we don't have to add another boolean every time someone invents a new
> type of wait - not that that should hopefully happen very often. A
> side benefit of this is that you can simplify the test in
> LWLockRelease(): keep releasing waiters until you come to an exclusive
> waiter, then stop. This possibly saves one shared memory fetch and
> subsequent test instruction, which might not be trivial - all of this
> code is extremely hot.

Makes sense. Attached is a new version, doing exactly that.

> We probably want to benchmark this carefully
> to make sure that it doesn't cause a performance regression even just
> from this:
>
> + if (!proc->lwWaitOnly)
> + lock->releaseOK = false;
>
> I know it sounds crazy, but I'm not 100% sure that that additional
> test there is cheap enough not to matter. Assuming it is, though, I
> kind of like the concept: I think there must be other places where
> these semantics are useful.

Yeah, we have to be careful with any overhead in there, it can be a hot
spot. I wouldn't expect any measurable difference from the above,
though. Could I ask you to rerun the pgbench tests you did recently with
this patch? Or can you think of a better test for this?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com