pipe chunks protocol
This patch implements the protocol Tom suggested for writing to the
syslogger pipe. It seems to pass my tests (basically "make installcheck"
against a server with stderr redirection turned on and log_statement set
to 'all').
The effect of this should be to prevent two problems:
. partial messages get written to the log file, which messes with
rotation, and
. messages from various backends get interleaved, causing garbled logs.
Please review ASAP. I want to get this applied soon so that a) it gets
wider testing and b) I can use it as the basis for the adapted CSV log
patch. If this is acceptable I intend to backpatch this all the way to
wherever we started using the syslogger pipe (was that 8.0?).
cheers
andrew
and here's the patch
Andrew Dunstan wrote:
Show quoted text
This patch implements the protocol Tom suggested for writing to the
syslogger pipe. It seems to pass my tests (basically "make
installcheck" against a server with stderr redirection turned on and
log_statement set to 'all').The effect of this should be to prevent two problems:
. partial messages get written to the log file, which messes with
rotation, and
. messages from various backends get interleaved, causing garbled logs.Please review ASAP. I want to get this applied soon so that a) it gets
wider testing and b) I can use it as the basis for the adapted CSV log
patch. If this is acceptable I intend to backpatch this all the way to
wherever we started using the syslogger pipe (was that 8.0?).cheers
andrew
Attachments:
logpipe.patchtext/x-patch; name=logpipe.patchDownload+278-25
Andrew Dunstan <andrew@dunslane.net> writes:
This patch implements the protocol Tom suggested for writing to the
syslogger pipe. It seems to pass my tests (basically "make
installcheck" against a server with stderr redirection turned on and
log_statement set to 'all').
I didn't like this patch much --- it broke the API of
write_syslogger_file, which is supposed to just write what it's given
(and it is used from outside syslogger.c with that expectation). Also
the way it slung unconsumed data back and forth between two buffers
seemed both confusing and inefficient. Here's a revised version.
In my testing, I found that a standard "make installcheck" run produces
only one message large enough to be split (the "infinite_recurse" thing
in errors.sql), so this is definitely not a Good Enough test. Maybe
we could get Ed L. or one of the other complainants to try it. (The
patch seems to need some adjustment to apply against 8.2, though.)
regards, tom lane
Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
This patch implements the protocol Tom suggested for writing to the
syslogger pipe. It seems to pass my tests (basically "make
installcheck" against a server with stderr redirection turned on and
log_statement set to 'all').I didn't like this patch much --- it broke the API of
write_syslogger_file, which is supposed to just write what it's given
(and it is used from outside syslogger.c with that expectation). Also
the way it slung unconsumed data back and forth between two buffers
seemed both confusing and inefficient. Here's a revised version.
Well. it was like the curate's egg :-) Anyway, thanks for the cleanup.
In my testing, I found that a standard "make installcheck" run produces
only one message large enough to be split (the "infinite_recurse" thing
in errors.sql), so this is definitely not a Good Enough test. Maybe
we could get Ed L. or one of the other complainants to try it.
Yeah, what I did was to wind back the chunk size - try 128 and you'll
see plenty of chunked messages :-) But we really need to do this with
installcheck-parallel to exercise it properly.
(The
patch seems to need some adjustment to apply against 8.2, though.)
I know we normally try not to do this, but I'd be happy to wait for the
back branches in this case.
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
Yeah, what I did was to wind back the chunk size - try 128 and you'll
see plenty of chunked messages :-) But we really need to do this with
installcheck-parallel to exercise it properly.
Doh, of course. I ran installcheck-parallel with log_statement = all
and the chunk size reduced to 64, and saw no apparent problems.
So that gives me at least enough confidence to apply it to HEAD.
(The
patch seems to need some adjustment to apply against 8.2, though.)
I know we normally try not to do this, but I'd be happy to wait for the
back branches in this case.
[confused...] How do you envision proceeding exactly?
regards, tom lane
Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
Yeah, what I did was to wind back the chunk size - try 128 and you'll
see plenty of chunked messages :-) But we really need to do this with
installcheck-parallel to exercise it properly.Doh, of course. I ran installcheck-parallel with log_statement = all
and the chunk size reduced to 64, and saw no apparent problems.
So that gives me at least enough confidence to apply it to HEAD.(The
patch seems to need some adjustment to apply against 8.2, though.)I know we normally try not to do this, but I'd be happy to wait for the
back branches in this case.[confused...] How do you envision proceeding exactly?
Never mind, if you're happy adapting and applying this right away to
back branches then I'm happy too. I just didn't want to have to wait
much before I start work on the CSVlog adaptation.
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
Tom Lane wrote:
[confused...] How do you envision proceeding exactly?
Never mind, if you're happy adapting and applying this right away to
back branches then I'm happy too. I just didn't want to have to wait
much before I start work on the CSVlog adaptation.
Actually, I was hoping you'd adapt/apply to the back branches ;-)
regards, tom lane