Refactor fallback to stderr for csvlog to handle better WIN32 service case

michaelpq · michaelpq · commit 8b76f89c3797 · 2021-10-08T11:08:35.000+09:00
send_message_to_server_log() would force a redirection of a log entry to stderr in some cases for csvlog, like the syslogger not being available yet. If this happens, csvlog would fall back to stderr to log some information rather than nothing. The code was organized so as stderr is done before csvlog, with csvlog checking that stderr did not happen yet with a reversed condition. With this code organization, it could be possible to lose some messages if running Postgres as a service on WIN32, as there is no usable stderr, and the handling of the StringInfoData holding the message for stderr was rather confusing because of that. This commit moves the csvlog handling to be before stderr, as as we are able to track down if it is necessary to log something to stderr. The reduces the handling of stderr to be in a single code path, adding a fallback to event logs for a WIN32 service. This also simplifies the way we handle the StringInfoData for stderr, making easier the integration of new file-based log destinations. I got to play with services and event logs on Windows while checking this change. Reviewed-by: Chris Bandy Discussion: https://postgr.es/m/YV0vwBovEKf1WXkl@paquier.xyz
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
@@ -3008,6 +3008,7 @@ static void
 send_message_to_server_log(ErrorData *edata)
 {
 	StringInfoData buf;
+	bool		fallback_to_stderr = false;
 
 	initStringInfo(&buf);
 
@@ -3159,8 +3160,27 @@ send_message_to_server_log(ErrorData *edata)
 	}
 #endif							/* WIN32 */
 
-	/* Write to stderr, if enabled */
-	if ((Log_destination & LOG_DESTINATION_STDERR) || whereToSendOutput == DestDebug)
+	/* Write to csvlog, if enabled */
+	if (Log_destination & LOG_DESTINATION_CSVLOG)
+	{
+		/*
+		 * Send CSV data if it's safe to do so (syslogger doesn't need the
+		 * pipe).  If this is not possible, fallback to an entry written to
+		 * stderr.
+		 */
+		if (redirection_done || MyBackendType == B_LOGGER)
+			write_csvlog(edata);
+		else
+			fallback_to_stderr = true;
+	}
+
+	/*
+	 * Write to stderr, if enabled or if required because of a previous
+	 * limitation.
+	 */
+	if ((Log_destination & LOG_DESTINATION_STDERR) ||
+		whereToSendOutput == DestDebug ||
+		fallback_to_stderr)
 	{
 		/*
 		 * Use the chunking protocol if we know the syslogger should be
@@ -3189,34 +3209,8 @@ send_message_to_server_log(ErrorData *edata)
 	if (MyBackendType == B_LOGGER)
 		write_syslogger_file(buf.data, buf.len, LOG_DESTINATION_STDERR);
 
-	/* Write to CSV log if enabled */
-	if (Log_destination & LOG_DESTINATION_CSVLOG)
-	{
-		if (redirection_done || MyBackendType == B_LOGGER)
-		{
-			/*
-			 * send CSV data if it's safe to do so (syslogger doesn't need the
-			 * pipe). First get back the space in the message buffer.
-			 */
-			pfree(buf.data);
-			write_csvlog(edata);
-		}
-		else
-		{
-			/*
-			 * syslogger not up (yet), so just dump the message to stderr,
-			 * unless we already did so above.
-			 */
-			if (!(Log_destination & LOG_DESTINATION_STDERR) &&
-				whereToSendOutput != DestDebug)
-				write_console(buf.data, buf.len);
-			pfree(buf.data);
-		}
-	}
-	else
-	{
-		pfree(buf.data);
-	}
+	/* No more need of the message formatted for stderr */
+	pfree(buf.data);
 }
 
 /*
