On Mon, Jan 16, 2017 at 1:14 PM, Karl O. Pinc <k...@meme.com> wrote:
> I do have a question here regards code formatting.
> The patch now contains:
>
> if (log_filepath == NULL)
> {
> /* Bad data. Avoid segfaults etc. and return NULL to caller. */
> break;
> }
>
> I'm not sure how this fits in with PG coding style,
> whether the {} should be removed or what. I've looked
> around and can't find an example of an if with a single
> line then block and a comment. Maybe this means that
> I shouldn't be doing this, but if I put the comment above
> the if then it will "run into" the comment block which
> immediately precedes the above code which describes
> a larger body of code. So perhaps someone should look
> at this and tell me how to improve it.
Including brackets in this case makes a more readable code. That's the
pattern respected the most in PG code. See for example
XLogInsertRecord():
else
{
/*
* This was an xlog-switch record, but the current insert location was
* already exactly at the beginning of a segment, so there was no need
* to do anything.
*/
}
> Attached also are 2 patches which abstract some hardcoded
> constants into symbols. Whether to apply them is a matter
> of style and left to the maintainers, which is why these
> are separate patches.
Making the strings csvlog, stderr and eventlog variables? Why not
because the patch presented here uses them in new places. Now it is
not like it is a huge maintenance burden to keep them as strings, so I
would personally not bother much.
> The first patch changes only code on the master
> branch, the 2nd patch changes the new code.
Thanks for keeping things separated.
> I have not looked further at the patch or checked
> to see that all changes previously recommended have been
> made. Michael, if you're confident that everything is good
> go ahead and move the patchs forward to the maintainers. Otherwise
> please let me know and I'll see about finding time
> for further review.
OK. I have done a round of hands-on review on this patch, finishing
with the attached. In the things I have done:
- Elimination of useless noise diff
- Fixes some typos (logile, log file log, etc.)
- Adjusted docs.
- Docs were overdoing in storage.sgml. Let's keep the description simple.
- Having a paragraph at the beginning of "Error Reporting and Logging"
in config.sgml does not look like a good idea to me. As the updates of
current_logfiles is associated with log_destination I'd rather see
this part added in the description of the GUC.
- Missed a (void) in the definition of update_metainfo_datafile().
- Moved update_metainfo_datafile() before the signal handler routines
in syslogger.c for clarity.
- The last "return" is useless in update_metainfo_datafile().
- In pg_current_logfile(), it is useless to call PG_RETURN_NULL after
emitting an ERROR message.
- Two calls to FreeFile(fd) have been forgotten in
pg_current_logfile(). In one case, errno needs to be saved beforehand
to be sure that the correct error string is generated for the user.
- A portion of 010_pg_basebackup.pl was not updated.
- Incorrect header ordering in basebackup.c.
- Decoding of CR and LF characters seem to work fine when
log_file_name include some.
With all those issues fixed, I finish with the attached, that I am
fine to pass down to a committer. I still think that this should be
only one function using a SRF with rows shaped as (type, path) for
simplicity, but as I am visibly outnumbered I won't insist further.
Also, I would rather see an ERROR returned to the user in case of bad
data in current_logfiles, I did not change that either as that's the
original intention of Gilles.
--
Michael
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 07afa3c77a..3188496bc2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4248,6 +4248,25 @@ SELECT * FROM parent WHERE key = 2400;
<xref linkend="guc-logging-collector"> must be enabled to generate
CSV-format log output.
</para>
+ <para>
+ When either <systemitem>stderr</systemitem> or
+ <systemitem>csvlog</systemitem> are included, the file
+ <filename>current_logfiles</> gets created and records the location
+ of the log file(s) currently in use by the logging collector and the
+ associated logging destination. This provides a convenient way to
+ find the logs currently in use by the instance. Here is an example of
+ contents of this file:
+<programlisting>
+stderr pg_log/postgresql.log
+csvlog pg_log/postgresql.csv
+</programlisting>
+ Note that this file gets updated when a new log file is created
+ as an effect of rotation or when <varname>log_destination</> is
+ reloaded. <filename>current_logfiles</> is removed when
+ <systemitem>stderr</systemitem> and <systemitem>csvlog</systemitem>
+ are not included in <varname>log_destination</> or when the
+ logging collector is disabled.
+ </para>
<note>
<para>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 10e31868ba..c756fbe066 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15441,6 +15441,13 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS
t(ls,n);
</row>
<row>
+
<entry><literal><function>pg_current_logfile(<optional><type>text</></optional>)</function></literal></entry>
+ <entry><type>text</type></entry>
+ <entry>Primary log file name, or log in the requested format,
+ currently in use by the logging collector</entry>
+ </row>
+
+ <row>
<entry><literal><function>pg_my_temp_schema()</function></literal></entry>
<entry><type>oid</type></entry>
<entry>OID of session's temporary schema, or 0 if none</entry>
@@ -15659,6 +15666,34 @@ SET search_path TO <replaceable>schema</> <optional>,
<replaceable>schema</>, ..
</para>
<indexterm>
+ <primary>pg_current_logfile</primary>
+ </indexterm>
+
+ <indexterm>
+ <primary>Logging</primary>
+ <secondary>pg_current_logfile function</secondary>
+ </indexterm>
+
+ <para>
+ <function>pg_current_logfile</function> returns, as <type>text</type>,
+ the path of the log file(s) currently in use by the logging collector.
+ The path includes the <xref linkend="guc-log-directory"> directory
+ and the log file name. Log collection must be enabled or the return value
+ is <literal>NULL</literal>. When multiple log files exist, each in a
+ different format, <function>pg_current_logfile</function> called
+ without arguments returns the path of the file having the first format
+ found in the ordered list: <systemitem>stderr</>, <systemitem>csvlog</>.
+ <literal>NULL</literal> is returned when no log file has any of these
+ formats. To request a specific file format supply, as <type>text</type>,
+ either <systemitem>csvlog</> or <systemitem>stderr</> as the value of the
+ optional parameter. The return value is <literal>NULL</literal> when the
+ log format requested is not a configured
+ <xref linkend="guc-log-destination">.
+ <function>pg_current_logfiles</function> reflects the contents of the
+ file <filename>current_logfiles</>.
+ </para>
+
+ <indexterm>
<primary>pg_my_temp_schema</primary>
</indexterm>
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824dfc..388ed344ee 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -61,6 +61,12 @@ Item
</row>
<row>
+ <entry><filename>current_logfiles</></entry>
+ <entry>File recording the log file(s) currently written to by the logging
+ collector</entry>
+</row>
+
+<row>
<entry><filename>global</></entry>
<entry>Subdirectory containing cluster-wide tables, such as
<structname>pg_database</></entry>
diff --git a/src/backend/catalog/system_views.sql
b/src/backend/catalog/system_views.sql
index 31aade102b..1d7f68b8a4 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1055,6 +1055,7 @@ REVOKE EXECUTE ON FUNCTION pg_xlog_replay_pause() FROM
public;
REVOKE EXECUTE ON FUNCTION pg_xlog_replay_resume() FROM public;
REVOKE EXECUTE ON FUNCTION pg_rotate_logfile() FROM public;
REVOKE EXECUTE ON FUNCTION pg_reload_conf() FROM public;
+REVOKE EXECUTE ON FUNCTION pg_current_logfile() FROM public;
REVOKE EXECUTE ON FUNCTION pg_stat_reset() FROM public;
REVOKE EXECUTE ON FUNCTION pg_stat_reset_shared(text) FROM public;
diff --git a/src/backend/postmaster/syslogger.c
b/src/backend/postmaster/syslogger.c
index 13a03014eb..e6899c6246 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -146,6 +146,7 @@ static char *logfile_getname(pg_time_t timestamp, const
char *suffix);
static void set_next_rotation_time(void);
static void sigHupHandler(SIGNAL_ARGS);
static void sigUsr1Handler(SIGNAL_ARGS);
+static void update_metainfo_datafile(void);
/*
@@ -348,6 +349,13 @@ SysLoggerMain(int argc, char *argv[])
rotation_disabled = false;
rotation_requested = true;
}
+
+ /*
+ * Force rewriting last log filename when reloading
configuration,
+ * even if rotation_requested is false, log_destination
may have
+ * been changed and we don't want to wait the next file
rotation.
+ */
+ update_metainfo_datafile();
}
if (Log_RotationAge > 0 && !rotation_disabled)
@@ -511,10 +519,16 @@ int
SysLogger_Start(void)
{
pid_t sysloggerPid;
- char *filename;
+ /*
+ * Logging collector is not enabled. We don't know where messages are
+ * logged. Remove outdated file holding the current log filenames.
+ */
if (!Logging_collector)
+ {
+ unlink(LOG_METAINFO_DATAFILE);
return 0;
+ }
/*
* If first time through, create the pipe which will receive stderr
@@ -570,11 +584,15 @@ SysLogger_Start(void)
* a time-based rotation.
*/
first_syslogger_file_time = time(NULL);
- filename = logfile_getname(first_syslogger_file_time, NULL);
- syslogFile = logfile_open(filename, "a", false);
+ if (last_file_name != NULL)
+ pfree(last_file_name);
+
+ last_file_name = logfile_getname(first_syslogger_file_time, NULL);
+
+ syslogFile = logfile_open(last_file_name, "a", false);
- pfree(filename);
+ update_metainfo_datafile();
#ifdef EXEC_BACKEND
switch ((sysloggerPid = syslogger_forkexec()))
@@ -1098,6 +1116,8 @@ open_csvlogfile(void)
pfree(last_csv_file_name);
last_csv_file_name = filename;
+
+ update_metainfo_datafile();
}
/*
@@ -1268,6 +1288,8 @@ logfile_rotate(bool time_based_rotation, int
size_rotation_for)
if (csvfilename)
pfree(csvfilename);
+ update_metainfo_datafile();
+
set_next_rotation_time();
}
@@ -1337,6 +1359,62 @@ set_next_rotation_time(void)
next_rotation_time = now;
}
+/*
+ * Store the name of the file(s) where the log collector, when enabled, writes
+ * log messages. Useful for finding the name(s) of the current log file(s)
+ * when there is time-based logfile rotation. Filenames are stored in a
+ * temporary file and which is renamed into the final destination for
+ * atomicity.
+ */
+static void
+update_metainfo_datafile(void)
+{
+ FILE *fh;
+
+ if (!(Log_destination & LOG_DESTINATION_STDERR) &&
+ !(Log_destination & LOG_DESTINATION_CSVLOG))
+ {
+ unlink(LOG_METAINFO_DATAFILE);
+ return;
+ }
+
+ if ((fh = logfile_open(LOG_METAINFO_DATAFILE_TMP, "w", true)) == NULL)
+ return;
+
+ if (last_file_name && (Log_destination & LOG_DESTINATION_STDERR))
+ {
+ if (fprintf(fh, "stderr %s\n", last_file_name) < 0)
+ {
+ ereport(LOG,
+ (errcode_for_file_access(),
+ errmsg("could not write stderr log file
path \"%s\": %m",
+
LOG_METAINFO_DATAFILE_TMP)));
+ fclose(fh);
+ return;
+ }
+ }
+
+ if (last_csv_file_name && (Log_destination & LOG_DESTINATION_CSVLOG))
+ {
+ if (fprintf(fh, "csvlog %s\n", last_csv_file_name) < 0)
+ {
+ ereport(LOG,
+ (errcode_for_file_access(),
+ errmsg("could not write csvlog log file
path \"%s\": %m",
+
LOG_METAINFO_DATAFILE_TMP)));
+ fclose(fh);
+ return;
+ }
+ }
+ fclose(fh);
+
+ if (rename(LOG_METAINFO_DATAFILE_TMP, LOG_METAINFO_DATAFILE) != 0)
+ ereport(LOG,
+ (errcode_for_file_access(),
+ errmsg("could not rename file \"%s\" to \"%s\":
%m",
+ LOG_METAINFO_DATAFILE_TMP,
LOG_METAINFO_DATAFILE)));
+}
+
/* --------------------------------
* signal handler routines
* --------------------------------
diff --git a/src/backend/replication/basebackup.c
b/src/backend/replication/basebackup.c
index 09ecc15365..24d5d9f5d6 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -27,6 +27,7 @@
#include "nodes/pg_list.h"
#include "pgtar.h"
#include "pgstat.h"
+#include "postmaster/syslogger.h"
#include "replication/basebackup.h"
#include "replication/walsender.h"
#include "replication/walsender_private.h"
@@ -148,6 +149,9 @@ static const char *excludeFiles[] =
/* Skip auto conf temporary file. */
PG_AUTOCONF_FILENAME ".tmp",
+ /* Skip current log file temporary file */
+ LOG_METAINFO_DATAFILE_TMP,
+
/*
* If there's a backup_label or tablespace_map file, it belongs to a
* backup started by the user with pg_start_backup(). It is *not*
correct
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 66d09bcb0c..c5969672e0 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -892,3 +892,103 @@ parse_ident(PG_FUNCTION_ARGS)
PG_RETURN_DATUM(makeArrayResult(astate, CurrentMemoryContext));
}
+
+/*
+ * pg_current_logfile
+ *
+ * Report current log file used by log collector by scanning current_logfiles.
+ */
+Datum
+pg_current_logfile(PG_FUNCTION_ARGS)
+{
+ FILE *fd;
+ char lbuffer[MAXPGPATH];
+ char *logfmt;
+ char *log_filepath;
+ char *log_format = lbuffer;
+
+ /* The log format parameter is optional */
+ if (PG_NARGS() == 0 || PG_ARGISNULL(0))
+ logfmt = NULL;
+ else
+ {
+ logfmt = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+ if (strcmp(logfmt, "stderr") != 0 && strcmp(logfmt, "csvlog")
!= 0)
+ ereport(ERROR,
+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("log format \"%s\" is not
supported", logfmt),
+ errhint("The supported log formats are
\"stderr\""
+ " and
\"csvlog\".")));
+ }
+
+ fd = AllocateFile(LOG_METAINFO_DATAFILE, "r");
+ if (fd == NULL)
+ {
+ if (errno != ENOENT)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\":
%m",
+ LOG_METAINFO_DATAFILE)));
+ PG_RETURN_NULL();
+ }
+
+ /*
+ * Read the file to gather current log filename(s) registered
+ * by the syslogger.
+ */
+ while (fgets(lbuffer, sizeof(lbuffer), fd) != NULL)
+ {
+ /* Check for a read error. */
+ if (ferror(fd))
+ {
+ int save_errno = errno;
+ FreeFile(fd);
+ errno = save_errno;
+
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\":
%m",
+
LOG_METAINFO_DATAFILE)));
+ }
+
+ /*
+ * Extract log format and log file path from the line;
+ * lbuffer == log_format, they share storage.
+ */
+ log_filepath = strchr(lbuffer, ' ');
+ if (log_filepath == NULL)
+ {
+ /* Bad data. Avoid segfaults etc. and return NULL to
caller. */
+ break;
+ }
+
+ *log_filepath = '\0';
+ log_filepath++;
+ log_filepath[strcspn(log_filepath, "\n")] = '\0';
+
+ if (logfmt == NULL || strcmp(logfmt, log_format) == 0)
+ {
+ FreeFile(fd);
+ PG_RETURN_TEXT_P(cstring_to_text(log_filepath));
+ }
+ }
+
+ /* Close the current log filename file. */
+ FreeFile(fd);
+
+ PG_RETURN_NULL();
+}
+
+/*
+ * Report current log file used by log collector (1 argument version)
+ *
+ * note: this wrapper is necessary to pass the sanity check in opr_sanity,
+ * which checks that all built-in functions that share the implementing C
+ * function take the same number of arguments
+ */
+Datum
+pg_current_logfile_1arg(PG_FUNCTION_ARGS)
+{
+ return pg_current_logfile(fcinfo);
+}
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 4c6670ce72..196db5be26 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -4,7 +4,7 @@ use Cwd;
use Config;
use PostgresNode;
use TestLib;
-use Test::More tests => 71;
+use Test::More tests => 72;
program_help_ok('pg_basebackup');
program_version_ok('pg_basebackup');
@@ -56,7 +56,7 @@ close CONF;
$node->restart;
# Write some files to test that they are not copied.
-foreach my $filename (qw(backup_label tablespace_map postgresql.auto.conf.tmp))
+foreach my $filename (qw(backup_label tablespace_map postgresql.auto.conf.tmp
current_logfiles.tmp))
{
open FILE, ">>$pgdata/$filename";
print FILE "DONOTCOPY";
@@ -83,7 +83,7 @@ foreach my $dirname (qw(pg_dynshmem pg_notify pg_replslot
pg_serial pg_snapshots
}
# These files should not be copied.
-foreach my $filename (qw(postgresql.auto.conf.tmp postmaster.opts
postmaster.pid tablespace_map))
+foreach my $filename (qw(postgresql.auto.conf.tmp current_logfiles.tmp
postmaster.opts postmaster.pid tablespace_map))
{
ok(! -f "$tempdir/backup/$filename", "$filename not copied");
}
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 37e022dc0d..eb56e8d6af 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3183,6 +3183,10 @@ DATA(insert OID = 2621 ( pg_reload_conf
PGNSP PGUID 12 1 0 0 0 f f f f t f v s
DESCR("reload configuration files");
DATA(insert OID = 2622 ( pg_rotate_logfile PGNSP PGUID 12 1 0 0 0
f f f f t f v s 0 0 16 "" _null_ _null_ _null_ _null_ _null_ pg_rotate_logfile
_null_ _null_ _null_ ));
DESCR("rotate log file");
+DATA(insert OID = 3800 ( pg_current_logfile PGNSP PGUID 12 1 0 0 0
f f f f f f v s 0 0 25 "" _null_ _null_ _null_ _null_ _null_ pg_current_logfile
_null_ _null_ _null_ ));
+DESCR("current logging collector file location");
+DATA(insert OID = 3801 ( pg_current_logfile PGNSP PGUID 12 1 0 0 0
f f f f f f v s 1 0 25 "25" _null_ _null_ _null_ _null_ _null_
pg_current_logfile_1arg _null_ _null_ _null_ ));
+DESCR("current logging collector file location");
DATA(insert OID = 2623 ( pg_stat_file PGNSP PGUID 12 1 0 0 0 f f f f
t f v s 1 0 2249 "25" "{25,20,1184,1184,1184,1184,16}" "{i,o,o,o,o,o,o}"
"{filename,size,access,modification,change,creation,isdir}" _null_ _null_
pg_stat_file_1arg _null_ _null_ _null_ ));
DESCR("get information about file");
diff --git a/src/include/postmaster/syslogger.h
b/src/include/postmaster/syslogger.h
index c187a5f23e..ebe324796f 100644
--- a/src/include/postmaster/syslogger.h
+++ b/src/include/postmaster/syslogger.h
@@ -87,4 +87,11 @@ extern void write_syslogger_file(const char *buffer, int
count, int dest);
extern void SysLoggerMain(int argc, char *argv[]) pg_attribute_noreturn();
#endif
+/*
+ * Name of files saving meta-data information about the log
+ * files currently in use by the system logging process
+ */
+#define LOG_METAINFO_DATAFILE "current_logfiles"
+#define LOG_METAINFO_DATAFILE_TMP LOG_METAINFO_DATAFILE ".tmp"
+
#endif /* _SYSLOGGER_H */
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index e1bb344e4f..e2532e64e8 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -522,6 +522,8 @@ extern Datum pg_collation_for(PG_FUNCTION_ARGS);
extern Datum pg_relation_is_updatable(PG_FUNCTION_ARGS);
extern Datum pg_column_is_updatable(PG_FUNCTION_ARGS);
extern Datum parse_ident(PG_FUNCTION_ARGS);
+extern Datum pg_current_logfile(PG_FUNCTION_ARGS);
+extern Datum pg_current_logfile_1arg(PG_FUNCTION_ARGS);
/* oid.c */
extern Datum oidin(PG_FUNCTION_ARGS);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
