Attached is a very much VIP (AKA rough draft) for $subject.
Right now we have two parameters controlling logging destination, and they
work in interesting combinations:
log_destination=stderr, logging_collector=off -> log to stderr (makes sense)
log_destination=stderr, logging_collector=on -> log to file (*highly*
illogical for most users, to set stderr when they want file)
log_destination=csvlog, logging_collector=on -> log to csvfile (makes sense)
log_destination=csvlog, logging_collector=off -> fail (ugh)
(beyond that we also have syslog and eventlog, but those are different and
not touched by this patch)
My understanding is that the main reason for this is that we cannot change
logging_collector without restarting postmaster, whereas we can change
log_destination.
My suggestion is we work around this by just always starting the logging
collector, even if we're not planning to use it. Then we'd just have:
log_destination = stderr -> log to stderr
log_destination = file -> log to file
log_destination = csvlog -> log to csv file
The main difference here is that log_destination=stderr would also go
through the logging collector which would then assemble it and write it out
to it's own stderr.
Do people see an actual problem with that? I agree it's an extra round of
indirection there, but is that a problem? It would also cause one more
backgorund process to always be running, but again, is that really a
problem? The overhead is not exactly large.
It would make configuration a lot more logical for logging. It would also
make it possible to switch between all logging configurations without
restarting.
The attached WIP is mostly working for the simple cases. It fails
completely if the syslogger is restarted at this point, simply because I
haven't figured out how to pass the original stderr down to the syslogger.
I'm sure there are also many other smaller issues around it, but I wanted
to get the discussion done before I spend the time to go through those.
Thoughts?
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 95180b2ef5..7c4eb5743f 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1764,7 +1764,7 @@ ServerLoop(void)
}
/* If we have lost the log collector, try to start a new one */
- if (SysLoggerPID == 0 && Logging_collector)
+ if (SysLoggerPID == 0)
SysLoggerPID = SysLogger_Start();
/*
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 3255b42c7d..15fc606d24 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -57,10 +57,8 @@
/*
- * GUC parameters. Logging_collector cannot be changed after postmaster
- * start, but the rest can change at SIGHUP.
+ * GUC parameters. Can change at SIGHUP.
*/
-bool Logging_collector = false;
int Log_RotationAge = HOURS_PER_DAY * MINS_PER_HOUR;
int Log_RotationSize = 10 * 1024;
char *Log_directory = NULL;
@@ -182,6 +180,10 @@ SysLoggerMain(int argc, char *argv[])
* assumes that all interesting messages generated in the syslogger will
* come through elog.c and will be sent to write_syslogger_file.
*/
+ /*
+ * XXX: this does not work when we want to use stderr in the syslogger.
+ * needs fixing!
+ */
if (redirection_done)
{
int fd = open(DEVNULL, O_WRONLY, 0);
@@ -370,10 +372,11 @@ SysLoggerMain(int argc, char *argv[])
if (!rotation_requested && Log_RotationSize > 0 && !rotation_disabled)
{
/* Do a rotation if file is too big */
- if (ftell(syslogFile) >= Log_RotationSize * 1024L)
+ if (syslogFile != NULL &&
+ ftell(syslogFile) >= Log_RotationSize * 1024L)
{
rotation_requested = true;
- size_rotation_for |= LOG_DESTINATION_STDERR;
+ size_rotation_for |= LOG_DESTINATION_FILE;
}
if (csvlogFile != NULL &&
ftell(csvlogFile) >= Log_RotationSize * 1024L)
@@ -390,7 +393,7 @@ SysLoggerMain(int argc, char *argv[])
* was sent by pg_rotate_logfile.
*/
if (!time_based_rotation && size_rotation_for == 0)
- size_rotation_for = LOG_DESTINATION_STDERR | LOG_DESTINATION_CSVLOG;
+ size_rotation_for = LOG_DESTINATION_FILE | LOG_DESTINATION_CSVLOG;
logfile_rotate(time_based_rotation, size_rotation_for);
}
@@ -522,9 +525,6 @@ SysLogger_Start(void)
pid_t sysloggerPid;
char *filename;
- if (!Logging_collector)
- return 0;
-
/*
* If first time through, create the pipe which will receive stderr
* output.
@@ -581,7 +581,10 @@ SysLogger_Start(void)
first_syslogger_file_time = time(NULL);
filename = logfile_getname(first_syslogger_file_time, NULL);
- syslogFile = logfile_open(filename, "a", false);
+ if (Log_destination & LOG_DESTINATION_FILE)
+ syslogFile = logfile_open(filename, "a", false);
+ else
+ syslogFile = NULL;
pfree(filename);
@@ -675,7 +678,8 @@ SysLogger_Start(void)
}
/* postmaster will never write the file; close it */
- fclose(syslogFile);
+ if (syslogFile != NULL)
+ fclose(syslogFile);
syslogFile = NULL;
return (int) sysloggerPid;
}
@@ -794,7 +798,7 @@ process_pipe_input(char *logbuffer, int *bytes_in_logbuffer)
{
char *cursor = logbuffer;
int count = *bytes_in_logbuffer;
- int dest = LOG_DESTINATION_STDERR;
+ bool dest_csv = false;
/* While we have enough for a header, process data... */
while (count >= (int) (offsetof(PipeProtoHeader, data) + 1))
@@ -822,8 +826,7 @@ process_pipe_input(char *logbuffer, int *bytes_in_logbuffer)
if (count < chunklen)
break;
- dest = (p.is_last == 'T' || p.is_last == 'F') ?
- LOG_DESTINATION_CSVLOG : LOG_DESTINATION_STDERR;
+ dest_csv = (p.is_last == 'T' || p.is_last == 'F');
/* Locate any existing buffer for this source pid */
buffer_list = buffer_lists[p.pid % NBUFFER_LISTS];
@@ -886,7 +889,7 @@ process_pipe_input(char *logbuffer, int *bytes_in_logbuffer)
appendBinaryStringInfo(str,
cursor + PIPE_HEADER_SIZE,
p.len);
- write_syslogger_file(str->data, str->len, dest);
+ write_syslogger_file(str->data, str->len, dest_csv);
/* Mark the buffer unused, and reclaim string storage */
existing_slot->pid = 0;
pfree(str->data);
@@ -895,7 +898,7 @@ process_pipe_input(char *logbuffer, int *bytes_in_logbuffer)
{
/* The whole message was one chunk, evidently. */
write_syslogger_file(cursor + PIPE_HEADER_SIZE, p.len,
- dest);
+ dest_csv);
}
}
@@ -922,7 +925,7 @@ process_pipe_input(char *logbuffer, int *bytes_in_logbuffer)
break;
}
/* fall back on the stderr log as the destination */
- write_syslogger_file(cursor, chunklen, LOG_DESTINATION_STDERR);
+ write_syslogger_file(cursor, chunklen, false);
cursor += chunklen;
count -= chunklen;
}
@@ -960,7 +963,7 @@ flush_pipe_input(char *logbuffer, int *bytes_in_logbuffer)
StringInfo str = &(buf->data);
write_syslogger_file(str->data, str->len,
- LOG_DESTINATION_STDERR);
+ LOG_DESTINATION_FILE);
/* Mark the buffer unused, and reclaim string storage */
buf->pid = 0;
pfree(str->data);
@@ -974,7 +977,7 @@ flush_pipe_input(char *logbuffer, int *bytes_in_logbuffer)
*/
if (*bytes_in_logbuffer > 0)
write_syslogger_file(logbuffer, *bytes_in_logbuffer,
- LOG_DESTINATION_STDERR);
+ LOG_DESTINATION_FILE);
*bytes_in_logbuffer = 0;
}
@@ -985,23 +988,39 @@ flush_pipe_input(char *logbuffer, int *bytes_in_logbuffer)
*/
/*
- * Write text to the currently open logfile
+ * Write text to the currently open logfile and/or stderr.
*
* This is exported so that elog.c can call it when am_syslogger is true.
* This allows the syslogger process to record elog messages of its own,
* even though its stderr does not point at the syslog pipe.
*/
void
-write_syslogger_file(const char *buffer, int count, int destination)
+write_syslogger_file(const char *buffer, int count, bool dest_csv)
{
- int rc;
- FILE *logfile;
-
- if (destination == LOG_DESTINATION_CSVLOG && csvlogFile == NULL)
- open_csvlogfile();
+ int rc = 0;
- logfile = destination == LOG_DESTINATION_CSVLOG ? csvlogFile : syslogFile;
- rc = fwrite(buffer, 1, count, logfile);
+ if (dest_csv)
+ {
+ if (csvlogFile == NULL)
+ open_csvlogfile();
+ rc = fwrite(buffer, 1, count, csvlogFile);
+ }
+ else
+ {
+ /* non-csv can go to file, stderr or both */
+ if (Log_destination & LOG_DESTINATION_STDERR)
+ {
+ rc = fwrite(buffer, 1, count, stderr);
+ /*
+ * if we fail to write to stderr, we're not likely to be able
+ * to write the error message about it either, so just
+ * ignore and let the result be overwritten if we log to both
+ * file and stderr.
+ */
+ }
+ if (Log_destination & LOG_DESTINATION_FILE)
+ rc = fwrite(buffer, 1, count, syslogFile);
+ }
/* can't use ereport here because of possible recursion */
if (rc != count)
@@ -1064,7 +1083,7 @@ pipeThread(void *arg)
*/
if (Log_RotationSize > 0)
{
- if (ftell(syslogFile) >= Log_RotationSize * 1024L ||
+ if ((syslogFile != NULL && ftell(syslogFile) >= Log_RotationSize * 1024L ||
(csvlogFile != NULL && ftell(csvlogFile) >= Log_RotationSize * 1024L))
SetLatch(MyLatch);
}
@@ -1189,7 +1208,7 @@ logfile_rotate(bool time_based_rotation, int size_rotation_for)
*
* Note: last_file_name should never be NULL here, but if it is, append.
*/
- if (time_based_rotation || (size_rotation_for & LOG_DESTINATION_STDERR))
+ if (time_based_rotation || (size_rotation_for & LOG_DESTINATION_FILE))
{
if (Log_truncate_on_rotation && time_based_rotation &&
last_file_name != NULL &&
@@ -1220,7 +1239,8 @@ logfile_rotate(bool time_based_rotation, int size_rotation_for)
return;
}
- fclose(syslogFile);
+ if (syslogFile != NULL)
+ fclose(syslogFile);
syslogFile = fh;
/* instead of pfree'ing filename, remember it for next time */
@@ -1362,7 +1382,7 @@ update_metainfo_datafile(void)
{
FILE *fh;
- if (!(Log_destination & LOG_DESTINATION_STDERR) &&
+ if (!(Log_destination & LOG_DESTINATION_FILE) &&
!(Log_destination & LOG_DESTINATION_CSVLOG))
{
if (unlink(LOG_METAINFO_DATAFILE) < 0 && errno != ENOENT)
@@ -1382,7 +1402,7 @@ update_metainfo_datafile(void)
return;
}
- if (last_file_name && (Log_destination & LOG_DESTINATION_STDERR))
+ if (last_file_name && (Log_destination & LOG_DESTINATION_FILE))
{
if (fprintf(fh, "stderr %s\n", last_file_name) < 0)
{
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 62341b84d1..5e7df05e01 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -349,13 +349,6 @@ pg_reload_conf(PG_FUNCTION_ARGS)
Datum
pg_rotate_logfile(PG_FUNCTION_ARGS)
{
- if (!Logging_collector)
- {
- ereport(WARNING,
- (errmsg("rotation not possible because log collection not active")));
- PG_RETURN_BOOL(false);
- }
-
SendPostmasterSignal(PMSIGNAL_ROTATE_LOGFILE);
PG_RETURN_BOOL(true);
}
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 918db0a8f2..2c23914b82 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -175,7 +175,7 @@ static const char *process_log_prefix_padding(const char *p, int *padding);
static void log_line_prefix(StringInfo buf, ErrorData *edata);
static void write_csvlog(ErrorData *edata);
static void send_message_to_server_log(ErrorData *edata);
-static void write_pipe_chunks(char *data, int len, int dest);
+static void write_pipe_chunks(char *data, int len, bool csv);
static void send_message_to_frontend(ErrorData *edata);
static char *expand_fmt_string(const char *fmt, ErrorData *edata);
static const char *useful_strerror(int errnum);
@@ -2831,7 +2831,7 @@ write_csvlog(ErrorData *edata)
if (am_syslogger)
write_syslogger_file(buf.data, buf.len, LOG_DESTINATION_CSVLOG);
else
- write_pipe_chunks(buf.data, buf.len, LOG_DESTINATION_CSVLOG);
+ write_pipe_chunks(buf.data, buf.len, true);
pfree(buf.data);
}
@@ -3008,7 +3008,7 @@ send_message_to_server_log(ErrorData *edata)
#endif /* WIN32 */
/* Write to stderr, if enabled */
- if ((Log_destination & LOG_DESTINATION_STDERR) || whereToSendOutput == DestDebug)
+ if ((Log_destination & (LOG_DESTINATION_STDERR | LOG_DESTINATION_FILE)) || whereToSendOutput == DestDebug)
{
/*
* Use the chunking protocol if we know the syslogger should be
@@ -3016,7 +3016,7 @@ send_message_to_server_log(ErrorData *edata)
* Otherwise, just do a vanilla write to stderr.
*/
if (redirection_done && !am_syslogger)
- write_pipe_chunks(buf.data, buf.len, LOG_DESTINATION_STDERR);
+ write_pipe_chunks(buf.data, buf.len, false);
#ifdef WIN32
/*
@@ -3088,7 +3088,7 @@ send_message_to_server_log(ErrorData *edata)
* rc to void to shut up the compiler.
*/
static void
-write_pipe_chunks(char *data, int len, int dest)
+write_pipe_chunks(char *data, int len, bool csv)
{
PipeProtoChunk p;
int fd = fileno(stderr);
@@ -3102,7 +3102,7 @@ write_pipe_chunks(char *data, int len, int dest)
/* write all but the last chunk */
while (len > PIPE_MAX_PAYLOAD)
{
- p.proto.is_last = (dest == LOG_DESTINATION_CSVLOG ? 'F' : 'f');
+ p.proto.is_last = (csv ? 'F' : 'f');
p.proto.len = PIPE_MAX_PAYLOAD;
memcpy(p.proto.data, data, PIPE_MAX_PAYLOAD);
rc = write(fd, &p, PIPE_HEADER_SIZE + PIPE_MAX_PAYLOAD);
@@ -3112,7 +3112,7 @@ write_pipe_chunks(char *data, int len, int dest)
}
/* write the last chunk */
- p.proto.is_last = (dest == LOG_DESTINATION_CSVLOG ? 'T' : 't');
+ p.proto.is_last = (csv ? 'T' : 't');
p.proto.len = len;
memcpy(p.proto.data, data, len);
rc = write(fd, &p, PIPE_HEADER_SIZE + len);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 246fea8693..26ae6bf296 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1436,15 +1436,6 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
- {"logging_collector", PGC_POSTMASTER, LOGGING_WHERE,
- gettext_noop("Start a subprocess to capture stderr output and/or csvlogs into log files."),
- NULL
- },
- &Logging_collector,
- false,
- NULL, NULL, NULL
- },
- {
{"log_truncate_on_rotation", PGC_SIGHUP, LOGGING_WHERE,
gettext_noop("Truncate existing log files of same name during log rotation."),
NULL
@@ -3325,7 +3316,7 @@ static struct config_string ConfigureNamesString[] =
{"log_destination", PGC_SIGHUP, LOGGING_WHERE,
gettext_noop("Sets the destination for server log output."),
gettext_noop("Valid values are combinations of \"stderr\", "
- "\"syslog\", \"csvlog\", and \"eventlog\", "
+ "\"file\", \"syslog\", \"csvlog\", and \"eventlog\", "
"depending on the platform."),
GUC_LIST_INPUT
},
@@ -10077,6 +10068,8 @@ check_log_destination(char **newval, void **extra, GucSource source)
if (pg_strcasecmp(tok, "stderr") == 0)
newlogdest |= LOG_DESTINATION_STDERR;
+ else if (pg_strcasecmp(tok, "file") == 0)
+ newlogdest |= LOG_DESTINATION_FILE;
else if (pg_strcasecmp(tok, "csvlog") == 0)
newlogdest |= LOG_DESTINATION_CSVLOG;
#ifdef HAVE_SYSLOG
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index df5d2f3f22..58f899c097 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -342,17 +342,10 @@
# - Where to Log -
#log_destination = 'stderr' # Valid values are combinations of
- # stderr, csvlog, syslog, and eventlog,
- # depending on platform. csvlog
- # requires logging_collector to be on.
-
-# This is used when logging to stderr:
-#logging_collector = off # Enable capturing of stderr and csvlog
- # into log files. Required to be on for
- # csvlogs.
- # (change requires restart)
+ # stderr, file, csvlog, syslog, and
+ # eventlog, depending on platform.
-# These are only used if logging_collector is on:
+# These are only used if log_destination is file or csvlog
#log_directory = 'log' # directory where log files are written,
# can be absolute or relative to PGDATA
#log_filename = 'postgresql-%Y-%m-%d_%H%M%S.log' # log file name pattern,
diff --git a/src/include/postmaster/syslogger.h b/src/include/postmaster/syslogger.h
index f4248ef5d4..82eafc0241 100644
--- a/src/include/postmaster/syslogger.h
+++ b/src/include/postmaster/syslogger.h
@@ -62,7 +62,6 @@ typedef union
/* GUC options */
-extern bool Logging_collector;
extern int Log_RotationAge;
extern int Log_RotationSize;
extern PGDLLIMPORT char *Log_directory;
@@ -81,7 +80,7 @@ extern HANDLE syslogPipe[2];
extern int SysLogger_Start(void);
-extern void write_syslogger_file(const char *buffer, int count, int dest);
+extern void write_syslogger_file(const char *buffer, int count, bool dest_csv);
#ifdef EXEC_BACKEND
extern void SysLoggerMain(int argc, char *argv[]) pg_attribute_noreturn();
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 7bfd25a9e9..49dc7ea9e5 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -396,6 +396,7 @@ extern bool syslog_split_messages;
#define LOG_DESTINATION_SYSLOG 2
#define LOG_DESTINATION_EVENTLOG 4
#define LOG_DESTINATION_CSVLOG 8
+#define LOG_DESTINATION_FILE 16
/* Other exported functions */
extern void DebugFileOpen(void);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
