Re: [PATCHES] pipe chunks protocol
Tom Lane wrote: Actually, I was hoping you'd adapt/apply to the back branches ;-) curses! foiled again! OK, will do. cheers andrew ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] pipe chunks protocol
Andrew Dunstan <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] pipe chunks protocol
Tom Lane wrote: Andrew Dunstan <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] pipe chunks protocol
Andrew Dunstan <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] pipe chunks protocol
Tom Lane wrote: Andrew Dunstan <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] pipe chunks protocol
Andrew Dunstan <[EMAIL PROTECTED]> 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 binVOvljxL6WO.bin Description: logpipe-2.patch.gz ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] pipe chunks protocol
and here's the patch
Andrew Dunstan wrote:
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
Index: src/backend/postmaster/syslogger.c
===
RCS file: /cvsroot/pgsql/src/backend/postmaster/syslogger.c,v
retrieving revision 1.31
diff -c -r1.31 syslogger.c
*** src/backend/postmaster/syslogger.c 4 Jun 2007 22:21:42 - 1.31
--- src/backend/postmaster/syslogger.c 13 Jun 2007 15:38:07 -
***
*** 42,47
--- 42,48
#include "utils/guc.h"
#include "utils/ps_status.h"
#include "utils/timestamp.h"
+ #include "lib/stringinfo.h"
/*
* We really want line-buffered mode for logfile output, but Windows does
***
*** 54,59
--- 55,77
#define LBF_MODE _IOLBF
#endif
+ /* try not to break chunked messages into multiple reads */
+ #if PIPE_BUF > 1024
+ #define READ_SIZE PIPE_BUF
+ #else
+ #define READ_SIZE 1024
+ #endif
+
+ /*
+ * we use a buffer twice as big as a read so that if there is a fragment left
+ * after processing what is read we can save it and copy it back before the
+ * next read.
+ */
+ #define READ_BUF_SIZE 2 * READ_SIZE
+
+ /* buffer to keep any partial chunks read between calls to read()/ReadFile() */
+ static char * read_fragment[READ_SIZE];
+ static int read_fragment_len = 0;
/*
* GUC parameters. Redirect_stderr cannot be changed after postmaster
***
*** 75,89
* Private state
*/
static pg_time_t next_rotation_time;
-
static bool redirection_done = false;
-
static bool pipe_eof_seen = false;
-
static FILE *syslogFile = NULL;
-
static char *last_file_name = NULL;
/* These must be exported for EXEC_BACKEND case ... annoying */
#ifndef WIN32
int syslogPipe[2] = {-1, -1};
--- 93,117
* Private state
*/
static pg_time_t next_rotation_time;
static bool redirection_done = false;
static bool pipe_eof_seen = false;
static FILE *syslogFile = NULL;
static char *last_file_name = NULL;
+ /*
+ * buffers for saving partial messages from different backends. We don't expect
+ * that there will be very many outstanding at one time, so 20 seems plenty of
+ * leeway. If this array gets full we won't lose messages, but we will lose
+ * the protocol protection against them being partially written or interleaved.
+ */
+ typedef struct
+ {
+ pid_t pid;
+ StringInfoData data;
+ } save_buffer;
+ #define CHUNK_SLOTS 20
+ static save_buffer saved_chunks[CHUNK_SLOTS];
+
/* These must be exported for EXEC_BACKEND case ... annoying */
#ifndef WIN32
int syslogPipe[2] = {-1, -1};
***
*** 117,123
static void set_next_rotation_time(void);
static void sigHupHandler(SIGNAL_ARGS);
static void sigUsr1Handler(SIGNAL_ARGS);
!
/*
* Main entry point for syslogger process
--- 145,151
static void set_next_rotation_time(void);
static void sigHupHandler(SIGNAL_ARGS);
static void sigUsr1Handler(SIGNAL_ARGS);
! static void write_chunk(const char * buffer, int count);
/*
* Main entry point for syslogger process
***
*** 244,250
bool time_based_rotation = false;
#ifndef WIN32
! char logbuffer[1024];
int bytesRead;
int rc;
fd_set rfds;
--- 272,278
bool time_based_rotation = false;
#ifndef WIN32
! char logbuffer[READ_BUF_SIZE];
int bytesRead;
int rc;
fd_set rfds;
***
*** 325,332
}
else if (rc > 0 && FD_ISSET(syslogPipe[0], &rfds))
{
bytesRead = piperead(syslogPipe[0],
! logbuffer, sizeof(logbuffer));
if (bytesRead < 0)
{
--- 353,363
}
else if (rc > 0 && FD_ISSET(syslogPipe[0], &rfds))
{
+ Assert (read_fragment_len <= READ_SIZE);
+
+ memcpy(logbuffer, read_fragment, read_fragment_len);
bytesRead = piperead(syslogPipe[0],
! logbuffer + read_fragment_len, READ_SIZE);
if (bytesRead < 0)
{
***
*** 337,343
}
else if (bytesRead > 0)
{
! write_syslogger_file(logbuffer, bytesRead);
continue;
}
else
--- 368,374
}
else if (bytesRead > 0)
{
! write_syslogger_file(logbuffer, bytesRead + read_fragment_len);
continue;
}
else
***
[PATCHES] 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
