Re: COPYable logs - Mailing list pgsql-patches

Tom Lane wrote:
> 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.

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