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 304dc574d864d1fa869e22151189e57e50e42f94..be67c9e5a287da47f7e1f538125b079cc9bb7bfe 100644 (file)
--- 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.56 2010/04/01 20:12:22 heikki Exp $
+ *       $PostgreSQL: pgsql/src/backend/postmaster/syslogger.c,v 1.57 2010/04/16 09:51:49 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);
 
        threadHandle = (HANDLE) _beginthreadex(NULL, 0, pipeThread, NULL, 0, NULL);
        if (threadHandle == 0)
@@ -423,8 +424,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)
@@ -911,17 +920,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));
@@ -945,11 +946,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();
 
@@ -966,6 +977,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 */
@@ -974,6 +986,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;
 }
@@ -1097,18 +1110,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);
@@ -1164,18 +1168,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);