Tom Lane wrote:
Andrew Dunstan <[EMAIL PROTECTED]> 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.
As always I submit to your gentle reproofs. I will try to attend to all
this next weekend.
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.)
#1 seems more intuitive to me, although it should be renamed, with a
caveat that we shouldn't allow csvlog as a destination if it's off.
Syslogger's own stderr isn't redirected, as we have discussed previously.
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.
yeah, probably.
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.)
well, it will be a lot less than it was originally ... I guess the major
extra cost comes from having to CSV escape the error message. Using 8k
for one copy let alone two seems way too small. Not sure what we should
do about it.
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.
sure.
cheers
andrew
---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings
