On Thu, Aug 14, 2014 at 2:21 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> 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
Ah, OK. If there's a separate slot for each backend, I agree that it's safe.
We should probably add barriers to pgstat.c, too.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
