On Mon, Dec 12, 2016 at 4:31 AM, Gilles Darold <gilles.dar...@dalibo.com> wrote:
> Applied in last version of the patch v18 as well as removing of an
> unused variable.
OK, this looks a lot closer to being committable. But:
@@ -570,11 +583,13 @@ SysLogger_Start(void)
* a time-based rotation.
*/
first_syslogger_file_time = time(NULL);
- filename = logfile_getname(first_syslogger_file_time, NULL);
+ last_file_name = logfile_getname(first_syslogger_file_time, NULL);
+
+ syslogFile = logfile_open(last_file_name, "a", false);
- syslogFile = logfile_open(filename, "a", false);
+ update_metainfo_datafile();
- pfree(filename);
+ pfree(last_file_name);
#ifdef EXEC_BACKEND
switch ((sysloggerPid = syslogger_forkexec()))
This leaves last_file_name pointing to free'd storage, which seems
like a recipe for bugs, because the next call to
update_metainfo_datafile() might try to follow that pointer. I think
you need to make a clear decision about what the contract is for
last_file_name and last_csv_file_name. Perhaps the idea is to always
keep them valid, but then you need to free the old value when
reassigning them and not free the current value.
The changes to logfile_rotate() appear to be mostly unrelated
refactoring which Karl was proposing for separate commit, not to be
incorporated into this patch. Please remove whatever deltas are not
essential to the new feature being implemented.
misc.c no longer needs to add an include of <sys/stat.h>. I hope, anyway.
+ errmsg("log format not supported, possible
values are stderr or csvlog")));
This doesn't follow our message style guidelines.
https://www.postgresql.org/docs/devel/static/error-style-guide.html
Basically, a comma splice is not an acceptable way of joining together
two independent thoughts. Obviously, people speak and write that way
informally all the time, but we try to be a bit rigorous about grammar
in our documentation and error messages. I think the way to do this
would be something like errmsg("log format \"%s\" is not supported"),
errhint("The supported log formats are \"stderr\" and \"csvlog\".").
+ FreeFile(fd);
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m",
+ LOG_METAINFO_DATAFILE)));
You don't need to FreeFile(fd) before ereport(ERROR). See header
comments for AllocateFile().
+ /* report the entry corresponding to the requested format */
+ if (strcmp(logfmt, log_format) == 0)
+ break;
+ }
+ /* Close the current log filename file. */
+ if (FreeFile(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m",
+ LOG_METAINFO_DATAFILE)));
+
+ if (lbuffer[0] == '\0')
+ PG_RETURN_NULL();
+
+ /* Recheck requested log format against the one extracted from the file */
+ if (logfmt != NULL && (log_format == NULL ||
+ strcmp(logfmt, log_format) != 0))
+ PG_RETURN_NULL();
Your email upthread claims that you fixed this per my suggestions, but
it doesn't look fixed to me. That check to see whether lbuffer[0] ==
'\0' is either protecting against nothing at all (in which case it
could be removed) or it's protecting against some weirdness that can
only happen here because of the strange way this logic is written.
It's hard to understand all the cases here because we can exit the
loop either having found the entry we want or not, and there's no
clear indication of which one it is. Why not change the if-statement
at the end of the loop like this:
if (logfmt == NULL || strcmp(logfmt, log_format) == 0)
{
FreeFile(fd);
PG_RETURN_TEXT_P(cstring_to_text(log_filepath));
}
Then after the loop, just:
FreeFile(fd);
PG_RETURN_NULL();
+ <para>
+ A file recording the log file(s) currently written to by the syslogger
+ and the file's log formats, <systemitem>stderr</>
+ or <systemitem>csvlog</>. Each line of the file is a space separated list of
+ two elements: the log format and the full path to the log file including the
+ value of <xref linkend="guc-log-directory">. The log format must be present
+ in <xref linkend="guc-log-destination"> to be listed in
+ <filename>current_logfiles</filename>.
+ </para>
+
+ <note>
+ <indexterm>
+ <primary><application>pg_ctl</application></primary>
+ <secondary>and <filename>current_logfiles</filename></secondary>
+ </indexterm>
+ <indexterm>
+ <primary><filename>stderr</filename></primary>
+ <secondary>and <filename>current_logfiles</filename></secondary>
+ </indexterm>
+ <indexterm>
+ <primary>log_destination configuration parameter</primary>
+ <secondary>and <filename>current_logfiles</filename></secondary>
+ </indexterm>
+
+ <para>
+ Although logs directed to <filename>stderr</filename> may be written
+ to the filesystem, when the writing of <filename>stderr</filename> is
+ managed outside of the <productname>PostgreSQL</productname> database
+ server the location of such files in the filesystem is not reflected in
+ the content of <filename>current_logfiles</filename>. One such case is
+ when the <application>pg_ctl</application> command is used to start
+ the <command>postgres</command> database server, capture
+ the <filename>stderr</filename> output of the server, and direct it to a
+ file.
+ </para>
+
+ <para>
+ There are other notable situations related
+ to <filename>stderr</filename> logging.
+ Non-<productname>PostgreSQL</productname> log sources, such as 3rd party
+ libraries, which deliver error messages directly
+ to <filename>stderr</filename> are always logged
+ by <productname>PostgreSQL</productname>
+ to <filename>stderr</filename>. <Filename>Stderr</Filename> is also the
+ destination for any incomplete log messages produced by
+ <productname>PostgreSQL</productname>. When
+ <systemitem>stderr</systemitem> is not in
+ <xref linkend="guc-log-destination">,
+ <filename>current_logfiles</filename> does not not contain the name of the
+ file where these sorts of log messages are written.
+ </para>
+ </note>
+
+ <para>
+ The <filename>current_logfiles</filename> file
+ is present only when <xref linkend="guc-logging-collector"> is
+ activated and when at least one of <systemitem>stderr</> or
+ <systemitem>csvlog</> value is present in
+ <xref linkend="guc-log-destination">.
+ </para>
+ </entry>
This seems unnecessarily long-winded to me. I would just remove the
entire <note>, which seems like a lengthy digression mostly on topics
not really related to the contents of current_logfiles. The text
above and below make it clear enough that this file is only capturing
what the syslogger is doing and that it is based on
guc-log-destination; we don't need to recapitulate that a third and a
fourth time. If the logging collector (syslogger) is turned on, it's
going to capture everything that pops out on stderr, because every
backend's stderr will have been redirected to the collector. If it's
not, then the current_logfiles metafile won't be populated. Other
things that might happen in other situations might need to be added to
the documentation someplace, but not in this patch and not here.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
