Change StatementCancelHandler() to check the DoingCommandRead flag to decide

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.

1 files changed, 14 insertions, 5 deletions

diff --git a/src/backend/port/sysv_sema.c b/src/backend/port/sysv_sema.c
index 876b9c6bbb0..54a1f7c93b3 100644
--- a/src/backend/port/sysv_sema.c
+++ b/src/backend/port/sysv_sema.c
@@ -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
 	{