Re: Patch to implement pg_current_logfile() function

From: "Karl O(dot) Pinc" <kop(at)meme(dot)com>
To: Gilles Darold <gilles(dot)darold(at)dalibo(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-30 07:04:59
Message-ID: 20161030020459.1b352e4a@slate.meme.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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().

I have some questions about logfile_writename():

Why does the logfile_open() call fail silently?
Why not use ereport() here? (With a WARNING level.)

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.)

Have you given any thought to my proposal to change
CURRENT_LOG_FILENAME to LOG_METAINFO_FILE?

I'm not sure I've looked at every bit of your patch
yet. I won't have much time to do more real work
until after Tuesday morning.

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

Attachment Content-Type Size
patch_pg_current_logfile-v10.diff.logfile_writename application/octet-stream 1.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Karl O. Pinc 2016-10-30 07:20:47 Re: Patch to implement pg_current_logfile() function
Previous Message Michael Paquier 2016-10-30 07:04:15 Re: Mention column name in error messages