Re: [HACKERS] Thoughts about bug #3883 - Mailing list pgsql-patches

I wrote:
> No, the problem is merely to get LockWaitCancel to return "true" so that
> StatementCancelHandler will go ahead with the immediate interrupt.  No
> new cleanup is needed other than resetting the new flag.

Actually there's a better way to do it: we can remove a little bit of
complexity instead of adding more.  The basic problem is that
StatementCancelHandler wants to tell the difference between waiting for
client input (which there is no use for it to interrupt) and other
states in which ImmediateInterruptOK is set.  ProcWaitForSignal() is
falling on the wrong side of the classification --- and I argue that any
other newly added interruptable wait would too.  We should reverse the
sense of the test so that it's "not in client input" instead of "in lock
wait".  At the time that code was written, there wasn't any conveniently
cheap way to check for client input state, so we kluged up
LockWaitCancel to detect the other case.  But now that we have the
DoingCommandRead flag, it's easy to check that.  This lets us remove
LockWaitCancel's return value (which was always a bit untidy, since all
but one of its callers ignored the result), ending up with exactly
parallel code in die() and StatementCancelHandler().  Seems cleaner than
before.

            regards, tom lane

Index: src/backend/port/posix_sema.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/port/posix_sema.c,v
retrieving revision 1.19
diff -c -r1.19 posix_sema.c
*** src/backend/port/posix_sema.c    1 Jan 2008 19:45:51 -0000    1.19
--- src/backend/port/posix_sema.c    25 Jan 2008 22:41:22 -0000
***************
*** 241,277 ****
      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.)
       */
      do
      {
--- 241,250 ----
      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.
       */
      do
      {
Index: src/backend/port/sysv_sema.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/port/sysv_sema.c,v
retrieving revision 1.22
diff -c -r1.22 sysv_sema.c
*** src/backend/port/sysv_sema.c    1 Jan 2008 19:45:51 -0000    1.22
--- src/backend/port/sysv_sema.c    25 Jan 2008 22:41:22 -0000
***************
*** 377,386 ****
       * 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).
       *
       * 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
--- 377,387 ----
       * 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
***************
*** 403,408 ****
--- 404,417 ----
       * 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: src/backend/port/win32_sema.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/port/win32_sema.c,v
retrieving revision 1.6
diff -c -r1.6 win32_sema.c
*** src/backend/port/win32_sema.c    1 Jan 2008 19:45:51 -0000    1.6
--- src/backend/port/win32_sema.c    25 Jan 2008 22:41:22 -0000
***************
*** 124,129 ****
--- 124,135 ----
      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: src/backend/storage/lmgr/proc.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/storage/lmgr/proc.c,v
retrieving revision 1.198
diff -c -r1.198 proc.c
*** src/backend/storage/lmgr/proc.c    1 Jan 2008 19:45:52 -0000    1.198
--- src/backend/storage/lmgr/proc.c    25 Jan 2008 22:41:22 -0000
***************
*** 486,505 ****
  /*
   * 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
  LockWaitCancel(void)
  {
      LWLockId    partitionLock;

      /* Nothing to do if we weren't waiting for a lock */
      if (lockAwaited == NULL)
!         return false;

      /* Turn off the deadlock timer, if it's still running (see ProcSleep) */
      disable_sig_alarm(false);
--- 486,503 ----
  /*
   * Cancel any pending wait for lock, when aborting a transaction.
   *
   * (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.)
   */
! void
  LockWaitCancel(void)
  {
      LWLockId    partitionLock;

      /* Nothing to do if we weren't waiting for a lock */
      if (lockAwaited == NULL)
!         return;

      /* Turn off the deadlock timer, if it's still running (see ProcSleep) */
      disable_sig_alarm(false);
***************
*** 538,549 ****
       * 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;
  }


--- 536,541 ----
Index: src/backend/tcop/postgres.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/tcop/postgres.c,v
retrieving revision 1.541
diff -c -r1.541 postgres.c
*** src/backend/tcop/postgres.c    1 Jan 2008 19:45:52 -0000    1.541
--- src/backend/tcop/postgres.c    25 Jan 2008 22:41:22 -0000
***************
*** 2449,2458 ****
              /* bump holdoff count to make ProcessInterrupts() a no-op */
              /* until we are done getting ready for it */
              InterruptHoldoffCount++;
              DisableNotifyInterrupt();
              DisableCatchupInterrupt();
-             /* Make sure CheckDeadLock won't run while shutting down... */
-             LockWaitCancel();
              InterruptHoldoffCount--;
              ProcessInterrupts();
          }
--- 2449,2457 ----
              /* 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();
              InterruptHoldoffCount--;
              ProcessInterrupts();
          }
***************
*** 2498,2517 ****
           * waiting for input, however.
           */
          if (ImmediateInterruptOK && InterruptHoldoffCount == 0 &&
!             CritSectionCount == 0)
          {
              /* 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--;
          }
      }

--- 2497,2512 ----
           * waiting for input, however.
           */
          if (ImmediateInterruptOK && InterruptHoldoffCount == 0 &&
!             CritSectionCount == 0 && !DoingCommandRead)
          {
              /* 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();
!             InterruptHoldoffCount--;
!             ProcessInterrupts();
          }
      }

Index: src/include/storage/proc.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/storage/proc.h,v
retrieving revision 1.103
diff -c -r1.103 proc.h
*** src/include/storage/proc.h    1 Jan 2008 19:45:59 -0000    1.103
--- src/include/storage/proc.h    25 Jan 2008 22:41:22 -0000
***************
*** 166,172 ****
  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 ProcWaitForSignal(void);
  extern void ProcSendSignal(int pid);
--- 166,172 ----
  extern int    ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable);
  extern PGPROC *ProcWakeup(PGPROC *proc, int waitStatus);
  extern void ProcLockWakeup(LockMethod lockMethodTable, LOCK *lock);
! extern void LockWaitCancel(void);

  extern void ProcWaitForSignal(void);
  extern void ProcSendSignal(int pid);