On Tue, Aug 4, 2015 at 8:59 PM, Robert Haas <robertmh...@gmail.com> wrote: > > On Mon, Aug 3, 2015 at 8:39 AM, Amit Kapila <amit.kapil...@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. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com