atomic reads & writes (with no barriers) - Mailing list pgsql-hackers

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