Re: elog(DEBUG2 in SpinLocked section.

Michael Paquier <michael@paquier.xyz> 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) \