On Fri, Nov 28, 2014 at 9:07 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Wed, Nov 26, 2014 at 4:05 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> 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.
>
> Thanks for reminding me of that! Barring any objection, I will commit it.
Applied.
Regards,
--
Fujii Masao
