Fix syslog bug: if any messages are emitted to write_syslog before

the facility has been set, the facility gets set to LOCAL0 and cannot
be changed later.  This seems reasonably plausible to happen, particularly
at higher debug log levels, though I am not certain it explains Han Holl's
recent report.  Easiest fix is to teach the code how to change the value
on-the-fly, which is nicer anyway.  I made the settings PGC_SIGHUP to
conform with log_destination.

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 9e695bad576d4ab1187ecef889aa9128591b3d74..b02598a7bcda0191413888fbbdb13cfd000506b1 100644 (file)
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1,5 +1,5 @@
 <!--
-$PostgreSQL: pgsql/doc/src/sgml/config.sgml,v 1.28 2005/10/13 22:55:19 momjian Exp $
+$PostgreSQL: pgsql/doc/src/sgml/config.sgml,v 1.29 2005/10/14 20:53:55 tgl Exp $
 -->
 <chapter Id="runtime-config">
   <title>Run-time Configuration</title>
@@ -2254,7 +2254,8 @@ SELECT * FROM parent WHERE key = 2400;
         the default is <literal>LOCAL0</>. See also the
         documentation of your system's
         <application>syslog</application> daemon.
-        This option can only be set at server start.
+        This option can only be set at server start or in the
+        <filename>postgresql.conf</filename> configuration file.
        </para>
       </listitem>
      </varlistentry>
@@ -2271,7 +2272,8 @@ SELECT * FROM parent WHERE key = 2400;
          <productname>PostgreSQL</productname> messages in
          <application>syslog</application> logs. The default is
          <literal>postgres</literal>.
-          This option can only be set at server start.
+         This option can only be set at server start or in the
+         <filename>postgresql.conf</filename> configuration file.
         </para>
        </listitem>
       </varlistentry>
@@ -2581,8 +2583,8 @@ SELECT * FROM parent WHERE key = 2400;
         <varname>log_statement</> to be logged.  When using this option, 
         if you are not using <application>syslog</>, it is recommended 
         that you log the PID or session ID using <varname>log_line_prefix</> 
-        so that you can link the statement to the 
-        duration using the process ID or session ID. The default is
+        so that you can link the statement message to the later
+        duration message using the process ID or session ID. The default is
         <literal>off</>. Only superusers can change this setting.
        </para>
       </listitem>

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 07b2d007395fdfcf3500de876c7364ee17736303..d24242e8409000f60d6fea11c534ba7dd3195781 100644 (file)
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -42,7 +42,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/error/elog.c,v 1.163 2005/10/14 16:41:02 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/error/elog.c,v 1.164 2005/10/14 20:53:56 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -80,8 +80,9 @@ char     *Log_line_prefix = NULL;             /* format for extra log line info */
 int                    Log_destination = LOG_DESTINATION_STDERR;
 
 #ifdef HAVE_SYSLOG
-char      *Syslog_facility;    /* openlog() parameters */
-char      *Syslog_ident;
+static bool openlog_done = false;
+static char *syslog_ident = NULL;
+static int     syslog_facility = LOG_LOCAL0;
 
 static void write_syslog(int level, const char *line);
 #endif
@@ -1148,6 +1149,39 @@ DebugFileOpen(void)
 
 #ifdef HAVE_SYSLOG
 
+/*
+ * Set or update the parameters for syslog logging
+ */
+void
+set_syslog_parameters(const char *ident, int facility)
+{
+       /*
+        * guc.c is likely to call us repeatedly with same parameters, so
+        * don't thrash the syslog connection unnecessarily.  Also, we do not
+        * re-open the connection until needed, since this routine will get called
+        * whether or not Log_destination actually mentions syslog.
+        *
+        * Note that we make our own copy of the ident string rather than relying
+        * on guc.c's.  This may be overly paranoid, but it ensures that we cannot
+        * accidentally free a string that syslog is still using.
+        */
+       if (syslog_ident == NULL || strcmp(syslog_ident, ident) != 0 ||
+               syslog_facility != facility)
+       {
+               if (openlog_done)
+               {
+                       closelog();
+                       openlog_done = false;
+               }
+               if (syslog_ident)
+                       free(syslog_ident);
+               syslog_ident = strdup(ident);
+               /* if the strdup fails, we will cope in write_syslog() */
+               syslog_facility = facility;
+       }
+}
+
+
 #ifndef PG_SYSLOG_LIMIT
 #define PG_SYSLOG_LIMIT 128
 #endif
