From: Heikki Linnakangas 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);