On 8/27/14, 7:33 AM, Fujii Masao wrote:
On Tue, Aug 19, 2014 at 1:07 AM, Robert Haas <robertmh...@gmail.com> wrote:
On Fri, Aug 15, 2014 at 7:17 AM, Fujii Masao <masao.fu...@gmail.com> wrote:
On Fri, Aug 15, 2014 at 3:40 AM, Andres Freund <and...@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 <and...@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.

That looks OK to me on a relatively-quick read-through.  I was
initially a bit worried about this part:

       do
       {
!         pgstat_increment_changecount_before(beentry);
       } while ((beentry->st_changecount & 1) == 0);

pgstat_increment_changecount_before is an increment followed by a
write barrier.  This seemed like funny coding to me at first because
while-test isn't protected by any sort of barrier.  But now I think
it's correct, because there's only one process that can possibly write
to that data, and that's the one that is making the test, and it had
certainly better see its own modifications in program order no matter
what.

I wouldn't object to back-patching this to 9.4 if we were earlier in
the beta cycle, but at this point I'm more inclined to just put it in
9.5.  If we get an actual bug report about any of this, we can always
back-patch the fix at that time.  But so far that seems mostly
hypothetical, so I think the less-risky course of action is to give
this a longer time to bake before it hits an official release.

Sounds reasonable. So, barring any objection, I will apply the patch
only to the master branch.

It's probably worth adding a comment explaining why it's safe to do this 
without a barrier...
--
Jim C. Nasby, Data Architect                       j...@nasby.net
512.569.9461 (cell)                         http://jim.nasby.net


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to