@@ -1158,46 +1192,16 @@ DebugFileOpen(void)
 static void
 write_syslog(int level, const char *line)
 {
-       static bool openlog_done = false;
        static unsigned long seq = 0;
 
        int                     len;
 
+       /* Open syslog connection if not done yet */
        if (!openlog_done)
        {
-               int             syslog_fac;
-               char   *syslog_ident;
-
-               if (pg_strcasecmp(Syslog_facility, "LOCAL0") == 0)
-                       syslog_fac = LOG_LOCAL0;
-               else if (pg_strcasecmp(Syslog_facility, "LOCAL1") == 0)
-                       syslog_fac = LOG_LOCAL1;
-               else if (pg_strcasecmp(Syslog_facility, "LOCAL2") == 0)
-                       syslog_fac = LOG_LOCAL2;
-               else if (pg_strcasecmp(Syslog_facility, "LOCAL3") == 0)
-                       syslog_fac = LOG_LOCAL3;
-               else if (pg_strcasecmp(Syslog_facility, "LOCAL4") == 0)
-                       syslog_fac = LOG_LOCAL4;
-               else if (pg_strcasecmp(Syslog_facility, "LOCAL5") == 0)
-                       syslog_fac = LOG_LOCAL5;
-               else if (pg_strcasecmp(Syslog_facility, "LOCAL6") == 0)
-                       syslog_fac = LOG_LOCAL6;
-               else if (pg_strcasecmp(Syslog_facility, "LOCAL7") == 0)
-                       syslog_fac = LOG_LOCAL7;
-               else
-                       syslog_fac = LOG_LOCAL0;
-               /*
-                * openlog() usually just stores the passed char pointer as-is,
-                * so we must give it a string that will be unchanged for the life of
-                * the process.  The Syslog_ident GUC variable does not meet this
-                * requirement, so strdup() it.  This isn't a memory leak because
-                * this code is executed at most once per process.
-                */
-               syslog_ident = strdup(Syslog_ident);
-               if (syslog_ident == NULL)                       /* out of memory already!? */
-                       syslog_ident = "postgres";
-
-               openlog(syslog_ident, LOG_PID | LOG_NDELAY | LOG_NOWAIT, syslog_fac);
+               openlog(syslog_ident ? syslog_ident : "postgres",
+                               LOG_PID | LOG_NDELAY | LOG_NOWAIT,
+                               syslog_facility);
                openlog_done = true;
        }
 

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 549393ce551ed554ecf5c634e8b901d3bc3ac161..1ba1ac31d3fd7fe8060bbc94807300fb3508dd8d 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -10,7 +10,7 @@
  * Written by Peter Eisentraut <peter_e@gmx.net>.
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.291 2005/10/08 20:08:19 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.292 2005/10/14 20:53:56 tgl Exp $
  *
  *--------------------------------------------------------------------
  */
@@ -21,6 +21,9 @@
 #include <limits.h>
 #include <unistd.h>
 #include <sys/stat.h>
+#ifdef HAVE_SYSLOG
+#include <syslog.h>
+#endif
 
 #include "utils/guc.h"
 #include "utils/guc_tables.h"
@@ -100,10 +103,11 @@ static const char *assign_log_destination(const char *value,
                                           bool doit, GucSource source);
 
 #ifdef HAVE_SYSLOG
-extern char *Syslog_facility;
-extern char *Syslog_ident;
+static int     syslog_facility = LOG_LOCAL0;
 
-static const char *assign_facility(const char *facility,
+static const char *assign_syslog_facility(const char *facility,
+                               bool doit, GucSource source);
+static const char *assign_syslog_ident(const char *ident,
                                bool doit, GucSource source);
 #endif
 
@@ -192,6 +196,8 @@ static char *log_error_verbosity_str;
 static char *log_statement_str;
 static char *log_min_error_statement_str;
 static char *log_destination_string;
+static char *syslog_facility_str;
+static char *syslog_ident_str;
 static bool phony_autocommit;
 static bool session_auth_is_superuser;
 static double phony_random_seed;
@@ -1964,22 +1970,22 @@ static struct config_string ConfigureNamesString[] =
 
 #ifdef HAVE_SYSLOG
        {
-               {"syslog_facility", PGC_POSTMASTER, LOGGING_WHERE,
+               {"syslog_facility", PGC_SIGHUP, LOGGING_WHERE,
                        gettext_noop("Sets the syslog \"facility\" to be used when syslog enabled."),
                        gettext_noop("Valid values are LOCAL0, LOCAL1, LOCAL2, LOCAL3, "
                                                 "LOCAL4, LOCAL5, LOCAL6, LOCAL7.")
                },
-               &Syslog_facility,
-               "LOCAL0", assign_facility, NULL
+               &syslog_facility_str,
+               "LOCAL0", assign_syslog_facility, NULL
        },
        {
-               {"syslog_ident", PGC_POSTMASTER, LOGGING_WHERE,
-                       gettext_noop("Sets the program name used to identify PostgreSQL messages "
-                                                "in syslog."),
+               {"syslog_ident", PGC_SIGHUP, LOGGING_WHERE,
+                       gettext_noop("Sets the program name used to identify PostgreSQL "
+                                                "messages in syslog."),
                        NULL
                },
-               &Syslog_ident,
-               "postgres", NULL, NULL
+               &syslog_ident_str,
+               "postgres", assign_syslog_ident, NULL
        },
 #endif
 
@@ -5552,27 +5558,49 @@ assign_log_destination(const char *value, bool doit, GucSource source)
 #ifdef HAVE_SYSLOG
 
 static const char *
-assign_facility(const char *facility, bool doit, GucSource source)
+assign_syslog_facility(const char *facility, bool doit, GucSource source)
 {
+       int             syslog_fac;
+
        if (pg_strcasecmp(facility, "LOCAL0") == 0)
-               return facility;
-       if (pg_strcasecmp(facility, "LOCAL1") == 0)
-               return facility;
-       if (pg_strcasecmp(facility, "LOCAL2") == 0)
-               return facility;
-       if (pg_strcasecmp(facility, "LOCAL3") == 0)
-               return facility;
-       if (pg_strcasecmp(facility, "LOCAL4") == 0)
-               return facility;
-       if (pg_strcasecmp(facility, "LOCAL5") == 0)
-               return facility;
-       if (pg_strcasecmp(facility, "LOCAL6") == 0)
-               return facility;
-       if (pg_strcasecmp(facility, "LOCAL7") == 0)
-               return facility;
-       return NULL;
+               syslog_fac = LOG_LOCAL0;
+       else if (pg_strcasecmp(facility, "LOCAL1") == 0)
+               syslog_fac = LOG_LOCAL1;
+       else if (pg_strcasecmp(facility, "LOCAL2") == 0)
+               syslog_fac = LOG_LOCAL2;
+       else if (pg_strcasecmp(facility, "LOCAL3") == 0)
+               syslog_fac = LOG_LOCAL3;
+       else if (pg_strcasecmp(facility, "LOCAL4") == 0)
+               syslog_fac = LOG_LOCAL4;
+       else if (pg_strcasecmp(facility, "LOCAL5") == 0)
+               syslog_fac = LOG_LOCAL5;
+       else if (pg_strcasecmp(facility, "LOCAL6") == 0)
+               syslog_fac = LOG_LOCAL6;
+       else if (pg_strcasecmp(facility, "LOCAL7") == 0)
+               syslog_fac = LOG_LOCAL7;
+       else
+               return NULL;                    /* reject */
+
+       if (doit)
+       {
+               syslog_facility = syslog_fac;
+               set_syslog_parameters(syslog_ident_str ? syslog_ident_str : "postgres",
+                                                         syslog_facility);
+       }
+
+       return facility;
 }
-#endif
+
+static const char *
+assign_syslog_ident(const char *ident, bool doit, GucSource source)
+{
+       if (doit)
+               set_syslog_parameters(ident, syslog_facility);
+
+       return ident;
+}
+
+#endif /* HAVE_SYSLOG */
 
 
 static const char *

diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index f226f772911a04b024b82de54742ee381c19b2e7..264dfcc1429c540651a67ad6da922b72f84c7883 100644 (file)
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/utils/elog.h,v 1.79 2005/06/10 16:23:10 neilc Exp $
+ * $PostgreSQL: pgsql/src/include/utils/elog.h,v 1.80 2005/10/14 20:53:56 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -283,6 +283,9 @@ extern int  Log_destination;
 /* Other exported functions */
 extern void DebugFileOpen(void);
 extern char *unpack_sql_state(int sql_state);
+#ifdef HAVE_SYSLOG
+extern void set_syslog_parameters(const char *ident, int facility);
+#endif
 
 /*
  * Write errors to stderr (or by equal means when stderr is