Re: COPYable logs - Mailing list pgsql-patches
From | Tom Lane |
---|---|
Subject | Re: COPYable logs |
Date | |
Msg-id | 13585.1185220994@sss.pgh.pa.us Whole thread Raw |
In response to | COPYable logs (Andrew Dunstan <andrew@dunslane.net>) |
Responses |
Re: COPYable logs
Re: COPYable logs |
List | pgsql-patches |
Andrew Dunstan <andrew@dunslane.net> writes: > Here is my latest version of this patch. The good news is that it now > seems to work on Windows. Please review carefully (esp Magnus, Dave, Tom). The GUC arrangements for this patch are utterly broken. The reason for the separation between Redirect_stderr and Log_destination is that Log_destination is allowed to change at SIGHUP, whereas Redirect_stderr can't change after startup (since it'd be too late to create the pipe). The exact same problem applies to the CSV pipe, and therefore you can't use (Log_destination & LOG_DESTINATION_CSVLOG) to control startup-time decisions. There are two approaches we could take to fixing this: 1. Redirect_stderr is the start-time flag for both features, ie, if it's set we always create both pipes and start the syslogger, but Log_destination controls what actually gets used. (Possibly Redirect_stderr should be renamed if we go this way.) 2. Invent a new PGC_POSTMASTER GUC variable that says whether to create the CSV pipe; then the syslogger process is started if either flag is set. #1 seems simpler but it might be too restrictive. (In particular I'm not too sure what happens to the syslogger's own stderr.) Also, given the creation of the chunking protocol for the stderr pipe, I would argue that there is no longer any value in actually having two pipes: you could include a flag in each chunk to say which protocol it belongs to. (This wouldn't even cost any extra bits, since is_last could be replaced with a four-valued field.) The patch could be simplified a lot if we got rid of the second pipe. > ! if (Redirect_stderr) > ! { > ! unsigned int tid; > ! int logtype = STDERR_LOGFILE; > + InitializeCriticalSection(&sysfileSection); > + stderrThreadHandle = (HANDLE) _beginthreadex(0, 0, pipeThread, > + &logtype, 0, &tid); > + } That can't possibly be a safe programming technique, can it? The local variable logtype could disappear from the stack long before pipeThread gets a chance to read it. I think you'd have to make it static to make this reliable. However, if you adopt suggestion above then this code goes away anyway. > extern bool redirection_done; > + char timestamp[128]; > + Why in the world is this made a global variable? AFAICS it should be local in get_timestamp. Or at least "static". > *** src/include/miscadmin.h 16 Apr 2007 18:29:56 -0000 1.194 > --- src/include/miscadmin.h 22 Jul 2007 22:18:45 -0000 > *************** > *** 132,137 **** > --- 132,138 ---- > extern int MaxConnections; > extern DLLIMPORT int MyProcPid; > + extern DLLIMPORT time_t MyStartTime; > extern DLLIMPORT struct Port *MyProcPort; > extern long MyCancelKey; Does this even compile? I don't think we necessarily include <time.h> before this. > + #define LOG_BUFFER_SIZE 32768 > + > + #define STDERR_LOGFILE 1 > + #define CSV_LOGFILE 2 Doesn't seem like LOG_BUFFER_SIZE should be exported to the world. Does seem like the other two require a comment. Should they be an enum type? Another thing that needs to be looked at carefully is how much memory write_csvlog() eats. I'm a little bit concerned about whether it will result in infinite recursion when our backs are against the wall and we only have the original 8K in ErrorContext to work in. (We could increase that figure if need be, but we need to know by how much.) As a general style note, the patch seems to add and remove blank lines in a most arbitrary and inconsistent way. Please try to clean that up. pgindent would fix some of that, but it's not very good about removing blank lines that shouldn't be there. regards, tom lane
pgsql-patches by date: