Re: log files and permissions - Mailing list pgsql-hackers

Itagaki Takahiro wrote:
> I checked "log_file_mode GUC" patch, and found a couple of Windows-specific
> and translation issues.

Thank you for the review. Attached patch attempts to fix these issues.

> * fchmod() is not available on some platforms, including Windows.
>     fh = fopen(filename, mode);
>     setvbuf(fh, NULL, LBF_MODE, 0);
>     fchmod(fileno(fh), Log_file_mode);
>

I've changed that to using chmod(), that should be available everywhere and
is already used in several places in Postgres code.

> * How does the file mode work on Windows?
> If it doesn't work, we should explain it in docs.

Indeed it seems that chmod() doesn't actually do anything useful on Windows.
So I've added a documentation note about it and put an #ifndef WIN32 around
the chmod() call.

> It might look a duplication of codes, but I think this form is better
> because we can reuse the existing translation catalogs.
>     if (am_rotating)
>         ereport(FATAL, ... "could not create log file ...);
>     else
>         ereport(LOG, ... "could not open new log file ...);
>

Fixed.

regards,
Martin

*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 2789,2794 **** local0.*    /var/log/postgresql
--- 2789,2814 ----
        </listitem>
       </varlistentry>

+      <varlistentry id="guc-log-file-mode" xreflabel="log_file_mode">
+       <term><varname>log_file_mode</varname> (<type>integer</type>)</term>
+       <indexterm>
+        <primary><varname>log_file_mode</> configuration parameter</primary>
+       </indexterm>
+       <listitem>
+        <para>
+         On Unix systems this parameter sets the permissions for log files
+         when <varname>logging_collector</varname> is enabled. On Microsoft
+         Windows the file mode will be ignored. The value is an octal number
+         consisting of 3 digits signifying the permissions for the user, group
+         and others.
+        </para>
+        <para>
+         This parameter can only be set in the <filename>postgresql.conf</>
+         file or on the server command line.
+        </para>
+       </listitem>
+      </varlistentry>
+
       <varlistentry id="guc-log-rotation-age" xreflabel="log_rotation_age">
        <term><varname>log_rotation_age</varname> (<type>integer</type>)</term>
        <indexterm>
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
***************
*** 914,916 **** show_role(void)
--- 914,946 ----

      return endptr + 1;
  }
+
+
+ /*
+  * LOG_FILE_MODE
+  */
+
+ extern int Log_file_mode;        /* in guc.c */
+
+ /*
+  * assign_log_file_mode: GUC assign_hook for log_file_mode
+  */
+ const char *
+ assign_log_file_mode(const char *value, bool doit, GucSource source)
+ {
+     char *endptr;
+     long file_mode = strtol(value, &endptr, 8);
+
+     if (!*value || *endptr || file_mode < 0 || file_mode > 0777)
+     {
+         ereport(GUC_complaint_elevel(source),
+                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                  errmsg("invalid value for parameter \"log_file_mode\"")));
+         return NULL;
+     }
+
+     if (doit)
+         Log_file_mode = (int) file_mode;
+
+     return value;
+ }
*** a/src/backend/postmaster/syslogger.c
--- b/src/backend/postmaster/syslogger.c
***************
*** 73,78 **** int            Log_RotationSize = 10 * 1024;
--- 73,79 ----
  char       *Log_directory = NULL;
  char       *Log_filename = NULL;
  bool        Log_truncate_on_rotation = false;
+ int            Log_file_mode = 0600;

  /*
   * Globally visible state (used by elog.c)
***************
*** 135,140 **** static void syslogger_parseArgs(int argc, char *argv[]);
--- 136,142 ----
  static void process_pipe_input(char *logbuffer, int *bytes_in_logbuffer);
  static void flush_pipe_input(char *logbuffer, int *bytes_in_logbuffer);
  static void open_csvlogfile(void);
+ static FILE *logfile_open(const char *filename, const char *mode, bool am_rotating);

  #ifdef WIN32
  static unsigned int __stdcall pipeThread(void *arg);
***************
*** 516,530 **** SysLogger_Start(void)
       */
      filename = logfile_getname(time(NULL), NULL);

!     syslogFile = fopen(filename, "a");
!
!     if (!syslogFile)
!         ereport(FATAL,
!                 (errcode_for_file_access(),
!                  (errmsg("could not create log file \"%s\": %m",
!                          filename))));
!
!     setvbuf(syslogFile, NULL, LBF_MODE, 0);

      pfree(filename);

--- 518,524 ----
       */
      filename = logfile_getname(time(NULL), NULL);

!     syslogFile = logfile_open(filename, "a", false);

      pfree(filename);

***************
*** 1004,1018 **** open_csvlogfile(void)

      filename = logfile_getname(time(NULL), ".csv");

!     fh = fopen(filename, "a");
!
!     if (!fh)
!         ereport(FATAL,
!                 (errcode_for_file_access(),
!                  (errmsg("could not create log file \"%s\": %m",
!                          filename))));
!
!     setvbuf(fh, NULL, LBF_MODE, 0);

  #ifdef WIN32
      _setmode(_fileno(fh), _O_TEXT);        /* use CRLF line endings on Windows */
--- 998,1004 ----

      filename = logfile_getname(time(NULL), ".csv");

!     fh = logfile_open(filename, "a", false);

  #ifdef WIN32
      _setmode(_fileno(fh), _O_TEXT);        /* use CRLF line endings on Windows */
