Thread: log files and permissions

log files and permissions

From
Martin Pihlak
Date:
With logging_collector enabled, all the postgres log files are created with
mode 0600.  This makes life complicated if users other than "postgres" need
to be able to examine the log files as well. Common example of this is when the
database runs under "postgres" user and DBA-s have named accounts. In order to
examine the log files the DBA then has to go through extra steps to sudo to
"postgres" or equivalent. Another example would be a monitoring script that
runs as an unprivileged user but needs to tail the log files.

It'd be convenient if the log files would have group read access. Then we could
make all the DBA or monitoring users members of the postgres group and they'd
have direct access to the logs. However, as the "group read" is not likely a
universally correct setting, the creation mode needs to be configurable.

Attached is a patch that adds a GUC "log_file_mode" which allows to specify
the creation mode for the log files. Presently it lacks documentation, which
I'll add if the idea is generally acceptable.

PS. I have no idea how all of this would work on Windows, maybe it's not
event relevant there?

regards,
Martin


Re: log files and permissions

From
Martin Pihlak
Date:
Martin Pihlak wrote:
> Attached is a patch that adds a GUC "log_file_mode" which allows to specify
> the creation mode for the log files. Presently it lacks documentation, which
> I'll add if the idea is generally acceptable.
>

Now it really is attached.

regards,
Martin

*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
***************
*** 914,916 **** show_role(void)
--- 914,947 ----

      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)
