Re: LWLocks in DSM memory - Mailing list pgsql-hackers

On Thu, Jul 28, 2016 at 6:51 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> That version didn't actually make LWLock any smaller, because of the
> additional offset stored in proclist_head when initialising the list.
> Here is a new version that requires callers to provide it when
> accessing the list, bringing sizeof(LWLock) down to 16 on LP64/LLP64
> systems.  In the spirit of the dlist_container macro, I added macros
> so you can name the link member rather than providing its offset:
> proclist_push_tail(&list, procno, lwWaitLink).

I have reviewed this patch.  I like the design and overall I would say
that the patch looks quite elegant.  The only problem I found is that
proclist_types.h doesn't seem to need to #include <stddef.h>.  I tried
taking that out, and, at least on my machine, it still compiles just
fine.

Of course, performance is a concern, as with any change that touches
deep internals.  Andres and Thomas seem to be in agreement that this
patch should cause a performance loss but that it should be
undetectable in practice, since only the slow path where we're
entering the kernel anyway is affected.  I agree with that conclusion.
I spent a little bit of time trying to think of a case where this
would have a measurable impact, and I'm not coming up with anything.
We've done a lot of work over the last few releases to try to
eliminate LWLock contention, and the only way you could even
theoretically see an impact from this is if you can come up with a
workload with furious LWLock contention so that there is lots and lots
of linked-list manipulation going on inside lwlock.c.  But I don't
really know of a workload that behaves that way, and even if I did, I
think the number of cycles we're talking about here is too small to be
measurable.  Having to put processes to sleep is going to be multiple
orders of magnitude more costly than any possible details of how we
handle the linked list manipulation.

I also agree with the goal of the patch: the reason why I developed
the idea of LWLock tranches was with the goal of being able to put
LWLocks inside DSM segments, and that worked fine up until Andres
changed lwlock.c to use dlists.  This change will get us back to being
able to use LWLocks inside of DSM segments.  Of course, we have no
code like that today; if we did, it would be broken.  But Thomas will
be submitting code that does exactly that in the near future, and I
suspect other people will want to do it, too. As a fringe benefit, I
have another patch I'm working on that can make use of this proclist
infrastructure.

Therefore, I plan to commit this patch, removing the #include
<stddef.h> unless someone convinces me we need it, shortly after
development for v10 opens, unless there are objections before then.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company