Re: Reduce ProcArrayLock contention

On Tue, Aug 4, 2015 at 8:59 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Aug 3, 2015 at 8:39 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> >> I agree and modified the patch to use 32-bit atomics based on idea
> >> suggested by Robert and didn't modify lwlock.c.
> >
> > While looking at patch, I found that the way it was initialising the
list
> > to be empty was wrong, it was using pgprocno as 0 to initialise the
> > list, as 0 is a valid pgprocno. I think we should use a number greater
> > that PROCARRAY_MAXPROC (maximum number of procs in proc
> > array).
> >
> > Apart from above fix, I have modified src/backend/access/transam/README
> > to include the information about the improvement this patch brings to
> > reduce ProcArrayLock contention.
>
> I spent some time looking at this patch today and found that a good
> deal of cleanup work seemed to be needed. Attached is a cleaned-up
> version which makes a number of changes:
>
> 1. I got rid of all of the typecasts. You're supposed to treat
> pg_atomic_u32 as a magic data type that is only manipulated via the
> primitives provided, not just cast back and forth between that and
> u32.
>
> 2. I got rid of the memory barriers. System calls are full barriers,
> and so are compare-and-exchange operations. Between those two facts,
> we should be fine without these.
>

I have kept barriers based on comments on top of atomic read, refer
below code:

* No barrier semantics.
*/
STATIC_IF_INLINE uint32
pg_atomic_read_u32(volatile pg_atomic_uint32 *ptr)

Note - The function header comments on pg_atomic_read_u32 and
corresponding write call seems to be reversed, but that is something
separate.

>
> I'm not entirely happy with the name "nextClearXidElem" but apart from
> that I'm fairly happy with this version. We should probably test it
> to make sure I haven't broken anything;

Okay, will look into it tomorrow.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com