Re: Submission Review: User control over psql error stream

Lists: pgsql-hackers
From: Alastair Turner <bell(at)ctrlf5(dot)co(dot)za>
To: "Karl O(dot) Pinc" <kop(at)meme(dot)com>
Cc: Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Submission Review: User control over psql error stream
Date: 2012-12-09 20:13:32
Message-ID: CAFgq2fVX7rY9f1rrU01_fNRUYt=fq4du9D_PPVjqHGGLmrcZNA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hi Karl,

I have given the patch a quick review and read the related mails
following its initial submission.

I agree with that functionality along these lines is desirable. The
ability to manage output from within psql at least as richly as is
possible with shell redirection - and change it between commands
because it is accessible from with psql - would be great.

The basics of the review were fine: the patch applied with minimal
fuzz and compiled cleanly.

The implementation of the functionality - as built for your specific
requirement - concerns me in a few ways though:

- It's closed ended - there are three things about error output which
affect where it's written to: does it go to query output, does it go
somewhere else (a file or pipe), does it get displayed as well as
going to the other destination. The patch addresses the first and
third questions without making allowance for asking or dealing with
second one. Internally I think this should be two bools rather than a
custom tri-state, mainly because this would allow the addition of the
error file option (in another patch, when the time came) with minimum
intrusion.

- \pset is not the right place for this switch - all the documentation
(including \?) indicate that psets are for controlling the display of
result tables and this doesn't fit with the other options to \pset.
Since this affects what goes into the output file I would think that
this should be an option to \o. Maybe \o& ?

Marked "Returned to Author".

Regards,
Alastair.


From: "Karl O(dot) Pinc" <kop(at)meme(dot)com>
To: Alastair Turner <bell(at)ctrlf5(dot)co(dot)za>
Cc: Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Submission Review: User control over psql error stream
Date: 2012-12-09 21:58:26
Message-ID: 1355090306.17572.3@mofo
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hi Alastair,

On 12/09/2012 02:13:32 PM, Alastair Turner wrote:
> Hi Karl,
>
> I have given the patch a quick review and read the related mails
> following its initial submission.

Thank you very much.

> - It's closed ended - there are three things about error output which
> affect where it's written to: does it go to query output, does it go
> somewhere else (a file or pipe), does it get displayed as well as
> going to the other destination. The patch addresses the first and
> third questions without making allowance for asking or dealing with
> second one. Internally I think this should be two bools rather than a
> custom tri-state, mainly because this would allow the addition of the
> error file option (in another patch, when the time came) with minimum
> intrusion.

I was thinking along the same lines, that case 2) stderr to a file
or pipe needs addressing. I think it's necessary to address the
issue now. Otherwise we risk cluttering up the syntax in
inhospitable ways.

> - \pset is not the right place for this switch - all the
> documentation
> (including \?) indicate that psets are for controlling the display of
> result tables and this doesn't fit with the other options to \pset.
> Since this affects what goes into the output file I would think that
> this should be an option to \o. Maybe \o& ?

Before talking about syntax let's get semantics out of the way.

I think we're agreed that we want to be able to mix stderr
in with stdout. It makes sense to me to be able to say,
as in shell, send stderr to wherever stdout is going,
without having to re-specify where stdout has actually gone.
This is especially needed in the case of pipes.

The big question is whether, after redirecting stderr
to stdout, redirecting stdout to someplace else also
then changes where stderr goes. My inclination is that
it does not, that it work like shell (without the
godawful syntax). In shell, cat foo 2>&1 >/dev/null
sends stderr to stdout and discards stdout.
(In psudocode: stderr=stdout; stdout=/dev/null)
Likewise, cat foo >/dev/null 2>&1 discards
both stderr and stdout. (stdout=/dev/null; stderr=stdout)

Regards syntax:

It seems cleanest to make the syntax involved in redirection of stderr
look (almost) like redirection of stdout. And likewise
redirecting stdout should look like redirecting stderr.
The latter would then allow the semantics of redirecting
stdout to wherever stderr is going, without having to
re-specify exactly where stderr has been sent.

I don't have a good sense of what you're proposing regards using
a '\o&' type of syntax. (But see below.) Please elaborate if you'd
like me to pursue this direction.

