On Fri, Aug 15, 2014 at 8:17 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Fri, Aug 15, 2014 at 3:40 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> On 2014-08-14 14:37:22 -0400, Robert Haas wrote:
>>> 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:
>>> >> 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.
>>
>> Yea, definitely. I think this is rather borked on "weaker"
>> architectures. It's just that the consequences of an out of date/torn
>> value are rather low, so it's unlikely to be noticed.
>>
>> Imo we should encapsulate the changecount modifications/checks somehow
>> instead of repeating the barriers, Asserts, comments et al everywhere.
>
> So what about applying the attached patch first, which adds the macros
> to load and store the changecount with the memory barries, and changes
> pgstat.c use them. Maybe this patch needs to be back-patch to at least 9.4?
>
> After applying the patch, I will rebase the pg_last_xact_insert_timestamp
> patch and post it again.
Hm, what's the status on this patch? The addition of those macros to
control count increment with a memory barrier seems like a good thing
at least. The 2nd patch has not been rebased but still..
--
Michael
