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: Christoph Berg <myon(at)debian(dot)org>, 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-10-31 08:26:27
Message-ID: 10910ff0-5394-fbd0-f683-77f1662aee57@dalibo.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Le 30/10/2016 à 08:04, Karl O. Pinc a écrit :
> On Sat, 29 Oct 2016 22:00:08 +0200
> Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
>
>> The attached v10 of the current_logfiles patch include your last
>> changes on documentation but not the patch on v9 about the
>> user-supplied GUC value. I think the v10 path is ready for committers
>> and that the additional patch to add src/include/utils/guc_values.h
>> to define user GUC values is something that need to be taken outside
>> this one. Imo, thoses GUC values (stderr, csvlog) are not expected to
>> change so often to require a global definition, but why not, if
>> committers think this must be done I can add it to a v11 patch.
> I agree that the guc_values.h patches should be submitted separately
> to the committers, when your patch is submitted. Creating symbols
> for these values is a matter of coding style they should resolve.
> (IMO it's not whether the values will change, it's whether
> someone reading the code can read the letters "stdout" and know
> to what they refer and where to find related usage elsewhere in
> the code.)
>
> I'll keep up the guc_values.h patches and have them ready for
> submission along with your patch.
>
> I don't think your patch is quite ready for submission to
> the committers.
>
> Attached is a patch to be applied on top of your v10 patch
> which does basic fixup to logfile_writename().

Attached patch v11 include your patch.

>
> I have some questions about logfile_writename():
>
> Why does the logfile_open() call fail silently?
> Why not use ereport() here? (With a WARNING level.)

logfile_open() "fail silently" in logfile_writename(), like in other
parts of syslogger.c where it is called, because this is a function()
that already report a message when an error occurs ("could not open log
file..."). I think I have already replied to this question.

> Why do the ereport() invocations all have a LOG level?
> You're not recovering and the errors are unexpected
> so I'd think WARNING would be the right level.
> (I previously, IIRC, suggested LOG level -- only if
> you are retrying and recovering from an error.)

If you look deeper in syslogger.c, all messages are reported at LOG
level. I guess the reason is to not call syslogger repeatedly. I think I
have already replied to this question in the thread too.

> Have you given any thought to my proposal to change
> CURRENT_LOG_FILENAME to LOG_METAINFO_FILE?
Yes, I don't think the information logged in this file are kind of meta
information and CURRENT_LOG_FILENAME seems obvious.

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

Attachment Content-Type Size
patch_pg_current_logfile-v11.diff text/x-diff 13.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Craig Ringer 2016-10-31 08:34:38 Logical decoding and walsender timeouts
Previous Message Albe Laurenz 2016-10-31 08:14:37 Re: sequential scan result order vs performance