Michael Paquier <[email protected]> writes:
> Indeed, this was incorrect. And you may not have noticed, but we have
> a second instance of that in LogicalIncreaseRestartDecodingForSlot()
> that goes down to 9.4 and b89e151. I used a dirty-still-efficient
> hack to detect that, and that's the only instance I have spotted.
Ugh, that is just horrid. I experimented with the attached patch
but it did not find any other problems. Still, that only proves
something about code paths that are taken during check-world, and
we know that our test coverage is not very good :-(.
Should we think about adding automated detection of this type of
mistake? I don't like the attached as-is because of the #include
footprint expansion, but maybe we can find a better way.
> I am not sure if that's worth worrying a back-patch, but we should
> really address that at least on HEAD.
It's actually worse in the back branches, because elog() did not have
a good short-circuit path like ereport() does. +1 for back-patch.
regards, tom lane
diff --git a/src/backend/storage/lmgr/spin.c b/src/backend/storage/lmgr/spin.c
index 4d2a4c6641..f95f83d039 100644
--- a/src/backend/storage/lmgr/spin.c
+++ b/src/backend/storage/lmgr/spin.c
@@ -27,6 +27,8 @@
#include "storage/spin.h"
+volatile slock_t *held_spinlock = NULL;
+
#ifndef HAVE_SPINLOCKS
PGSemaphore *SpinlockSemaArray;
#endif
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 14fa127ab1..247d9a8089 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -28,6 +28,10 @@
#include "datatype/timestamp.h" /* for TimestampTz */
#include "pgtime.h" /* for pg_time_t */
+#ifndef FRONTEND
+#include "storage/spin.h"
+#endif
+
#define InvalidPid (-1)
@@ -98,6 +102,7 @@ extern void ProcessInterrupts(void);
#define CHECK_FOR_INTERRUPTS() \
do { \
+ NotHoldingSpinLock(); \
if (InterruptPending) \
ProcessInterrupts(); \
} while(0)
@@ -105,6 +110,7 @@ do { \
#define CHECK_FOR_INTERRUPTS() \
do { \
+ NotHoldingSpinLock(); \
if (UNBLOCKED_SIGNAL_QUEUE()) \
pgwin32_dispatch_queued_signals(); \
if (InterruptPending) \
@@ -113,7 +119,11 @@ do { \
#endif /* WIN32 */
-#define HOLD_INTERRUPTS() (InterruptHoldoffCount++)
+#define HOLD_INTERRUPTS() \
+do { \
+ NotHoldingSpinLock(); \
+ InterruptHoldoffCount++; \
+} while(0)
#define RESUME_INTERRUPTS() \
do { \
diff --git a/src/include/storage/spin.h b/src/include/storage/spin.h
index 5ad25d0f6f..6d806620a1 100644
--- a/src/include/storage/spin.h
+++ b/src/include/storage/spin.h
@@ -56,12 +56,26 @@
#include "storage/pg_sema.h"
#endif
+extern volatile slock_t *held_spinlock;
+
+#define NotHoldingSpinLock() \
+ Assert(held_spinlock == NULL)
#define SpinLockInit(lock) S_INIT_LOCK(lock)
-#define SpinLockAcquire(lock) S_LOCK(lock)
+#define SpinLockAcquire(lock) \
+ do { \
+ Assert(held_spinlock == NULL); \
+ S_LOCK(lock); \
+ held_spinlock = (lock); \
+ } while (0)
-#define SpinLockRelease(lock) S_UNLOCK(lock)
+#define SpinLockRelease(lock) \
+ do { \
+ Assert(held_spinlock == (lock)); \
+ S_UNLOCK(lock); \
+ held_spinlock = NULL; \
+ } while (0)
#define SpinLockFree(lock) S_LOCK_FREE(lock)
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 1e09ee0541..902996fed4 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -16,6 +16,10 @@
#include <setjmp.h>
+#ifndef FRONTEND
+#include "storage/spin.h"
+#endif
+
/* Error level codes */
#define DEBUG5 10 /* Debugging messages, in categories of
* decreasing detail. */
@@ -124,6 +128,7 @@
#define ereport_domain(elevel, domain, ...) \
do { \
pg_prevent_errno_in_scope(); \
+ NotHoldingSpinLock(); \
if (errstart(elevel, domain)) \
__VA_ARGS__, errfinish(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
@@ -134,6 +139,7 @@
do { \
const int elevel_ = (elevel); \
pg_prevent_errno_in_scope(); \
+ NotHoldingSpinLock(); \
if (errstart(elevel_, domain)) \
__VA_ARGS__, errfinish(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
if (elevel_ >= ERROR) \
