From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 16 Apr 2010 09:52:01 +0000 (+0000)
Subject: On Windows, syslogger runs in two threads. The main thread processes config
X-Git-Tag: REL8_3_11~19
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=144399a331c9ac7059669a16d470239753257ac5;p=postgresql

On Windows, syslogger runs in two threads. The main thread processes config
reload and rotation signals, and a helper thread reads messages from the
pipe and writes them to the log file. However, server code isn't generally
thread-safe, so if both try to do e.g palloc()/pfree() at the same time,
bad things will happen. To fix that, use a critical section (which is like
a mutex) to enforce that only one the threads are active at a time.
---

diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 3b1ad0b0f3..deeb8d3f35 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -18,7 +18,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/postmaster/syslogger.c,v 1.44.2.3 2010/04/01 20:12:34 heikki Exp $
+ *	  $PostgreSQL: pgsql/src/backend/postmaster/syslogger.c,v 1.44.2.4 2010/04/16 09:52:01 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -117,7 +117,7 @@ HANDLE		syslogPipe[2] = {0, 0};
 
 #ifdef WIN32
 static HANDLE threadHandle = 0;
-static CRITICAL_SECTION sysfileSection;
+static CRITICAL_SECTION sysloggerSection;
 #endif
 
 /*
@@ -268,7 +268,8 @@ SysLoggerMain(int argc, char *argv[])
 
 #ifdef WIN32
 	/* Fire up separate data transfer thread */
-	InitializeCriticalSection(&sysfileSection);
+	InitializeCriticalSection(&sysloggerSection);
+	EnterCriticalSection(&sysloggerSection);
 
 	{
 		unsigned int tid;
@@ -424,8 +425,16 @@ SysLoggerMain(int argc, char *argv[])
 		 * On Windows we leave it to a separate thread to transfer data and
 		 * detect pipe EOF.  The main thread just wakes up once a second to
 		 * check for SIGHUP and rotation conditions.
+		 *
+		 * Server code isn't generally thread-safe, so we ensure that only
+		 * one of the threads is active at a time by entering the critical
+		 * section whenever we're not sleeping.
 		 */
+		LeaveCriticalSection(&sysloggerSection);
+
 		pg_usleep(1000000L);
+
+		EnterCriticalSection(&sysloggerSection);
 #endif   /* WIN32 */
 
 		if (pipe_eof_seen)
@@ -912,17 +921,9 @@ write_syslogger_file(const char *buffer, int count, int destination)
 	if (destination == LOG_DESTINATION_CSVLOG && csvlogFile == NULL)
 		open_csvlogfile();
 
-#ifdef WIN32
-	EnterCriticalSection(&sysfileSection);
-#endif
-
 	logfile = destination == LOG_DESTINATION_CSVLOG ? csvlogFile : syslogFile;
 	rc = fwrite(buffer, 1, count, logfile);
 
-#ifdef WIN32
-	LeaveCriticalSection(&sysfileSection);
-#endif
-
 	/* can't use ereport here because of possible recursion */
 	if (rc != count)
 		write_stderr("could not write to log file: %s\n", strerror(errno));
@@ -946,11 +947,21 @@ pipeThread(void *arg)
 	for (;;)
 	{
 		DWORD		bytesRead;
+		BOOL		result;
+
+		result = ReadFile(syslogPipe[0],
+						  logbuffer + bytes_in_logbuffer,
+						  sizeof(logbuffer) - bytes_in_logbuffer,
+						  &bytesRead, 0);
 
-		if (!ReadFile(syslogPipe[0],
-					  logbuffer + bytes_in_logbuffer,
-					  sizeof(logbuffer) - bytes_in_logbuffer,
-					  &bytesRead, 0))
+		/*
+		 * Enter critical section before doing anything that might touch
+		 * global state shared by the main thread. Anything that uses
+		 * palloc()/pfree() in particular are not safe outside the critical
+		 * section.
+		 */
+		EnterCriticalSection(&sysloggerSection);
+		if (!result)
 		{
 			DWORD		error = GetLastError();
 
@@ -967,6 +978,7 @@ pipeThread(void *arg)
 			bytes_in_logbuffer += bytesRead;
 			process_pipe_input(logbuffer, &bytes_in_logbuffer);
 		}
+		LeaveCriticalSection(&sysloggerSection);
 	}
 
 	/* We exit the above loop only upon detecting pipe EOF */
@@ -975,6 +987,7 @@ pipeThread(void *arg)
 	/* if there's any data left then force it out now */
 	flush_pipe_input(logbuffer, &bytes_in_logbuffer);
 
+	LeaveCriticalSection(&sysloggerSection);
 	_endthread();
 	return 0;
 }
@@ -1098,18 +1111,9 @@ logfile_rotate(bool time_based_rotation, int size_rotation_for)
 		_setmode(_fileno(fh), _O_TEXT); /* use CRLF line endings on Windows */
 #endif
 
-		/* On Windows, need to interlock against data-transfer thread */
-#ifdef WIN32
-		EnterCriticalSection(&sysfileSection);
-#endif
-
 		fclose(syslogFile);
 		syslogFile = fh;
 
-#ifdef WIN32
-		LeaveCriticalSection(&sysfileSection);
-#endif
-
 		/* instead of pfree'ing filename, remember it for next time */
 		if (last_file_name != NULL)
 			pfree(last_file_name);
@@ -1165,18 +1169,9 @@ logfile_rotate(bool time_based_rotation, int size_rotation_for)
 		_setmode(_fileno(fh), _O_TEXT); /* use CRLF line endings on Windows */
 #endif
 
-		/* On Windows, need to interlock against data-transfer thread */
-#ifdef WIN32
-		EnterCriticalSection(&sysfileSection);
-#endif
-
 		fclose(csvlogFile);
 		csvlogFile = fh;
 
-#ifdef WIN32
-		LeaveCriticalSection(&sysfileSection);
-#endif
-
 		/* instead of pfree'ing filename, remember it for next time */
 		if (last_csv_file_name != NULL)
 			pfree(last_csv_file_name);