Re: COPYable logs

Andrew Dunstan <andrew(at)dunslane(dot)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