Re: Patch to implement pg_current_logfile() function

From: Gilles Darold <gilles(dot)darold(at)dalibo(dot)com>
To: "Karl O(dot) Pinc" <kop(at)meme(dot)com>
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "Shulgin, Oleksandr" <oleksandr(dot)shulgin(at)zalando(dot)de>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Patch to implement pg_current_logfile() function
Date: 2016-06-28 09:06:24
Message-ID: 57723E10.7070300@dalibo.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Le 07/04/2016 08:30, Karl O. Pinc a écrit :
> On Thu, 7 Apr 2016 01:13:51 -0500
> "Karl O. Pinc" <kop(at)meme(dot)com> wrote:
>
>> On Wed, 6 Apr 2016 23:37:09 -0500
>> "Karl O. Pinc" <kop(at)meme(dot)com> wrote:
>>
>>> On Wed, 6 Apr 2016 22:26:13 -0500
>>> "Karl O. Pinc" <kop(at)meme(dot)com> wrote:
>>>> On Wed, 23 Mar 2016 23:22:26 +0100
>>>> Gilles Darold <gilles(dot)darold(at)dalibo(dot)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(at)meme(dot)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

Attachment Content-Type Size
patch_pg_current_logfile-v4.diff text/x-patch 7.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Rajeev rastogi 2016-06-28 10:01:28 Re: Improving executor performance
Previous Message Etsuro Fujita 2016-06-28 07:22:28 Re: Postgres_fdw join pushdown - wrong results with whole-row reference