Escalate ERRORs during async notify processing to FATAL

hlinnaka · hlinnaka · commit 7cb05dd2d198 · 2025-11-12T21:02:32.000+02:00
Previously, if async notify processing encountered an error, we would report the error to the client and advance our read position past the offending entry to prevent trying to process it over and over again. Trying to continue after an error has a few problems however: - We have no way of telling the client that a notification was lost. They get an ERROR, but that doesn't tell you much. As such, it's not clear if keeping the connection alive after losing a notification is a good thing. Depending on the application logic, missing a notification could cause the application to get stuck waiting, for example. - If the connection is idle, PqCommReadingMsg is set and any ERROR is turned into FATAL anyway. - We bailed out of the notification processing loop on first error without processing any subsequent notifications. The subsequent notifications would not be processed until another notify interrupt arrives. For example, if there were two notifications pending, and processing the first one caused an ERROR, the second notification would not be processed until someone sent a new NOTIFY. This commit changes the behavior so that any ERROR while processing async notifications is turned into FATAL, causing the client connection to be terminated. That makes the behavior more consistent as that's what happened in idle state already, and terminating the connection is a clear signal to the application that it might've missed some notifications. The reason to do this now is that the next commits will change the notification processing code in a way that would make it harder to skip over just the offending notification entry on error. Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com> Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de> Reviewed-by: Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> Discussion: https://www.postgresql.org/message-id/fedbd908-4571-4bbe-b48e-63bfdcc38f64@iki.fi Backpatch-through: 14
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
@@ -461,7 +461,7 @@ static double asyncQueueUsage(void);
 static void asyncQueueFillWarning(void);
 static void SignalBackends(void);
 static void asyncQueueReadAllNotifications(void);
-static bool asyncQueueProcessPageEntries(volatile QueuePosition *current,
+static bool asyncQueueProcessPageEntries(QueuePosition *current,
 										 QueuePosition stop,
 										 char *page_buffer,
 										 Snapshot snapshot);
@@ -1913,7 +1913,7 @@ ProcessNotifyInterrupt(bool flush)
 static void
 asyncQueueReadAllNotifications(void)
 {
-	volatile QueuePosition pos;
+	QueuePosition pos;
 	QueuePosition head;
 	Snapshot	snapshot;
 
@@ -1983,16 +1983,25 @@ asyncQueueReadAllNotifications(void)
 	 * It is possible that we fail while trying to send a message to our
 	 * frontend (for example, because of encoding conversion failure).  If
 	 * that happens it is critical that we not try to send the same message
-	 * over and over again.  Therefore, we place a PG_TRY block here that will
-	 * forcibly advance our queue position before we lose control to an error.
-	 * (We could alternatively retake NotifyQueueLock and move the position
-	 * before handling each individual message, but that seems like too much
-	 * lock traffic.)
+	 * over and over again.  Therefore, we set ExitOnAnyError to upgrade any
+	 * ERRORs to FATAL, causing the client connection to be closed on error.
+	 *
+	 * We used to only skip over the offending message and try to soldier on,
+	 * but it was somewhat questionable to lose a notification and give the
+	 * client an ERROR instead.  A client application is not be prepared for
+	 * that and can't tell that a notification was missed.  It was also not
+	 * very useful in practice because notifications are often processed while
+	 * a connection is idle and reading a message from the client, and in that
+	 * state, any error is upgraded to FATAL anyway.  Closing the connection
+	 * is a clear signal to the application that it might have missed
+	 * notifications.
 	 */
-	PG_TRY();
 	{
+		bool		save_ExitOnAnyError = ExitOnAnyError;
 		bool		reachedStop;
 
+		ExitOnAnyError = true;
+
 		do
 		{
 			int			curpage = QUEUE_POS_PAGE(pos);
@@ -2045,15 +2054,14 @@ asyncQueueReadAllNotifications(void)
 													   page_buffer.buf,
 													   snapshot);
 		} while (!reachedStop);
-	}
-	PG_FINALLY();
-	{
+
 		/* Update shared state */
 		LWLockAcquire(NotifyQueueLock, LW_SHARED);
 		QUEUE_BACKEND_POS(MyBackendId) = pos;
 		LWLockRelease(NotifyQueueLock);
+
+		ExitOnAnyError = save_ExitOnAnyError;
 	}
-	PG_END_TRY();
 
 	/* Done with snapshot */
 	UnregisterSnapshot(snapshot);
@@ -2076,7 +2084,7 @@ asyncQueueReadAllNotifications(void)
  * The QueuePosition *current is advanced past all processed messages.
  */
 static bool
-asyncQueueProcessPageEntries(volatile QueuePosition *current,
+asyncQueueProcessPageEntries(QueuePosition *current,
 							 QueuePosition stop,
 							 char *page_buffer,
 							 Snapshot snapshot)
