Remove the option to service interrupts during PGSemaphoreLock().
authorAndres Freund
Tue, 3 Feb 2015 22:25:00 +0000 (23:25 +0100)
committerAndres Freund
Tue, 3 Feb 2015 22:25:00 +0000 (23:25 +0100)
The remaining caller (lwlocks) doesn't need that facility, and we plan
to remove ImmedidateInterruptOK entirely. That means that interrupts
can't be serviced race-free and portably anyway, so there's little
reason for keeping the feature.

Reviewed-By: Heikki Linnakangas
src/backend/port/posix_sema.c
src/backend/port/sysv_sema.c
src/backend/port/win32_sema.c
src/backend/storage/lmgr/lwlock.c
src/include/storage/pg_sema.h

index 633bf5afebb0df63a0d84f164f03dd123bc8085b..ad9c80a8b5be7d138d5c9ea35c342627fb0f4f83 100644 (file)
@@ -236,22 +236,14 @@ PGSemaphoreReset(PGSemaphore sema)
  * Lock a semaphore (decrement count), blocking if count would be < 0
  */
 void
-PGSemaphoreLock(PGSemaphore sema, bool interruptOK)
+PGSemaphoreLock(PGSemaphore sema)
 {
    int         errStatus;
 
-   /*
-    * See notes in sysv_sema.c's implementation of PGSemaphoreLock. Just as
-    * that code does for semop(), we handle both the case where sem_wait()
-    * returns errno == EINTR after a signal, and the case where it just keeps
-    * waiting.
-    */
+   /* See notes in sysv_sema.c's implementation of PGSemaphoreLock. */
    do
    {
-       ImmediateInterruptOK = interruptOK;
-       CHECK_FOR_INTERRUPTS();
        errStatus = sem_wait(PG_SEM_REF(sema));
-       ImmediateInterruptOK = false;
    } while (errStatus < 0 && errno == EINTR);
 
    if (errStatus < 0)
index 5a0193f06b1c461a48fce7fe1b1889217a6b5ab2..ba46c640fc9a1a70a3ef88ae3bc6582a0e327e38 100644 (file)
@@ -364,7 +364,7 @@ PGSemaphoreReset(PGSemaphore sema)
  * Lock a semaphore (decrement count), blocking if count would be < 0
  */
 void
-PGSemaphoreLock(PGSemaphore sema, bool interruptOK)
+PGSemaphoreLock(PGSemaphore sema)
 {
    int         errStatus;
    struct sembuf sops;
@@ -378,48 +378,13 @@ PGSemaphoreLock(PGSemaphore sema, bool interruptOK)
     * from the operation prematurely because we were sent a signal.  So we
     * try and lock the semaphore again.
     *
-    * Each time around the loop, we check for a cancel/die interrupt.  On
-    * some platforms, if such an interrupt comes in while we are waiting, it
-    * will cause the semop() call to exit with errno == EINTR, allowing us to
-    * service the interrupt (if not in a critical section already) during the
-    * next loop iteration.
-    *
-    * Once we acquire the lock, we do NOT check for an interrupt before
-    * returning.  The caller needs to be able to record ownership of the lock
-    * before any interrupt can be accepted.
-    *
-    * There is a window of a few instructions between CHECK_FOR_INTERRUPTS
-    * and entering the semop() call.  If a cancel/die interrupt occurs in
-    * that window, we would fail to notice it until after we acquire the lock
-    * (or get another interrupt to escape the semop()).  We can avoid this
-    * problem by temporarily setting ImmediateInterruptOK to true before we
-    * do CHECK_FOR_INTERRUPTS; then, a die() interrupt in this interval will
-    * execute directly.  However, there is a huge pitfall: there is another
-    * window of a few instructions after the semop() before we are able to
-    * reset ImmediateInterruptOK.  If an interrupt occurs then, we'll lose
-    * control, which means that the lock has been acquired but our caller did
-    * not get a chance to record the fact. Therefore, we only set
-    * ImmediateInterruptOK if the caller tells us it's OK to do so, ie, the
-    * caller does not need to record acquiring the lock.  (This is currently
-    * true for lockmanager locks, since the process that granted us the lock
-    * did all the necessary state updates. It's not true for SysV semaphores
-    * used to implement LW locks or emulate spinlocks --- but the wait time
-    * for such locks should not be very long, anyway.)
-    *
-    * On some platforms, signals marked SA_RESTART (which is most, for us)
-    * will not interrupt the semop(); it will just keep waiting.  Therefore
-    * it's necessary for cancel/die interrupts to be serviced directly by the
-    * signal handler.  On these platforms the behavior is really the same
-    * whether the signal arrives just before the semop() begins, or while it
-    * is waiting.  The loop on EINTR is thus important only for platforms
-    * without SA_RESTART.
+    * We used to check interrupts here, but that required servicing
+    * interrupts directly from signal handlers. Which is hard to do safely
+    * and portably.
     */
    do
    {
-       ImmediateInterruptOK = interruptOK;
-       CHECK_FOR_INTERRUPTS();
        errStatus = semop(sema->semId, &sops, 1);
-       ImmediateInterruptOK = false;
    } while (errStatus < 0 && errno == EINTR);
 
    if (errStatus < 0)
index f848ff82b09ba35a2f4f25256ade6aab15fa13d8..011e2fd4a6ac2a301e6baa45e034f6914bfc0d2f 100644 (file)
@@ -116,13 +116,11 @@ PGSemaphoreReset(PGSemaphore sema)
  * Serve the interrupt if interruptOK is true.
  */
 void
-PGSemaphoreLock(PGSemaphore sema, bool interruptOK)
+PGSemaphoreLock(PGSemaphore sema)
 {
    HANDLE      wh[2];
    bool        done = false;
 
-   ImmediateInterruptOK = interruptOK;
-
    /*
     * Note: pgwin32_signal_event should be first to ensure that it will be
     * reported when multiple events are set.  We want to guarantee that
@@ -173,8 +171,6 @@ PGSemaphoreLock(PGSemaphore sema, bool interruptOK)
                break;
        }
    }
-
-   ImmediateInterruptOK = false;
 }
 
 /*
index 7cb002357a26c7495857599cdf1b0e03df864679..7ca8dc0007bd2d77277e7de1ab919b2592f87819 100644 (file)
@@ -863,8 +863,7 @@ LWLockDequeueSelf(LWLock *lock)
         */
        for (;;)
        {
-           /* "false" means cannot accept cancel/die interrupt here. */
-           PGSemaphoreLock(&MyProc->sem, false);
+           PGSemaphoreLock(&MyProc->sem);
            if (!MyProc->lwWaiting)
                break;
            extraWaits++;
@@ -1034,8 +1033,7 @@ LWLockAcquireCommon(LWLock *lock, LWLockMode mode, uint64 *valptr, uint64 val)
 
        for (;;)
        {
-           /* "false" means cannot accept cancel/die interrupt here. */
-           PGSemaphoreLock(&proc->sem, false);
+           PGSemaphoreLock(&proc->sem);
            if (!proc->lwWaiting)
                break;
            extraWaits++;
@@ -1195,8 +1193,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
 
            for (;;)
            {
-               /* "false" means cannot accept cancel/die interrupt here. */
-               PGSemaphoreLock(&proc->sem, false);
+               PGSemaphoreLock(&proc->sem);
                if (!proc->lwWaiting)
                    break;
                extraWaits++;
@@ -1397,8 +1394,7 @@ LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newval)
 
        for (;;)
        {
-           /* "false" means cannot accept cancel/die interrupt here. */
-           PGSemaphoreLock(&proc->sem, false);
+           PGSemaphoreLock(&proc->sem);
            if (!proc->lwWaiting)
                break;
            extraWaits++;
index 5eaf2bf582a7f2c7e8e0bb408c0f1fdb2a0038c3..9f8be759fbafac564d5420f823a1a4cb1a5eb26d 100644 (file)
@@ -72,7 +72,7 @@ extern void PGSemaphoreCreate(PGSemaphore sema);
 extern void PGSemaphoreReset(PGSemaphore sema);
 
 /* Lock a semaphore (decrement count), blocking if count would be < 0 */
-extern void PGSemaphoreLock(PGSemaphore sema, bool interruptOK);
+extern void PGSemaphoreLock(PGSemaphore sema);
 
 /* Unlock a semaphore (increment count) */
 extern void PGSemaphoreUnlock(PGSemaphore sema);