Fix bogus when-to-deregister-from-listener-array logic.

tglsfdc · tglsfdc · commit ae1525fdcba5 · 2013-02-13T12:48:20.000-05:00
Since a backend adds itself to the global listener array during
Exec_ListenPreCommit, it's inappropriate for it to remove itself during
Exec_UnlistenCommit or Exec_UnlistenAllCommit --- that leads to failure
when committing a transaction that did UNLISTEN then LISTEN, since we end
up not registered though we should be.  (This leads to missing later
notifications, or to Assert failures in assert-enabled builds.)  Instead
deal with deregistering at the bottom of AtCommit_Notify, when we know the
final state of the listenChannels list.

Also, simplify the representation of registration status by replacing the
transient backendHasExecutedInitialListen flag with an amRegisteredListener
flag.

Per report from Greg Sabino Mullane.  Back-patch to 9.0, where the problem
was introduced during the LISTEN/NOTIFY rewrite.
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
@@ -349,12 +349,12 @@ static volatile sig_atomic_t notifyInterruptOccurred = 0;
 /* True if we've registered an on_shmem_exit cleanup */
 static bool unlistenExitRegistered = false;
 
+/* True if we're currently registered as a listener in asyncQueueControl */
+static bool amRegisteredListener = false;
+
 /* has this backend sent notifications in the current transaction? */
 static bool backendHasSentNotifications = false;
 
-/* has this backend executed its first LISTEN in the current transaction? */
-static bool backendHasExecutedInitialListen = false;
-
 /* GUC parameter */
 bool		Trace_notify = false;
 
@@ -745,6 +745,7 @@ static void
 Async_UnlistenOnExit(int code, Datum arg)
 {
 	Exec_UnlistenAllCommit();
+	asyncQueueUnregister();
 }
 
 /*
@@ -789,8 +790,6 @@ PreCommit_Notify(void)
 	if (Trace_notify)
 		elog(DEBUG1, "PreCommit_Notify");
 
-	Assert(backendHasExecutedInitialListen == false);
-
 	/* Preflight for any pending listen/unlisten actions */
 	foreach(p, pendingActions)
 	{
@@ -913,11 +912,9 @@ AtCommit_Notify(void)
 		}
 	}
 
-	/*
-	 * If we did an initial LISTEN, listenChannels now has the entry, so we no
-	 * longer need or want the flag to be set.
-	 */
-	backendHasExecutedInitialListen = false;
+	/* If no longer listening to anything, get out of listener array */
+	if (amRegisteredListener && listenChannels == NIL)
+		asyncQueueUnregister();
 
 	/* And clean up */
 	ClearPendingActionsAndNotifies();
@@ -935,19 +932,12 @@ Exec_ListenPreCommit(void)
 	 * Nothing to do if we are already listening to something, nor if we
 	 * already ran this routine in this transaction.
 	 */
-	if (listenChannels != NIL || backendHasExecutedInitialListen)
+	if (amRegisteredListener)
 		return;
 
 	if (Trace_notify)
 		elog(DEBUG1, "Exec_ListenPreCommit(%d)", MyProcPid);
 
-	/*
-	 * We need this variable to detect an aborted initial LISTEN. In that case
-	 * we would set up our pointer but not listen on any channel. This flag
-	 * gets cleared in AtCommit_Notify or AtAbort_Notify().
-	 */
-	backendHasExecutedInitialListen = true;
-
 	/*
 	 * Before registering, make sure we will unlisten before dying. (Note:
 	 * this action does not get undone if we abort later.)
@@ -971,6 +961,9 @@ Exec_ListenPreCommit(void)
 	QUEUE_BACKEND_PID(MyBackendId) = MyProcPid;
 	LWLockRelease(AsyncQueueLock);
 
+	/* Now we are listed in the global array, so remember we're listening */
+	amRegisteredListener = true;
+
 	/*
 	 * Try to move our pointer forward as far as possible. This will skip over
 	 * already-committed notifications. Still, we could get notifications that
@@ -1043,10 +1036,6 @@ Exec_UnlistenCommit(const char *channel)
 	 * We do not complain about unlistening something not being listened;
 	 * should we?
 	 */
-
-	/* If no longer listening to anything, get out of listener array */
-	if (listenChannels == NIL)
-		asyncQueueUnregister();
 }
 
 /*
@@ -1062,8 +1051,6 @@ Exec_UnlistenAllCommit(void)
 
 	list_free_deep(listenChannels);
 	listenChannels = NIL;
-
-	asyncQueueUnregister();
 }
 
 /*
@@ -1181,6 +1168,9 @@ asyncQueueUnregister(void)
 
 	Assert(listenChannels == NIL);		/* else caller error */
 
+	if (!amRegisteredListener)			/* nothing to do */
+		return;
+
 	LWLockAcquire(AsyncQueueLock, LW_SHARED);
 	/* check if entry is valid and oldest ... */
 	advanceTail = (MyProcPid == QUEUE_BACKEND_PID(MyBackendId)) &&
@@ -1189,6 +1179,9 @@ asyncQueueUnregister(void)
 	QUEUE_BACKEND_PID(MyBackendId) = InvalidPid;
 	LWLockRelease(AsyncQueueLock);
 
+	/* mark ourselves as no longer listed in the global array */
+	amRegisteredListener = false;
+
 	/* If we were the laziest backend, try to advance the tail pointer */
 	if (advanceTail)
 		asyncQueueAdvanceTail();
@@ -1545,21 +1538,12 @@ void
 AtAbort_Notify(void)
 {
 	/*
-	 * If we LISTEN but then roll back the transaction we have set our pointer
-	 * but have not made any entry in listenChannels. In that case, remove our
-	 * pointer again.
+	 * If we LISTEN but then roll back the transaction after PreCommit_Notify,
+	 * we have registered as a listener but have not made any entry in
+	 * listenChannels.  In that case, deregister again.
 	 */
-	if (backendHasExecutedInitialListen)
-	{
-		/*
-		 * Checking listenChannels should be redundant but it can't hurt doing
-		 * it for safety reasons.
-		 */
-		if (listenChannels == NIL)
-			asyncQueueUnregister();
-
-		backendHasExecutedInitialListen = false;
-	}
+	if (amRegisteredListener && listenChannels == NIL)
+		asyncQueueUnregister();
 
 	/* And clean up */
 	ClearPendingActionsAndNotifies();
