Re: Patch to implement pg_current_logfile() function

Le 07/04/2016 08:30, Karl O. Pinc a écrit :
> On Thu, 7 Apr 2016 01:13:51 -0500
> "Karl O. Pinc" <kop@meme.com> wrote:
>
>> On Wed, 6 Apr 2016 23:37:09 -0500
>> "Karl O. Pinc" <kop@meme.com> wrote:
>>
>>> On Wed, 6 Apr 2016 22:26:13 -0500
>>> "Karl O. Pinc" <kop@meme.com> wrote:
>>>> On Wed, 23 Mar 2016 23:22:26 +0100
>>>> Gilles Darold <gilles.darold@dalibo.com> wrote:
>>>>
>>>>> Thanks for the reminder, here is the v3 of the patch after a
>>>>> deeper review and testing. It is now registered to the next
>>>>> commit fest under the System Administration topic.
>> This is what I see at the moment.  I'll wait for replies
>> before looking further and writing more.
> Er, one more thing.  Isn't it true that in logfile_rotate()
> you only need to call store_current_log_filename() when
> logfile_open() is called with a "w" mode, and never need to
> call it when logfile_open() is called with an "a" mode?
>
>
> Karl <kop@meme.com>
> Free Software:  "You don't pay back, you pay forward."
>                  -- Robert A. Heinlein
>
>

Hi Karl,

Thank you very much for the patch review and please apologies this too
long response delay. I was traveling since end of April and totally
forgotten this patch. I have applied all your useful feedbacks on the
patch and attached a new one (v4) to this email :

    - Fix the missing <row> in doc/src/sgml/func.sgml
    - Rewrite pg_current_logfile descritption as suggested
    - Remove comment in src/backend/postmaster/syslogger.c
    - Use pgrename to first write the filename to a temporary file
before overriding the last log file.
    - Rename store_current_log_filename() into logfile_writename() -
logfile_getname is used to retrieve the filename.
    - Use logfile_open() to enable CRLF line endings on Windows
    - Change log level for when it can't create pg_log_file, from
WARNING to LOG
    - Remove NOTICE message when the syslogger is not enabled, the
function return null.

On other questions:

> "src/backend/postmaster/syslogger.c expects to see fopen() fail with
ENFILE and EMFILE.  What will you do if you get these?"

    - Nothing, if the problem occurs during the log rotate call, then
log rotation file is disabled so logfile_writename() will not be called.
Case where the problem occurs between the rotation call and the
logfile_writename() call is possible but I don't think that it will be
useful to try again. In this last case log filename will be updated
during next rotation.

> "Have you given any thought as to when logfile_rotate() is called?
Since logfile_rotate() itself logs with ereport(), it would _seem_ safe
to ereport() from within your store_current_log_filename(), called from
within logfile_rotate()."

    - Other part of logfile_rotate() use ereport() so I guess it is safe
to use it.

> "The indentation of the ereport(), in the part that continues over
multiple lines"

    - This was my first thought but seeing what is done in the other
call to ereport() I think I have done it the right way.

> "Isn't it true that in logfile_rotate() you only need to call
store_current_log_filename() when logfile_open() is called with a "w"
mode, and never need to call it when logfile_open() is called with an
"a" mode?"

    - No, it is false, append mode is used when logfile_rotate used an
existing file but the filename still change.


Best regards,

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