]> granicus.if.org Git - postgresql/commitdiff
Change StatementCancelHandler() to check the DoingCommandRead flag to decide
authorTom Lane <tgl@sss.pgh.pa.us>
Sat, 26 Jan 2008 19:55:08 +0000 (19:55 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Sat, 26 Jan 2008 19:55:08 +0000 (19:55 +0000)
whether to execute an immediate interrupt, rather than testing whether
LockWaitCancel() cancelled a lock wait.  The old way misclassified the case
where we were blocked in ProcWaitForSignal(), and arguably would misclassify
any other future additions of new ImmediateInterruptOK states too.  This
allows reverting the old kluge that gave LockWaitCancel() a return value,
since no callers care anymore.  Improve comments in the various
implementations of PGSemaphoreLock() to explain that on some platforms, the
assumption that semop() exits after a signal is wrong, and so we must ensure
that the signal handler itself throws elog if we want cancel or die interrupts
to be effective.  Per testing related to bug #3883, though this patch doesn't
solve those problems fully.

Perhaps this change should be back-patched, but since pre-8.3 branches aren't
really relying on autovacuum to respond to SIGINT, it doesn't seem critical
for them.

src/backend/port/posix_sema.c
src/backend/port/sysv_sema.c
src/backend/port/win32_sema.c
src/backend/storage/lmgr/proc.c
src/backend/tcop/postgres.c
src/include/storage/proc.h

index 7eaa89cdddb57bfeed2e29595ae3614c2e0ffd70..335dae270d0595013fed5711ec5d875545b10edc 100644 (file)
@@ -11,7 +11,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/port/posix_sema.c,v 1.19 2008/01/01 19:45:51 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/port/posix_sema.c,v 1.20 2008/01/26 19:55:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -241,37 +241,10 @@ PGSemaphoreLock(PGSemaphore sema, bool interruptOK)
        int                     errStatus;
 
        /*
-        * Note: if errStatus is -1 and errno == EINTR then it means we returned
-        * 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. We
-        * assume that if such an interrupt comes in while we are waiting, it will
-        * cause the sem_wait() call to exit with errno == EINTR, so that we will
-        * be able to service the interrupt (if not in a critical section
-        * already).
-        *
-        * 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 sem_wait() 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 sem_wait()).  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 sem_wait() 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 Posix semaphores
-        * used to implement LW locks or emulate spinlocks --- but the wait time
-        * for such locks should not be very long, anyway.)
+        * 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.
         */
        do
        {
index 876b9c6bbb03b09f210ba5b5303ed4c702d544ab..54a1f7c93b388e9e2085ac91ed15240bc0e7cdda 100644 (file)
@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/port/sysv_sema.c,v 1.22 2008/01/01 19:45:51 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/port/sysv_sema.c,v 1.23 2008/01/26 19:55:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -377,10 +377,11 @@ 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. We
-        * assume that if such an interrupt comes in while we are waiting, it will
-        * cause the semop() call to exit with errno == EINTR, so that we will be
-        * able to service the interrupt (if not in a critical section already).
+        * 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
@@ -403,6 +404,14 @@ PGSemaphoreLock(PGSemaphore sema, bool interruptOK)
         * 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 other types
+        * of interrupts.
         */
        do
        {
index 6dc50bff8d64e9a1525645233fc3458bde4c7d40..3c2248b0788e3de489ed28f8a9a23a9626be3059 100644 (file)
@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/port/win32_sema.c,v 1.6 2008/01/01 19:45:51 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/port/win32_sema.c,v 1.7 2008/01/26 19:55:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -124,6 +124,12 @@ PGSemaphoreLock(PGSemaphore sema, bool interruptOK)
        wh[0] = *sema;
        wh[1] = pgwin32_signal_event;
 
+       /*
+        * As in other implementations of PGSemaphoreLock, we need to check
+        * for cancel/die interrupts each time through the loop.  But here,
+        * there is no hidden magic about whether the syscall will internally
+        * service a signal --- we do that ourselves.
+        */
        do
        {
                ImmediateInterruptOK = interruptOK;
index 48d18e54781d5134c73203d638f9b8ca4b454884..0ac70da1f0a01293039a07aebf80b997fefe9321 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/storage/lmgr/proc.c,v 1.198 2008/01/01 19:45:52 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/storage/lmgr/proc.c,v 1.199 2008/01/26 19:55:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -486,20 +486,18 @@ HaveNFreeProcs(int n)
 /*
  * Cancel any pending wait for lock, when aborting a transaction.
  *
- * Returns true if we had been waiting for a lock, else false.
- *
  * (Normally, this would only happen if we accept a cancel/die
  * interrupt while waiting; but an ereport(ERROR) while waiting is
  * within the realm of possibility, too.)
  */
-bool
+void
 LockWaitCancel(void)
 {
        LWLockId        partitionLock;
 
        /* Nothing to do if we weren't waiting for a lock */
        if (lockAwaited == NULL)
-               return false;
+               return;
 
        /* Turn off the deadlock timer, if it's still running (see ProcSleep) */
        disable_sig_alarm(false);
@@ -538,12 +536,6 @@ LockWaitCancel(void)
         * wakeup signal isn't harmful, and it seems not worth expending cycles to
         * get rid of a signal that most likely isn't there.
         */
-
-       /*
-        * Return true even if we were kicked off the lock before we were able to
-        * remove ourselves.
-        */
-       return true;
 }
 
 
index 05127c40b309a7d82a186f6ceb38fe7c00c81158..325411df09ca2e3b96c1da6e06d901570d3f67e8 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.541 2008/01/01 19:45:52 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.542 2008/01/26 19:55:08 tgl Exp $
  *
  * NOTES
  *       this is the "main" module of the postgres backend and
@@ -2449,10 +2449,9 @@ die(SIGNAL_ARGS)
                        /* bump holdoff count to make ProcessInterrupts() a no-op */
                        /* until we are done getting ready for it */
                        InterruptHoldoffCount++;
+                       LockWaitCancel();       /* prevent CheckDeadLock from running */
                        DisableNotifyInterrupt();
                        DisableCatchupInterrupt();
-                       /* Make sure CheckDeadLock won't run while shutting down... */
-                       LockWaitCancel();
                        InterruptHoldoffCount--;
                        ProcessInterrupts();
                }
@@ -2498,20 +2497,16 @@ StatementCancelHandler(SIGNAL_ARGS)
                 * waiting for input, however.
                 */
                if (ImmediateInterruptOK && InterruptHoldoffCount == 0 &&
-                       CritSectionCount == 0)
+                       CritSectionCount == 0 && !DoingCommandRead)
                {
                        /* bump holdoff count to make ProcessInterrupts() a no-op */
                        /* until we are done getting ready for it */
                        InterruptHoldoffCount++;
-                       if (LockWaitCancel())
-                       {
-                               DisableNotifyInterrupt();
-                               DisableCatchupInterrupt();
-                               InterruptHoldoffCount--;
-                               ProcessInterrupts();
-                       }
-                       else
-                               InterruptHoldoffCount--;
+                       LockWaitCancel();       /* prevent CheckDeadLock from running */
+                       DisableNotifyInterrupt();
+                       DisableCatchupInterrupt();
+                       InterruptHoldoffCount--;
+                       ProcessInterrupts();
                }
        }
 
index 5b7c7ec4e03fd7f077eb9d5353f252e3e0789c7a..1ce3eb26fc324a49ff144c8af155402ef5e174d5 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/storage/proc.h,v 1.103 2008/01/01 19:45:59 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/storage/proc.h,v 1.104 2008/01/26 19:55:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -166,7 +166,7 @@ extern void ProcQueueInit(PROC_QUEUE *queue);
 extern int     ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable);
 extern PGPROC *ProcWakeup(PGPROC *proc, int waitStatus);
 extern void ProcLockWakeup(LockMethod lockMethodTable, LOCK *lock);
-extern bool LockWaitCancel(void);
+extern void LockWaitCancel(void);
 
 extern void ProcWaitForSignal(void);
 extern void ProcSendSignal(int pid);