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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
