Обсуждение: log files and permissions
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
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;
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
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
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
* 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
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
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
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.
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;
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
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;
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
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
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;
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
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;
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