Re: log files and permissions
Started by Itagaki Takahiroover 15 years ago8 messages
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 log file ...);
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
Attachments:
log-file-mode.patchtext/x-diff; name=log-file-mode.patchDownload
*** 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); +#endifDon'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); +#endifDon'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
Attachments:
log-file-mode.patchtext/x-diff; name=log-file-mode.patchDownload
*** 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
Attachments:
log-file-mode.patchtext/x-diff; name=log-file-mode.patchDownload
*** 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 number must 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