My thoughts are having a \e command that would look
like \o.

So:

\o file stdout to file
\o |prog stdout to pipe
\oe stdout to wherever stderr is going
\o stdout to /dev/stdout

\e file stderr to file
\e |prog stderr to pipe
\eo stderr to wherever stdout is going
\e stderr to /dev/stderr

The above seem to be the semantic permutations the syntax needs to
handle. It would be nice to be able to not have to
introduce a new \ command, but I can't think of a clear
way to shoehorn stderr into \o. The best I can
come up with, based on your & idea, is something like:
(Text enclosed in [] is optional.)

\o[o][&] file stdout to file
\o[o][&] |prog stdout to pipe
\o[o]&e stdout to wherever stderr is going
\o[&] stdout to /dev/stdout
\o[&o] stdout to wherever stdout is going (noop)

\oe[&] file stderr to file
\oe[&] |prog stderr to pipe
\oe&o stderr to wherever stdout is going
\oe[&] stderr to /dev/stderr
\oe[&e] stderr to wherever stderr is going (noop)

It seems cluttered. And the & seems redundant.
Without & it looks like:

\o[o] file stdout to file
\o[o] |prog stdout to pipe
\o[o]e stdout to wherever stderr is going
\o stdout to /dev/stdout

\oe file stderr to file
\oe |prog stderr to pipe
\oeo stderr to wherever stdout is going
\oe stderr to /dev/stderr

I suppose it's also possible to do as shell
does and (probably with & as a syntax token) be
able to use file descriptor numbers directly.
It's not clear to me where the win is in doing that.

Would you be thinking & introduces additional syntax?:

\o[&[o[>]]] file stdout to file
\o[&[o[>]]] |prog stdout to pipe
\o&[o]>e stdout to wherever stderr is going
\o[&[o[>]]] stdout to /dev/stdout

\o&e[>] file stderr to file
\o&e[>] |prog stderr to pipe
\o&e>o stderr to wherever stdout is going
\o&e[>] stderr to /dev/stderr

What sort of future extensibility do we get by
adding syntax to \o as opposed to adding a new command?

I'm unclear on how extending \o would fit in with --output.
(Adding a new \e command would seemingly involve adding
a new command line option.)

Thanks for the help.

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


From: "Karl O(dot) Pinc" <kop(at)meme(dot)com>
To: "Karl O(dot) Pinc" <kop(at)meme(dot)com>
Cc: Alastair Turner <bell(at)ctrlf5(dot)co(dot)za>, Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Submission Review: User control over psql error stream
Date: 2012-12-10 03:00:29
Message-ID: 1355108429.17572.4@mofo
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 12/09/2012 03:58:26 PM, Karl O. Pinc wrote:
> Hi Alastair,
>
> On 12/09/2012 02:13:32 PM, Alastair Turner wrote:

> > - It's closed ended - there are three things about error output
> which
> > affect where it's written to: does it go to query output, does it
> go
> > somewhere else (a file or pipe), does it get displayed as well as
> > going to the other destination. The patch addresses the first and
> > third questions without making allowance for asking or dealing with
> > second one. Internally I think this should be two bools rather than
> a
> > custom tri-state, mainly because this would allow the addition of
> the
> > error file option (in another patch, when the time came) with
> minimum
> > intrusion.
>
> I was thinking along the same lines, that case 2) stderr to a file
> or pipe needs addressing. I think it's necessary to address the
> issue now. Otherwise we risk cluttering up the syntax in
> inhospitable ways.

It occurs to me that my reply does not address the issue
of case 3, displaying or not) errors to the terminal in
addition to wherever else errors are sent.

I think you're right, whether or not errors continue to be sent
to stdout when they are directed elsewhere should be a separate
flag. My inclination is to have another special psql variable
to control this but that would break backwards compatibility.
Instead, let's work out a syntax for the rest of the functionality
and try to fit this in.

One nice thing about special psql variables is that they self-report
their state. I mention this since we're adding more state.
It would be nice if the chosen syntax does not preclude some
additional tweaking to report on the state involving the
output streams. (Although I don't think that needs to be
a part of this patch.)

And oh yes, \e won't work as a command since it's already
taken. Since this is only an issue if \o is not overloaded
I await your review.