+ {
+     int file_mode;
+
+     /* Parse the octal mode, complain if invalid */
+     if (sscanf(value, "%o", &file_mode) != 1 || 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 = 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 die_on_error);

  #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", true);

      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", true);

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

  /*
+  * Open the logfile, set permissions and buffering options.
+  */
+ static FILE *
+ logfile_open(const char *filename, const char *mode, bool die_on_error)
+ {
+     FILE   *fh;
+
+     fh = fopen(filename, mode);
+
+     if (fh)
+     {
+         setvbuf(fh, NULL, LBF_MODE, 0);
+         fchmod(fileno(fh), Log_file_mode);
+     }
+     else
+         ereport(die_on_error ? FATAL : LOG,
+                 (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.
--- 1080,1093 ----
          if (Log_truncate_on_rotation && time_based_rotation &&
              last_file_name != NULL &&
              strcmp(filename, last_file_name) != 0)
!             fh = logfile_open(filename, "w", false);
          else
!             fh = logfile_open(filename, "a", false);

          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.
--- 1133,1146 ----
          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", false);
          else
!             fh = logfile_open(csvfilename, "a", false);

          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
--- 1162,1167 ----
*** 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;


Re: log files and permissions

From
Tom Lane
Date:
Martin Pihlak <martin.pihlak@gmail.com> writes:
> It'd be convenient if the log files would have group read access. Then we could
> make all the DBA or monitoring users members of the postgres group and they'd
> have direct access to the logs. However, as the "group read" is not likely a
> universally correct setting, the creation mode needs to be configurable.

It doesn't appear to me that this helps unless you are willing to make
the containing director(ies) group-readable/executable as well, which is
something we've resisted doing.
        regards, tom lane


Re: log files and permissions

From
"Kevin Grittner"
Date:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Martin Pihlak <martin.pihlak@gmail.com> writes:
>> It'd be convenient if the log files would have group read access.
>> Then we could make all the DBA or monitoring users members of the
>> postgres group and they'd have direct access to the logs.
>> However, as the "group read" is not likely a universally correct
>> setting, the creation mode needs to be configurable.
> 
> It doesn't appear to me that this helps unless you are willing to
> make the containing director(ies) group-readable/executable as
> well, which is something we've resisted doing.
I just tried creating a symbolic link to the pg_log directory and
flagging the existing logs within it to 640.  As a member of the
group I was able to list and view the contents of log files through
the symbolic link, even though I didn't have any authority to the
PostgreSQL data directory.
That seems potentially useful to me.
-Kevin


Re: log files and permissions

From
Martin Pihlak
Date:
Tom Lane wrote:
> It doesn't appear to me that this helps unless you are willing to make
> the containing director(ies) group-readable/executable as well, which is
> something we've resisted doing.
> 

The log can be moved outside of data directory by setting "log_directory"
to an absolute path. Then the permissions for the log directory can be arbitrary
as the postmaster is only strict about permissions on data directory.

regards,
Martin


Re: log files and permissions

From
Stephen Frost
Date:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Martin Pihlak <martin.pihlak@gmail.com> writes:
> > It'd be convenient if the log files would have group read access. Then we could
> > make all the DBA or monitoring users members of the postgres group and they'd
> > have direct access to the logs. However, as the "group read" is not likely a
> > universally correct setting, the creation mode needs to be configurable.
>
> It doesn't appear to me that this helps unless you are willing to make
> the containing director(ies) group-readable/executable as well, which is
> something we've resisted doing.

Perhaps we should have a umask-like GUC instead of this?

In the end, I agree with and completely understand the OP's complaint.
I havn't run into this issue much since, on Debian systems, we use
logrotate to move log files around and use the copy/truncate method
there, so permissions end up being preserved once an admin has decided
to change them.  Might be something to consider, but, really, we should
give the admin some flexibility here, even if the default is the same as
current behaviour.

I'll refrain from bringing up the fact that we're concerned about log
files having group permissions by default, but we ship with "trust" in
pg_hba.conf...
Thanks,
    Stephen

Re: log files and permissions

From
Tom Lane
Date:
Martin Pihlak <martin.pihlak@gmail.com> writes:
> Tom Lane wrote:
>> It doesn't appear to me that this helps unless you are willing to make
>> the containing director(ies) group-readable/executable as well, which is
>> something we've resisted doing.

> The log can be moved outside of data directory by setting "log_directory"
> to an absolute path.

Oh, of course.  We'd need to mention that in the documentation for the
log-file-permission GUC.
        regards, tom lane


Re: log files and permissions

From
Michael Tharp
Date:
On 07/01/2010 12:56 PM, Kevin Grittner wrote:
> I just tried creating a symbolic link to the pg_log directory and
> flagging the existing logs within it to 640.  As a member of the
> group I was able to list and view the contents of log files through
> the symbolic link, even though I didn't have any authority to the
> PostgreSQL data directory.
>
> That seems potentially useful to me.

Symlinks are exactly equivalent to using the target of the link. Your 
permissions are probably already arranged so that you (as a group 
member) can access the files. Fedora's initscript seems to deliberately 
revoke group permissions from PGDATA and pg_log so I'm guessing that at 
some point some things were created with some group permissions.

That said, as Martin mentions one can easily place the log directory 
outside of the data directory and set appropriate directory permissions.

-- m. tharp


Re: log files and permissions

From
"Stephen J. Butler"
Date:
On Thu, Jul 1, 2010 at 12:19 PM, Michael Tharp
<gxti@partiallystapled.com> wrote:
> That said, as Martin mentions one can easily place the log directory outside
> of the data directory and set appropriate directory permissions.

If I can offer my $0.02, I recently solved such a problem on SuSE
Linux with apache logs. I used the ACL support on ext3 to give a
specific group read-only access:

cd /var/log
# Add an ACL for the 'www' user
setfacl -m u:www:r-x apache2
setfacl -m u:www:r-- apache2/*
# Modify the default ACL so that new files get 'r' for user
setfacl -d -m u:www:r-- apache2

Just pointing out that this problem is solvable on systems that
support ACLs w/o patching postgres.


Re: log files and permissions

From
Martin Pihlak
Date:
Martin Pihlak wrote:
> Attached is a patch that adds a GUC "log_file_mode" which allows to specify
> the creation mode for the log files. Presently it lacks documentation, which
> I'll add if the idea is generally acceptable.
>

Updated patch attached.

regards,
Martin

*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 2789,2794 **** local0.*    /var/log/postgresql
--- 2789,2813 ----
        </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>
+         When <varname>logging_collector</varname> is enabled,
+         this parameter sets the permissions of the created log files.
+     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,1040 ----
  }

  /*
+  * 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);
+         fchmod(fileno(fh), Log_file_mode);
+     }
+     else
+         ereport(am_rotating ? LOG : FATAL,
+                 (errcode_for_file_access(),
+                  (errmsg("could not create%slog file \"%s\": %m",
+                          am_rotating ? " new " : " ", 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.
--- 1080,1093 ----
          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.
--- 1133,1146 ----
          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
--- 1162,1167 ----
*** 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;


Re: log files and permissions

From
Itagaki Takahiro
Date:
I checked "log_file_mode GUC" patch, and found a couple of Windows-specific
and translation 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 think umask()->fopen() is better rather than fopen()->chmod().
See codes in DoCopyTo() at commands/copy.c.

* How does the file mode work on Windows?
If it doesn't work, we should explain it in docs.
Description for .pgpass for Windows might be a help.
| http://developer.postgresql.org/pgdocs/postgres/libpq-pgpass.html
| On Microsoft Windows, ... no special permissions check is made.

* This message format is hard to translate.   ereport(am_rotating ? LOG : FATAL,         (errcode_for_file_access(),
     (errmsg("could not create%slog file \"%s\": %m",                  am_rotating ? " new " : " ", filename))));
 

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
logfile ...);   else       ereport(LOG, ... "could not open new log file ...);
 

-- 
Itagaki Takahiro


Re: log files and permissions

From
Martin Pihlak
Date:
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;


Re: log files and permissions

From
Fujii Masao
Date:
On Mon, Jul 12, 2010 at 7:36 PM, Martin Pihlak <martin.pihlak@gmail.com> wrote:
> 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.

> +    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\"")));

The sticky bit cannot be set via log_file_mode. Is this intentional?

> +#ifndef WIN32
> +        chmod(filename, Log_file_mode);
> +#endif

Don't we need to check the return value of chmod()?

> +const char *assign_log_file_mode(const char *value,
> +                            bool doit, GucSource source);
> +const char *show_log_file_mode(void);

You forgot to write the show_log_file_mode()? I was not able to find that
in the patch.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


Re: log files and permissions

From
Itagaki Takahiro
Date:
I think the patch is almost ready for committer except the following
three issues:

2010/7/13 Fujii Masao <masao.fujii@gmail.com>:
>> +     if (!*value || *endptr || file_mode < 0 || file_mode > 0777)
> The sticky bit cannot be set via log_file_mode. Is this intentional?

We should also check the value not to be something like 0699.
How about checking it with (file_mode & ~0666) != 0 ?

>> +#ifndef WIN32
>> +             chmod(filename, Log_file_mode);
>> +#endif
> Don't we need to check the return value of chmod()?

I prefer umask() rather than chmod() here.

>> +const char *show_log_file_mode(void);
> You forgot to write the show_log_file_mode()? I was not able to find that
> in the patch.

I want show_log_file_mode to print the setting value in octal format.

--
Itagaki Takahiro


Re: log files and permissions

From
Martin Pihlak
Date:
Itagaki Takahiro wrote:
> I think the patch is almost ready for committer except the following
> three issues:
>
> 2010/7/13 Fujii Masao <masao.fujii@gmail.com>:
>>> +     if (!*value || *endptr || file_mode < 0 || file_mode > 0777)
>> The sticky bit cannot be set via log_file_mode. Is this intentional?

Yes -- I don't think there is a use case for sticky or setuid bits on log
files, even allowing execute is questionable.

> We should also check the value not to be something like 0699.
> How about checking it with (file_mode & ~0666) != 0 ?

Aha, that would ensure that the execute bit is not specified. Works for me.
The 0699 and other invalid octal values are caught by strtol()

>>> +#ifndef WIN32
>>> +             chmod(filename, Log_file_mode);
>>> +#endif
>> Don't we need to check the return value of chmod()?
>
> I prefer umask() rather than chmod() here.
>

Converted to umask()

>>> +const char *show_log_file_mode(void);
>> You forgot to write the show_log_file_mode()? I was not able to find that
>> in the patch.
>
> I want show_log_file_mode to print the setting value in octal format.
>

I've now (re)added the show_log_file_mode(). It used to be there, but then
at some point I decided to display the value "as-is".

While going through it, I moved the _setmode() call for win32 to logfile_open().

regards,
Martin

*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 2789,2794 **** local0.*    /var/log/postgresql
--- 2789,2815 ----
        </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. Specifying execute permissions for log files results in
+         an error.
+        </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,956 ----

      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 & ~0666) != 0)
+     {
+         ereport(GUC_complaint_elevel(source),
+                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                  errmsg("invalid value for parameter \"log_file_mode\""),
+                  errhint("value must be a 3 digit octal number, with the execute bit not set")));
+         return NULL;
+     }
+
+     if (doit)
+         Log_file_mode = (int) file_mode;
+
+     return value;
+ }
+
+ const char *
+ show_log_file_mode(void)
+ {
+     static char buf[5];
+
+     snprintf(buf, sizeof(buf), "%04o", Log_file_mode);
+     return buf;
+ }
*** 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,1027 **** 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 */
! #endif

!     csvlogFile = fh;

!     pfree(filename);

  }

  /*
--- 998,1050 ----

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

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

!     csvlogFile = fh;

!     pfree(filename);

! }

! /*
!  * Open the logfile with proper permissions and buffering options.
!  *
!  * We ignore any errors if the logfile is opened as part of log rotation. In
!  * other cases the errors are treated as fatal.
!  */
! static FILE *
! logfile_open(const char *filename, const char *mode, bool am_rotating)
! {
!     mode_t    oumask;
!     FILE   *fh;

!     oumask = umask((mode_t) (~Log_file_mode & 0777));
!     fh = fopen(filename, mode);
!     umask(oumask);
!
!     if (fh)
!     {
!         setvbuf(fh, NULL, LBF_MODE, 0);

+ #ifdef WIN32
+         _setmode(_fileno(fh), _O_TEXT); /* use CRLF line endings on Windows */
+ #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;
  }

  /*
***************
*** 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.
--- 1093,1106 ----
          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.
***************
*** 1104,1115 **** 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
-
          fclose(syslogFile);
          syslogFile = fh;

--- 1122,1127 ----
***************
*** 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.
--- 1140,1153 ----
          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,1173 **** 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
-
          fclose(csvlogFile);
          csvlogFile = fh;

--- 1169,1174 ----
*** 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, show_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;


Re: log files and permissions

From
Tom Lane
Date:
Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:
> ...
> We should also check the value not to be something like 0699.
> How about checking it with (file_mode & ~0666) != 0 ?
> ...
> I want show_log_file_mode to print the setting value in octal format.

It seems like a whole lot of lily-gilding is going on here.  Just make
it work like unix_socket_permissions already does.  That's been there
for years and nobody has complained about it.
        regards, tom lane


Re: log files and permissions

From
Martin Pihlak
Date:
Tom Lane wrote:
> Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:
>> ...
>> We should also check the value not to be something like 0699.
>> How about checking it with (file_mode & ~0666) != 0 ?
>> ...
>> I want show_log_file_mode to print the setting value in octal format.
>
> It seems like a whole lot of lily-gilding is going on here.  Just make
> it work like unix_socket_permissions already does.  That's been there
> for years and nobody has complained about it.
>

Thanks, somehow I missed that we can already specify octal integers
as GUC-s. I now converted the log_file_mode to integer and dropped
the assign_log_file_mode function.

regards,
Martin
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 2789,2794 **** local0.*    /var/log/postgresql
--- 2789,2817 ----
        </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 parameter value is expected to be a numeric mode
+         specified in the format accepted by the
+         <function>chmod</function> and <function>umask</function>
+         system calls.  (To use the customary octal format the number
+         must start with a <literal>0</literal> (zero).)
+        </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/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,1027 **** 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 */
! #endif

!     csvlogFile = fh;

!     pfree(filename);

  }

  /*
--- 998,1050 ----

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

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

!     csvlogFile = fh;

!     pfree(filename);

! }

! /*
!  * Open the logfile with proper permissions and buffering options.
!  *
!  * We ignore any errors if the logfile is opened as part of log rotation. In
!  * other cases the errors are treated as fatal.
!  */
! static FILE *
! logfile_open(const char *filename, const char *mode, bool am_rotating)
! {
!     mode_t    oumask;
!     FILE   *fh;

!     oumask = umask((mode_t) (~Log_file_mode & 0777));
!     fh = fopen(filename, mode);
!     umask(oumask);
!
!     if (fh)
!     {
!         setvbuf(fh, NULL, LBF_MODE, 0);
!
! #ifdef WIN32
!         _setmode(_fileno(fh), _O_TEXT); /* use CRLF line endings on Windows */
! #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;
  }

  /*
***************
*** 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.
--- 1093,1106 ----
          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.
***************
*** 1104,1115 **** 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
-
          fclose(syslogFile);
          syslogFile = fh;

--- 1122,1127 ----
***************
*** 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.
--- 1140,1153 ----
          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,1173 **** 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
-
          fclose(csvlogFile);
          csvlogFile = fh;

--- 1169,1174 ----
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 174,179 **** static bool assign_autovacuum_max_workers(int newval, bool doit, GucSource sourc
--- 174,180 ----
  static bool assign_effective_io_concurrency(int newval, bool doit, GucSource source);
  static const char *assign_pgstat_temp_directory(const char *newval, bool doit, GucSource source);
  static const char *assign_application_name(const char *newval, bool doit, GucSource source);
+ static const char *show_log_file_mode(void);

  static char *config_enum_get_options(struct config_enum * record,
                          const char *prefix, const char *suffix,
***************
*** 2068,2073 **** static struct config_int ConfigureNamesInt[] =
--- 2069,2083 ----
          1024, 100, 102400, NULL, NULL
      },

+     {
+         {"log_file_mode", PGC_SIGHUP, LOGGING_WHERE,
+             gettext_noop("Sets the file permissions for log files."),
+             NULL
+         },
+         &Log_file_mode,
+         0600, 0000, 0777, NULL, show_log_file_mode
+     },
+
      /* End-of-list marker */
      {
          {NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL
***************
*** 8072,8075 **** assign_application_name(const char *newval, bool doit, GucSource source)
--- 8082,8094 ----
          return newval;
  }

+ static const char *
+ show_log_file_mode(void)
+ {
+     static char buf[5];
+
+     snprintf(buf, sizeof(buf), "%04o", Log_file_mode);
+     return buf;
+ }
+
  #include "guc-file.c"
*** a/src/backend/utils/misc/postgresql.conf.sample
--- b/src/backend/utils/misc/postgresql.conf.sample
***************
*** 265,270 ****
--- 265,272 ----
                      # 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,
+                     # begin with 0 to use octal notation
  #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/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;


Re: log files and permissions

From
Tom Lane
Date:
Martin Pihlak <martin.pihlak@gmail.com> writes:
> Thanks, somehow I missed that we can already specify octal integers
> as GUC-s. I now converted the log_file_mode to integer and dropped
> the assign_log_file_mode function.

Applied with a few corrections.  The noncosmetic changes were:

* prevent Log_file_mode from disabling S_IWUSR permissions --- we had
better be able to write the files no matter what.

* save and restore errno across ereport() call; needed since some
callers look at errno after a failure.

* make unix_socket_permissions print its value in octal, for consistency with log_file_mode.

BTW, I'm not 100% convinced that having the octal show-functions is
a good idea, mainly because they aren't consistent with the other
columns in pg_settings:

regression=# select * from pg_settings where name = 'log_file_mode';    name      | setting | unit |
category              |               short_desc                |
                                      extra_desc
                      | co
 
ntext | vartype | source  | min_val | max_val | enumvals | boot_val | reset_val 
| sourcefile | sourceline 
---------------+---------+------+--------------------------------------+--------
----------------------------------+---------------------------------------------
--------------------------------------------------------------------------------
----------------------------------------------------------------------------+---
------+---------+---------+---------+---------+----------+----------+-----------
+------------+------------log_file_mode | 0600    |      | Reporting and Logging / Where to Log | Sets th
e file permissions for log files. | The parameter value is expected to be a nume
ric mode specification in the form accepted by the chmod and umask system calls.(To use the customary octal format the
numbermust start with a 0 (zero).) | si
 
ghup  | integer | default | 0       | 511     |          | 384      | 384       
|            |           
(1 row)

I guess this is not strictly incorrect, as long as you understand what
the leading '0' means per C conventions, but it looks a bit weird.
However, we're not going to be able to improve on this without a lot more
hackery than I think it's worth.
        regards, tom lane