Re: Patch to implement pg_current_logfile() function - Mailing list pgsql-hackers

On Tue, 18 Oct 2016 14:18:36 +0200
Gilles Darold <gilles.darold@dalibo.com> wrote:

Here is the v6 of the patch, here is the description of the
pg_current_logfile() function, I have tried to keep thing as simple as
possible:

pg_current_logfile( [ destination text ] )
-----------------------------------------------------

Attached is a doc patch to apply on top of your v6 patch.

Thanks a lot for the documentation fixes, I've also patched some of your changes, see v7 of the patch and explanations bellow.

Changes are, roughly:

Put pg_log_file in alphabetical order in the file name listing
and section body.

This file is now named current_logfile, I have changed the named in the documentation, especially in storage.sgml. Sorry for missing that in my v6 patch.

One other change in documentation is the explanation of values stored in this file. This is a path: log_directory/log_filename, and no more the log file name only. This will help to get full path of the log at system command level. This is also the value returned by function the pg_current_logfile() to be able to read file directly with a simple:

     SELECT pg_read_file(pg_current_logfile());

It can be also useful if the file is included in a backup to find the last log corresponding to the backup.

I also have a couple of preliminary comments.

It seems like the code is testing for whether the
logfile is a CSV file by looking for a '.csv' suffix
on the end of the file name.  This seems a little fragile.
Shouldn't it be looking at something set internally when the
log_destination config declaration is parsed?

Right, even if syslogger.c always adds the .csv suffix to the log file. This method would failed if the log file was using this extension even for a stderr format. It has been fixed in the v7 patch to no more use the extension suffix.

Since pg_log_file may contain only one line, and that
line may be either the filename of the csv log file or
the file name of the stderr file name it's impossible
to tell whether that single file is in csv or stderr
format.  I suppose it might be possible based on file
name suffix, but Unix expressly ignores file name
extensions and it seems unwise to force dependence
on them.  Perhaps each line could begin with
the "type" of the file, either 'csv' or 'stderr'
followed by a space and the file name? 

The current_logfile may contain 2 lines, one for csvlog and an other for stderr when they are both defined in log_destination. As said above, the csvlog file will always have the .csv suffix, I guess it is enough to now the format but I agree that it will not be true in all cases. To keep things simple I prefer to only register the path to the log file, external tools can easily detect the format or can ask for the path to a specific log format using SELECT pg_current_logfile('stderr'); for example. This is my point of view, but if there's a majority to add the log format into the current_logfile why not.

I have copy/paste here your other comments to limit the number of messages:

> You're writing Unix eol characters into pg_log_file.  (I think.)
> Does this matter on MS Windows?  (I'm not up on MS Windows,
> and haven't put any thought into this at all.  But didn't
> want to forget about the potential issue.)

This doesn't matter as the file is opened in text mode (see logfile_open() in syslogger.c) and use of LF in fprintf() will be automatically converted to CRLF on MS Windows.

> While you're at it, it wouldn't hurt to provide another
> function that tells you the format of the file returned
> by pg_current_logfile(), since pg_current_logfile()
> called without arguments could return either a stderr
> formatted file or a csvlog formatted file.

The log format can be check using something like:

postgres=# SELECT pg_current_logfile(), pg_current_logfile('csvlog');
           pg_current_logfile            |           pg_current_logfile           
-----------------------------------------+-----------------------------------------
 pg_log/postgresql-2016-10-26_231700.log | pg_log/postgresql-2016-10-26_231700.csv
(1 row)

postgres=# SELECT CASE WHEN (pg_current_logfile() = pg_current_logfile('stderr')) THEN 'stderr' ELSE 'csvlog' END AS log_format;
 log_format
------------
 stderr
(1 row)

but if we want we can have an other internal function with a call like:

    SELECT pg_log_format(pg_current_logfile());

that will return stderr or csvlog but I'm not sure this is necessary.

Thanks for your review, let me know if there's any thing to adapt.

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org