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.
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
