Re: Patch to implement pg_current_logfile() function

Le 26/10/2016 à 05:30, Karl O. Pinc a écrit :
> On Tue, 18 Oct 2016 14:18:36 +0200
> Gilles Darold <gilles(dot)darold(at)dalibo(dot)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