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.ge20...@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) int64 GetOldSnapshotThresholdTimestamp(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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
