On Tue, Aug 4, 2015 at 8:59 PM, Robert Haas <
robertmhaas@gmail.com> wrote:
>
> On Mon, Aug 3, 2015 at 8:39 AM, Amit Kapila <
amit.kapila16@gmail.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.
