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-26 03:30:48
Message-ID: 20161025223048.1ce89f65@slate.meme.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

Changes are, roughly:

Put pg_log_file in alphabetical order in the file name listing
and section body.

Put pg_current_logfile in alphabetical order in the function
name listing and section body.

1 argument functions don't seem to have a parameter name
when listed in documentation tables, just a data type,
so I got rid of the parameter name for pg_current_logfile().

Cleaned up the wording and provided more detail.

Added hyperlink to log_destination config parameter.

Added markup to various system values. The markup does
not stand out very well in the html docs, but that's a different
issue and should be fixed by changing the markup processing.
(I used the markup found in the log_destination()
config parameter docs.)

pg_current_logfile does not seem related to pg_listening_channels
or pg_notification_queue_usage so I moved the latter 2 index
entries closer to their text.

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?

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?

In other words,
as long as you're making the content of pg_log_file
a data structure that contains more than just a single
file name you may as well make that data structure
something well-defined, easily parseable in shell, extensible,
and informative.

Hope to provide more feedback soon.

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-v6.diff.patch text/x-patch 4.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Venkata B Nagothi 2016-10-26 03:32:44 Re: patch proposal
Previous Message Amit Kapila 2016-10-26 03:09:15 Re: Declarative partitioning - another take