***************
*** 1025,1030 **** open_csvlogfile(void)
--- 1011,1050 ----
  }

  /*
+  * Open the logfile, set permissions and buffering options.
+  */
+ static FILE *
+ logfile_open(const char *filename, const char *mode, bool am_rotating)
+ {
+     FILE   *fh;
+
+     fh = fopen(filename, mode);
+
+     if (fh)
+     {
+         setvbuf(fh, NULL, LBF_MODE, 0);
+ #ifndef WIN32
+         chmod(filename, Log_file_mode);
+ #endif
+     }
+     else
+     {
+         if (am_rotating)
+             ereport(LOG,
+                     (errcode_for_file_access(),
+                      errmsg("could not create new log file \"%s\": %m",
+                              filename)));
+         else
+             ereport(FATAL,
+                     (errcode_for_file_access(),
+                      errmsg("could not create log file \"%s\": %m",
+                              filename)));
+     }
+
+     return fh;
+ }
+
+ /*
   * perform logfile rotation
   */
  static void
***************
*** 1070,1088 **** logfile_rotate(bool time_based_rotation, int size_rotation_for)
          if (Log_truncate_on_rotation && time_based_rotation &&
              last_file_name != NULL &&
              strcmp(filename, last_file_name) != 0)
!             fh = fopen(filename, "w");
          else
!             fh = fopen(filename, "a");

          if (!fh)
          {
              int            saveerrno = errno;

-             ereport(LOG,
-                     (errcode_for_file_access(),
-                      errmsg("could not open new log file \"%s\": %m",
-                             filename)));
-
              /*
               * ENFILE/EMFILE are not too surprising on a busy system; just
               * keep using the old file till we manage to get a new one.
--- 1090,1103 ----
          if (Log_truncate_on_rotation && time_based_rotation &&
              last_file_name != NULL &&
              strcmp(filename, last_file_name) != 0)
!             fh = logfile_open(filename, "w", true);
          else
!             fh = logfile_open(filename, "a", true);

          if (!fh)
          {
              int            saveerrno = errno;

              /*
               * ENFILE/EMFILE are not too surprising on a busy system; just
               * keep using the old file till we manage to get a new one.
***************
*** 1128,1146 **** logfile_rotate(bool time_based_rotation, int size_rotation_for)
          if (Log_truncate_on_rotation && time_based_rotation &&
              last_csv_file_name != NULL &&
              strcmp(csvfilename, last_csv_file_name) != 0)
!             fh = fopen(csvfilename, "w");
          else
!             fh = fopen(csvfilename, "a");

          if (!fh)
          {
              int            saveerrno = errno;

-             ereport(LOG,
-                     (errcode_for_file_access(),
-                      errmsg("could not open new log file \"%s\": %m",
-                             csvfilename)));
-
              /*
               * ENFILE/EMFILE are not too surprising on a busy system; just
               * keep using the old file till we manage to get a new one.
--- 1143,1156 ----
          if (Log_truncate_on_rotation && time_based_rotation &&
              last_csv_file_name != NULL &&
              strcmp(csvfilename, last_csv_file_name) != 0)
!             fh = logfile_open(csvfilename, "w", true);
          else
!             fh = logfile_open(csvfilename, "a", true);

          if (!fh)
          {
              int            saveerrno = errno;

              /*
               * ENFILE/EMFILE are not too surprising on a busy system; just
               * keep using the old file till we manage to get a new one.
***************
*** 1162,1169 **** logfile_rotate(bool time_based_rotation, int size_rotation_for)
              return;
          }

-         setvbuf(fh, NULL, LBF_MODE, 0);
-
  #ifdef WIN32
          _setmode(_fileno(fh), _O_TEXT); /* use CRLF line endings on Windows */
  #endif
--- 1172,1177 ----
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 412,417 **** static char *server_version_string;
--- 412,418 ----
  static int    server_version_num;
  static char *timezone_string;
  static char *log_timezone_string;
+ static char *log_file_mode_string;
  static char *timezone_abbreviations_string;
  static char *XactIsoLevel_string;
  static char *data_directory;
***************
*** 2243,2248 **** static struct config_string ConfigureNamesString[] =
--- 2244,2258 ----
      },

      {
+         {"log_file_mode", PGC_SIGHUP, LOGGING_WHERE,
+             gettext_noop("Sets the file permissions for log files."),
+             NULL
+         },
+         &log_file_mode_string,
+         "0600", assign_log_file_mode
+     },
+
+     {
          {"DateStyle", PGC_USERSET, CLIENT_CONN_LOCALE,
              gettext_noop("Sets the display format for date and time values."),
              gettext_noop("Also controls interpretation of ambiguous "
*** a/src/backend/utils/misc/postgresql.conf.sample
--- b/src/backend/utils/misc/postgresql.conf.sample
***************
*** 265,270 ****
--- 265,271 ----
                      # can be absolute or relative to PGDATA
  #log_filename = 'postgresql-%Y-%m-%d_%H%M%S.log'    # log file name pattern,
                      # can include strftime() escapes
+ #log_file_mode = 0600            # creation mode for log files, octal
  #log_truncate_on_rotation = off        # If on, an existing log file of the
                      # same name as the new log file will be
                      # truncated rather than appended to.
*** a/src/include/commands/variable.h
--- b/src/include/commands/variable.h
***************
*** 35,39 **** extern const char *show_role(void);
--- 35,42 ----
  extern const char *assign_session_authorization(const char *value,
                               bool doit, GucSource source);
  extern const char *show_session_authorization(void);
+ const char *assign_log_file_mode(const char *value,
+                             bool doit, GucSource source);
+ const char *show_log_file_mode(void);

  #endif   /* VARIABLE_H */
*** a/src/include/postmaster/syslogger.h
--- b/src/include/postmaster/syslogger.h
***************
*** 68,73 **** extern int    Log_RotationSize;
--- 68,74 ----
  extern PGDLLIMPORT char *Log_directory;
  extern PGDLLIMPORT char *Log_filename;
  extern bool Log_truncate_on_rotation;
+ extern int Log_file_mode;

  extern bool am_syslogger;