On 2014-08-14 14:19:13 -0400, Robert Haas wrote:
> On Thu, Aug 14, 2014 at 1:51 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> > On Fri, Aug 15, 2014 at 1:55 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> >> On Mon, Aug 11, 2014 at 3:20 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> >>> There is no extra spinlock.
> >>
> >> The version I reviewed had one; that's what I was objecting to.
> >
> > Sorry for confusing you. I posted the latest patch to other thread.
> > This version doesn't use any spinlock.
> >
> > http://www.postgresql.org/message-id/CAHGQGwEwuh5CC3ib6wd0fxs9LAWme=kO09S4MOXnYnAfn7N5Bg@mail.gmail.com
> >
> >> Might need to add some pg_read_barrier() and pg_write_barrier() calls,
> >> since we have those now.
> >
> > Yep, memory barries might be needed as follows.
> >
> > * Set the commit timestamp to shared memory.
> >
> > shmem->counter++;
> > pg_write_barrier();
> > shmem->timestamp = my_timestamp;
> > pg_write_barrier();
> > shmem->count++;
> >
> > * Read the commit timestamp from shared memory
> >
> > my_count = shmem->counter;
> > pg_read_barrier();
> > my_timestamp = shmem->timestamp;
> > pg_read_barrier();
> > my_count = shmem->counter;
> >
> > Is this way to use memory barriers right?
>
> That's about the idea. However, what you've got there is actually
> unsafe, because shmem->counter++ is not an atomic operation. It reads
> the counter (possibly even as two separate 4-byte loads if the counter
> is an 8-byte value), increments it inside the CPU, and then writes the
> resulting value back to memory. If two backends do this concurrently,
> one of the updates might be lost.
All these are only written by one backend, so it should be safe. Note
that that coding pattern, just without memory barriers, is all over
pgstat.c
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
