Remove the option to service interrupts during PGSemaphoreLock().

anarazel · anarazel · commit d06995710bc7 · 2015-02-03T23:25:00.000+01:00
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
diff --git a/src/backend/port/posix_sema.c b/src/backend/port/posix_sema.c
@@ -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)
diff --git a/src/backend/port/sysv_sema.c b/src/backend/port/sysv_sema.c
@@ -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)
diff --git a/src/backend/port/win32_sema.c b/src/backend/port/win32_sema.c
@@ -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;
 }
 
 /*
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
@@ -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++;
diff --git a/src/include/storage/pg_sema.h b/src/include/storage/pg_sema.h
@@ -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);

Original file line number	Diff line number	Diff line change
`@@ -116,13 +116,11 @@ PGSemaphoreReset(PGSemaphore sema)`
`116`	`116`	`* Serve the interrupt if interruptOK is true.`
`117`	`117`	`*/`
`118`	`118`	`void`
`119`		`-PGSemaphoreLock(PGSemaphore sema, bool interruptOK)`
	`119`	`+PGSemaphoreLock(PGSemaphore sema)`
`120`	`120`	`{`
`121`	`121`	`HANDLE wh[2];`
`122`	`122`	`bool done = false;`
`123`	`123`
`124`		`- ImmediateInterruptOK = interruptOK;`
`125`		`-`
`126`	`124`	`/*`
`127`	`125`	`* Note: pgwin32_signal_event should be first to ensure that it will be`
`128`	`126`	`* reported when multiple events are set. We want to guarantee that`
`@@ -173,8 +171,6 @@ PGSemaphoreLock(PGSemaphore sema, bool interruptOK)`
`173`	`171`	`break;`
`174`	`172`	`}`
`175`	`173`	`}`
`176`		`-`
`177`		`- ImmediateInterruptOK = false;`
`178`	`174`	`}`
`179`	`175`
`180`	`176`	`/*`