Thanks for the help.

Regards,

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


From: Alastair Turner <bell(at)ctrlf5(dot)co(dot)za>
To: "Karl O(dot) Pinc" <kop(at)meme(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Submission Review: User control over psql error stream
Date: 2012-12-28 20:33:03
Message-ID: CAFgq2fW9ykG2NM-XU8umXiv_S9eEzBO4=3y3qAMAQVHrodaDDg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hi Karl,

Sorry for the slow reply ...

Excerpt from Karl O. Pinc <kop(at)meme(dot)com> On Mon, Dec 10, 2012 at 5:00 AM:

> I was thinking along the same lines, that case 2) stderr to a file
> or pipe needs addressing. I think it's necessary to address the
> issue now. Otherwise we risk cluttering up the syntax in
> inhospitable ways.

The discussion needs to be a little broader than stdout and stderr,
there are currently three output streams from psql:
- stdout - prompts, not tabular output such as the results from \set and \c
- stderr - errors, notices, ...
- query output - result from queries and \ commands which return
tables such as \l - this is what is currently piped or redirected by
\o

I see a patch like this adding a fourth - diagnostic output. While
this would probably be the same as stderr initially but I think that
the option to make them subtly different should be kept open.

> It occurs to me that my reply does not address the issue
> of case 3, displaying or not) errors to the terminal in
> addition to wherever else errors are sent.

I don't know of any precedent syntax for this - in *nix shells you'd
pipe to tee rather than modify the redirect in some way.

> I think you're right, whether or not errors continue to be sent
> to stdout when they are directed elsewhere should be a separate
> flag. My inclination is to have another special psql variable
> to control this but that would break backwards compatibility.
> Instead, let's work out a syntax for the rest of the functionality
> and try to fit this in.
>
> One nice thing about special psql variables is that they self-report
> their state. I mention this since we're adding more state.
> It would be nice if the chosen syntax does not preclude some
> additional tweaking to report on the state involving the
> output streams. (Although I don't think that needs to be
> a part of this patch.)

State reporting and reset tend to use the same syntax - the command
without parameters. I think the best route to achieve this would be
for the commands to set a variables which would be reported by \set.

In my initial list of what needed to be specified for output
management I missed one - whether file output should be appended or
should rewrite the file. It's not likely to be particularly useful for
query output but would matter for diagnostics.

Given all of these considerations I would propose changing your
calling syntax to:

\o reset all output redirection

\oq reset query output redirection
\o[q] [>] file query output to file
\o[q] >> file append query output to file
\o[q] | prog pipe query output to program
\o[q] {[>]|>>||}+ display query output on stdout in addition to redirection

\od reset query output redirection
\odq diagnostic output to query output stream
\odq + diagnostic output to stderr and query output
stream (same as \odq and \o + in combination)
\od [>] file diagnostic output to file
\od >> file append diagnostic output to file
\od | prog pipe diagnostic output to program
\od {[>]|>>||}+ display diagnostic output on stderr in addition to
redirection

To meet its original goals your patch would need to implement \o, \od,
\odq and \odq +.

Regards,
Alastair.


From: "Karl O(dot) Pinc" <kop(at)meme(dot)com>
To: Alastair Turner <bell(at)ctrlf5(dot)co(dot)za>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Submission Review: User control over psql error stream
Date: 2012-12-31 18:11:27
Message-ID: 1356977487.10106.0@mofo
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hi Allastair,

On 12/28/2012 02:33:03 PM, Alastair Turner wrote:

> Sorry for the slow reply ...

Such is life.

> The discussion needs to be a little broader than stdout and stderr,
> there are currently three output streams from psql:
> - stdout - prompts, not tabular output such as the results from \set
> and \c
> - stderr - errors, notices, ...
> - query output - result from queries and \ commands which return
> tables such as \l - this is what is currently piped or redirected by
> \o
>
> I see a patch like this adding a fourth - diagnostic output. While
> this would probably be the same as stderr initially but I think that
> the option to make them subtly different should be kept open.

What is the distinction between diagnostic output and stderr?
My thought would be to distinguish between the server error
stream and any errors that psql might generate.

(I have some thoughts on syntax but am not yet ready to respond
to your proposal.)

Regards,

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