Re: Patch to implement pg_current_logfile() function

Le 17/01/2017 à 03:22, Michael Paquier a écrit :
> On Tue, Jan 17, 2017 at 2:21 AM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
>> On January 15, 2017 11:47:51 PM CST, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> On Mon, Jan 16, 2017 at 1:14 PM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
>>> With all those issues fixed, I finish with the attached, that I am
>>> fine to pass down to a committer. I still think that this should be
>>> only one function using a SRF with rows shaped as (type, path) for
>>> simplicity, but as I am visibly outnumbered I won't insist further.
>> That also makes a certain amount of sense to me but I can't say I have thought deeply about it. Haven't paid any attention to this issue and am fine letting things move forward as is.
> Gilles, what's your opinion here? As the author that's your call at
> the end. What the point here is would be to change
> pg_current_logfiles() to something like that:
> =# SELECT * FROM pg_current_logfiles();
> method | file
> --------|--------
> stderr | pg_log/postgresql.log
> csvlog | pg_log/postgresql.csv
> And using this SRF users can filter the method with a WHERE clause.
> And as a result the 1-arg version is removed. No rows are returned if
> current_logfiles does not exist. I don't think there is much need for
> a system view either.

This have already been discuted previoulsy in this thread, one of my
previous patch version has implemented this behavior but we decide that
what we really want is to be able to use the function using the
following simple query:

SELECT pg_read_file(pg_current_logfiles());

and not something like

SELECT pg_read_file(SELECT file FROM pg_current_logfiles() LIMIT 1);
or
SELECT pg_read_file(SELECT file FROM pg_current_logfiles() WHERE
method='stderr');

You can also obtain the "kind" of output from the SRF function using:

SELECT pg_read_file('current_logfiles');

>>> Also, I would rather see an ERROR returned to the user in case of bad
>>> data in current_logfiles, I did not change that either as that's the
>>> original intention of Gilles.
>> I could be wrong but I seem to recall that Robert recommended against an error message. If there is bad data then something is really wrong, up to some sort of an attack on the back end. Because this sort of thing simply shouldn't happen it's enough in my opinion to guard against buffer overruns etc and just get on with life. If something goes unexpectedly and badly wrong with internal data structures in general there would have to be all kinds of additional code to cover every possible problem and that doesn't seem to make sense.
> Hm... OK. At the same time not throwing at least a WARNING is
> confusing, because a NULL result is returned as well even if the input
> method is incorrect and even if the method is correct but it is not
> present in current_logfiles. As the user is thought as a trusted user
> as it has access to this function, I would think that being verbose on
> the error handling, or at least warnings, would make things easier to
> analyze.

I'm not against adding a warning or error message here even if it may
never occurs, but we need a new error code as it seems to me that no
actual error code can be used.

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