Обсуждение: atomic reads & writes (with no barriers)

The "snapshot too old" patch has an XXX comment about being able to
drop a spinlock usage from a frequently called "get" method if the
64-bit read could be trusted to be atomic.  There is no need for a
barrier in this case, because a stale value just means we won't be
quite as aggressive about pruning tuples still within the global
xmin boundary (potentially leading to the "snapshot too old"
error), normally by a small fraction of a second.  Removing
contention on spinlocks is a good thing.  Andres commented here:

http://www.postgresql.org/message-id/20151203154230.GE20698@awork2.anarazel.de

| We currently don't assume it's atomic. And there are platforms,
| e.g 32 bit arm, where that's not the case (c.f.
| https://wiki.postgresql.org/wiki/Atomics ). It'd be rather useful
| to abstract that knowledge into a macro...

I did some web searches and then opened a question on Stack
Overflow.  Not surprisingly I got a mix of useful answers, and
those not quite so much.  Based on the most useful comment, I got
something that isn't too awfully pretty, but it seems to work on my
Ubuntu machine.  Maybe it can be the start of something useful.

Before C11 the C standard specifically disclaimed any atomic
access.  C11 adds what seems to me to be a fairly nice mechanism
for supporting it with the _Atomic modifier on a declaration.
Based on a suggestion from Ian Abbott I added this:

diff --git a/src/include/c.h b/src/include/c.h
index 8163b00..105c542 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -288,6 +288,11 @@ typedef unsigned long long int uint64;#error must have a working 64-bit integer datatype#endif

+#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L) &&
!defined(__STDC_NO_ATOMICS__)
+#define HAVE_ATOMICINT64
+typedef _Atomic int64 atomicint64;
+#endif
+/* Decide if we need to decorate 64-bit constants */#ifdef HAVE_LL_CONSTANTS#define INT64CONST(x)  ((int64) x##LL)

To use it in my patch, I made these changes to the patched code:

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 13c58d2..1b891b5 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -81,7 +81,11 @@ typedef struct OldSnapshotControlData   slock_t     mutex_current;          /* protect current
timestamp*/   int64       current_timestamp;      /* latest snapshot timestamp */   slock_t     mutex_threshold;
/*protect threshold fields */
 
+#ifdef HAVE_ATOMICINT64
+   atomicint64 threshold_timestamp;    /* earlier snapshot is old */
+#else   int64       threshold_timestamp;    /* earlier snapshot is old */
+#endif   TransactionId threshold_xid;        /* earlier xid may be gone */
   /*
@@ -1553,6 +1557,9 @@ GetSnapshotCurrentTimestamp(void)int64GetOldSnapshotThresholdTimestamp(void){
+#ifdef HAVE_ATOMICINT64
+   return oldSnapshotControl->threshold_timestamp;
+#else   int64       threshold_timestamp;
   SpinLockAcquire(&oldSnapshotControl->mutex_threshold);
@@ -1560,6 +1567,7 @@ GetOldSnapshotThresholdTimestamp(void)   SpinLockRelease(&oldSnapshotControl->mutex_threshold);
   return threshold_timestamp;
+#endif}
/*

I'm sure this could be improved upon, and we would want to expand
it to uint64; but it seems to work, and I think it should do no
harm on non-C11 compilers.  (I'm less sure about 32-bit builds, but
if there's a problem there, it should be fixable.)  We may be able
to make use of non-standard features of other (non-C11) compilers,
but I'm not aware of what options there are.

Is the c.h change above on anything resembling the right track for
a patch for this?  If not, what would such a patch look like?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Dec 3, 2015 at 10:10 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
> Is the c.h change above on anything resembling the right track for
> a patch for this?  If not, what would such a patch look like?

It would be nicer if we could come up with an interface that didn't
require #ifdefs everywhere it's used.

Something like
... pg_maybe_atomic int64 threshold_timestamp;
...

SpinLockAcquire_if_no_atomics(...)
threshold_timestamp = &oldSnapshotControl->threshold_timestamp;
SpinLockRelease_if_no_atomics(...)

return threshold_timestamp;

-- 
greg

On Fri, Dec 4, 2015 at 6:35 AM, Greg Stark <stark@mit.edu> wrote:
> On Thu, Dec 3, 2015 at 10:10 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
>> Is the c.h change above on anything resembling the right track for
>> a patch for this?  If not, what would such a patch look like?
>
> It would be nicer if we could come up with an interface that didn't
> require #ifdefs everywhere it's used.
>
> Something like
> ...
>   pg_maybe_atomic int64 threshold_timestamp;
> ...
>
> SpinLockAcquire_if_no_atomics(...)
> threshold_timestamp = &oldSnapshotControl->threshold_timestamp;
> SpinLockRelease_if_no_atomics(...)
>
> return threshold_timestamp;

Yeah, I didn't much like including the #ifdefs everywhere; I like
your suggestions.  Will work up a patch for the next CF along those
lines.

Thanks!

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2015-12-03 16:10:51 -0600, Kevin Grittner wrote:
> Is the c.h change above on anything resembling the right track for
> a patch for this?  If not, what would such a patch look like?

I think a better path would be to add fallback support for 64bit atomics
- like we already have for 32bit. That should really only take a few
lines. Then you can use pg_atomic_read_u64/pg_atomic_write_u64 and all
the rest of the atomics api.

For that to be helpful we'd want a more efficient read/write
implementation for some platforms (falls back to compare/exchange right
now), but that's easy.

Andres