Обсуждение: [HACKERS] psql's \d and \dt are sending their complaints to different output files

On Mon, Jun 19, 2017 at 6:30 PM, Oleksandr Shulgin
<oleksandr.shulgin@zalando.de> wrote:
> I think can be helpful, though rarely, to be able to send the output of \d*
> commands to a file.  At the same time it would be nice to see the message on
> stderr instead of appending to the output file, in case the relation was not
> found, because of less head-scratching needed to realize the problem.  So
> I'd vote for changing \dt (and alike) behavior to use stderr.

+1

And, we can also change the error message to be more consistent.

\d says "Did not find any relation named "xxx" ". whereas \dt says "No
matching relations found".

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Dilip Kumar <dilipbalaut@gmail.com> writes:
> On Mon, Jun 19, 2017 at 6:30 PM, Oleksandr Shulgin
> <oleksandr.shulgin@zalando.de> wrote:
>> I think can be helpful, though rarely, to be able to send the output of \d*
>> commands to a file.  At the same time it would be nice to see the message on
>> stderr instead of appending to the output file, in case the relation was not
>> found, because of less head-scratching needed to realize the problem.  So
>> I'd vote for changing \dt (and alike) behavior to use stderr.

> +1

> And, we can also change the error message to be more consistent.
> \d says "Did not find any relation named "xxx" ". whereas \dt says "No
> matching relations found".

So, if we're getting into enforcing consistency in describe.c, there's
lots to do.

* listDbRoleSettings does this for a server too old to have the desired
feature:
    fprintf(pset.queryFout,    _("No per-database role settings support in this server version.\n"));

Just about every other function handles too-old-server like this:
    psql_error("The server (version %s) does not support full text search.\n",

* listTables and listDbRoleSettings do this for zero matches:
if (PQntuples(res) == 0 && !pset.quiet){    if (pattern)        fprintf(pset.queryFout, _("No matching relations
found.\n"));   else        fprintf(pset.queryFout, _("No relations found.\n"));}else    ... print the result normally 

Note the test on quiet mode, as well as the choice of two messages
depending on whether the command had a pattern argument or not.

Other functions in describe.c mostly follow this pattern:
if (PQntuples(res) == 0){    if (!pset.quiet)        psql_error("Did not find any relation named \"%s\".\n",
      pattern);    PQclear(res);    return false;} 

So this has a different behavior in quiet mode, which is to print
*nothing* to queryFout rather than a result table with zero entries.
That's probably bogus.  It will also crash, on some machines, if pattern
is NULL, although no doubt nobody's noticed because there would always be
a match.  (One call site does defend itself against null pattern, and it
uses two messages like the previous example.)

So we've got at least three things to normalize:

* fprintf(pset.queryFout) vs. psql_error()

* message wording inconsistencies

* behavior with -q and no matches.

Anybody want to draft a patch?
        regards, tom lane

On 6/19/17 09:00, Oleksandr Shulgin wrote:
> I wonder if it is intentional that \d complains on stderr if it cannot
> find relations to match, but \dt prints the message to the current
> output file?
> 
> postgres=# \d xxx
> Did not find any relation named "xxx".
> postgres=# \dt xxx
> No matching relations found.
> 
> I've noticed the difference exactly because my output was
> (accidentally) redirected to a file and I didn't see the complaint from
> the 2nd backslash command.

I think this is intentional.

The first command is "show me relation xxx", and that gives an error
message if it does not exist (and would also create an appropriate exit
status if certain options are used).

The second command says "show me all relations matched 'xxx'".  The
result of this is successful execution showing nothing.  The message it
prints is a "courtesy" message.

Maybe there is something to tweak here, but the basic distinction of
what is an error and what isn't should be preserved.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 6/19/17 09:00, Oleksandr Shulgin wrote:
>> postgres=# \d xxx
>> Did not find any relation named "xxx".
>> postgres=# \dt xxx
>> No matching relations found.

> I think this is intentional.

> The first command is "show me relation xxx", and that gives an error
> message if it does not exist (and would also create an appropriate exit
> status if certain options are used).
> The second command says "show me all relations matched 'xxx'".  The
> result of this is successful execution showing nothing.  The message it
> prints is a "courtesy" message.

I don't buy that line of argument one bit, because both commands take
pattern arguments.

regression=# \d fool*
Did not find any relation named "fool*".
regression=# \dt fool*
No matching relations found.
regression=# create table fool1(f1 int);
CREATE TABLE
regression=# create table fool2(f1 int);
CREATE TABLE
regression=# \d fool*              Table "public.fool1"Column |  Type   | Collation | Nullable | Default 
--------+---------+-----------+----------+---------f1     | integer |           |          | 
              Table "public.fool2"Column |  Type   | Collation | Nullable | Default 
--------+---------+-----------+----------+---------f1     | integer |           |          | 

regression=# \dt fool*        List of relationsSchema | Name  | Type  |  Owner   
--------+-------+-------+----------public | fool1 | table | postgrespublic | fool2 | table | postgres
(2 rows)

AFAICS, this is just randomly different responses for identical
situations.

There's certainly room to quibble about whether this is an error
condition or not, but I'm not very sure which side of that argument
I'd come down on.  In any case the main point is that different
\d commands are doing different things for no very obvious reason.
        regards, tom lane

On Mon, Jun 19, 2017 at 2:25 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 6/19/17 09:00, Oleksandr Shulgin wrote:
>> I wonder if it is intentional that \d complains on stderr if it cannot
>> find relations to match, but \dt prints the message to the current
>> output file?
>>
>> postgres=# \d xxx
>> Did not find any relation named "xxx".
>> postgres=# \dt xxx
>> No matching relations found.
>>>
> The first command is "show me relation xxx" and that gives an error
> message if it does not exist (and would also create an appropriate exit
> status if certain options are used).
>
> The second command says "show me all relations matched 'xxx'".  The
> result of this is successful execution showing nothing.  The message it
> prints is a "courtesy" message.

The docs indicate the optional argument to both is a pattern so I'm
not seeing why they are treated differently.

The docs also indicate that we don't include materialized views as
part of "\d" which seems like an oversight somewhere.

It sounds like, though isn't specified anywhere, the while you can
write "\dit" to get a subset of all available options omitting all of
the options leaves you with "\d" which is equivalent to specifying
them all.  Which leads on to believe the output from both should be in
sync.

David J.

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> The docs also indicate that we don't include materialized views as
> part of "\d" which seems like an oversight somewhere.

Where are you reading that?  Experimentation shows that "\d" does include
matviews, and that matches the code, which has this as the default
expansion of \d:
               /* standard listing of interesting things */               success = listTables("tvmsE", NULL,
show_verbose,show_system); 
        regards, tom lane

On Mon, Jun 19, 2017 at 2:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "David G. Johnston" <david.g.johnston@gmail.com> writes:
>> The docs also indicate that we don't include materialized views as
>> part of "\d" which seems like an oversight somewhere.
>
> Where are you reading that?

https://www.postgresql.org/docs/9.6/static/app-psql.html

First sentence:

"For each relation (table, view, index, sequence, or foreign table) or
composite type matching the pattern..."

I was expecting "materialized view" to be one of the parenthetical options.

> Experimentation shows that "\d" does include
> matviews, and that matches the code, which has this as the default
> expansion of \d:
>
>                 /* standard listing of interesting things */
>                 success = listTables("tvmsE", NULL, show_verbose, show_system);
>

\dT / "composite type" seems to be a special case.

David J.

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Mon, Jun 19, 2017 at 2:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Where are you reading that?

> https://www.postgresql.org/docs/9.6/static/app-psql.html

> First sentence:
> "For each relation (table, view, index, sequence, or foreign table) or
> composite type matching the pattern..."

Ah.  Clearly missed in the matviews patch (which I now see also missed
updating some relevant comments, though it did change the code).

Will fix.
        regards, tom lane

> On 19 Jun 2017, at 17:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> So, if we're getting into enforcing consistency in describe.c, there's
> lots to do.
>
> * listDbRoleSettings does this for a server too old to have the desired
> feature:
>
>         fprintf(pset.queryFout,
>         _("No per-database role settings support in this server version.\n"));
>
> Just about every other function handles too-old-server like this:
>
>         psql_error("The server (version %s) does not support full text search.\n”,

Addressed in attached patch, see list of patches below.

> * listTables and listDbRoleSettings do this for zero matches:
>
>     if (PQntuples(res) == 0 && !pset.quiet)
>     {
>         if (pattern)
>             fprintf(pset.queryFout, _("No matching relations found.\n"));
>         else
>             fprintf(pset.queryFout, _("No relations found.\n"));
>     }
>     else
>         ... print the result normally
>
> Note the test on quiet mode, as well as the choice of two messages
> depending on whether the command had a pattern argument or not.
>
> Other functions in describe.c mostly follow this pattern:
>
>     if (PQntuples(res) == 0)
>     {
>         if (!pset.quiet)
>             psql_error("Did not find any relation named \"%s\".\n",
>                        pattern);
>         PQclear(res);
>         return false;
>     }
>
> So this has a different behavior in quiet mode, which is to print
> *nothing* to queryFout rather than a result table with zero entries.
> That's probably bogus.

There are two cases in verbose metacommands, we either print a generic “List of
XXX” title with a table following it, or multiple tables (with additional
non-table data) a per-table title.  For unmatched commands in the former case,
we print the title and an empty table in verbose mode, the latter case prints a
“Did not found XXX” message.  When QUIET is set to on, the latter case doesn’t
print anything for the most case.

Not printing anything is clearly not helpful, but choosing what to print
requires some thinking so before hacking on that I figured I’d solicit
opinions.  We can either keep printing a “Did not find ..” message; change a
per-table title to a generic one and include an empty table; a mix as today;
something completely different.  What preferences on output are there?

Personally I’m in favour of trying to add an empty table to all verbose
commands, simply because that’s what I expect to see when using psql.

> It will also crash, on some machines, if pattern
> is NULL, although no doubt nobody's noticed because there would always be
> a match.  (One call site does defend itself against null pattern, and it
> uses two messages like the previous example.)

Addressed in attached patch, see list of patches below.

> So we've got at least three things to normalize:
>
> * fprintf(pset.queryFout) vs. psql_error()
>
> * message wording inconsistencies
>
> * behavior with -q and no matches.
>
> Anybody want to draft a patch?

I poked around the code with an eye to improving consistency, and included some
more things that caught my eye.  Rather than attaching one patch with
everything, I pulled out the various proposals as separate patches:

0001 - Not really a consistency thing but included here since it’s sort of
related.  A small memleak on pattern2 spotted while reading code, unless I’m
missing where it’s freed.

0002 - While on the topic of consistency, tiny function comment reformat on the
metacommand function because I have comment-OCD, feel free to ignore.

0003 - Apply the same server version check pattern in listDbRoleSettings() as
elsewhere in other functions

0004 - Most verbose metacommands include additional information on top of what
is in the normal output, while \dRp+ dropped two columns (moving one to the
title).  This include the information from \dRp in \dRp+.  Having the pubname
in the title as well as the table is perhaps superfuous, but consistent with
other commands so opted for it.

0005 - Most tables titles were created using a PQExpBuffer with two using a
char buffer and snprintf().  Moved to using a PQExpBuffer instead in all cases
since it’s consistent and clean (not that the buffers risked overflowing or
anything like that, but I like the PQExpBuffer API).

0006 - Moves to psql_error() for all not-found messages and the same language
for all messages.  I don’t think these are errors per se, but psql_error() was
the most prevelant option used, so went there for consistency as a starting
point for discussion.  Also adds appropriate NULL deref check on pattern and
adds a not-found message in describePublications() which previously didn’t
print anything at all on not-found.

Hope there is something of interest in there.

cheers ./daniel





-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 19 Jun 2017, at 17:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So, if we're getting into enforcing consistency in describe.c, there's
>> lots to do.

> Addressed in attached patch, see list of patches below.

I've pushed most of this.  There are a couple of remaining
inconsistencies:

listTables and listDbRoleSettings print a custom message rather than
an empty table for no matches (but in QUIET mode they just do the
latter).  I think that's actually a good decision for listDbRoleSettings,
because the user might be confused about which pattern is which, and
we can straighten him out with a custom error message.  But the only
good argument for listTables behaving that way is that people are used
to it.  Should we override backwards compatibility to the extent of
dropping the custom messages in listTables, and just printing an empty
table-of-tables?

> 0004 - Most verbose metacommands include additional information on top of what
> is in the normal output, while \dRp+ dropped two columns (moving one to the
> title).  This include the information from \dRp in \dRp+.  Having the pubname
> in the title as well as the table is perhaps superfuous, but consistent with
> other commands so opted for it.

I'm not sure about this one.  It definitely seems bogus that \dRp+ is
omitting the owner column, but I'm less excited about duplicating the
pubname.  We'd better make up our minds before v10 freezes, though.
Anybody else have an opinion?
        regards, tom lane

> On 27 Jul 2017, at 19:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 19 Jun 2017, at 17:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> So, if we're getting into enforcing consistency in describe.c, there's
>>> lots to do.
>
>> Addressed in attached patch, see list of patches below.
>
> I've pushed most of this.

Thanks!

> listTables and listDbRoleSettings print a custom message rather than
> an empty table for no matches (but in QUIET mode they just do the
> latter).  I think that's actually a good decision for listDbRoleSettings,
> because the user might be confused about which pattern is which, and
> we can straighten him out with a custom error message.  But the only
> good argument for listTables behaving that way is that people are used
> to it.

Which is a pretty good argument for not changing it.

> Should we override backwards compatibility to the extent of
> dropping the custom messages in listTables, and just printing an empty
> table-of-tables?
>
>> 0004 - Most verbose metacommands include additional information on top of what
>> is in the normal output, while \dRp+ dropped two columns (moving one to the
>> title).  This include the information from \dRp in \dRp+.  Having the pubname
>> in the title as well as the table is perhaps superfuous, but consistent with
>> other commands so opted for it.
>
> I'm not sure about this one.  It definitely seems bogus that \dRp+ is
> omitting the owner column, but I'm less excited about duplicating the
> pubname.

It’s indeed a bit silly to duplicate the information like that.

Another option would perhaps be to move to a format like the one in \dx+, and
only list the tables per subscription like how extension objects are listed.
Not sure if that’s any better here though.

cheers ./daniel

Daniel Gustafsson <daniel@yesql.se> writes:
> On 27 Jul 2017, at 19:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> listTables and listDbRoleSettings print a custom message rather than
>> an empty table for no matches (but in QUIET mode they just do the
>> latter).  I think that's actually a good decision for listDbRoleSettings,
>> because the user might be confused about which pattern is which, and
>> we can straighten him out with a custom error message.  But the only
>> good argument for listTables behaving that way is that people are used
>> to it.  

> Which is a pretty good argument for not changing it.

Yeah, not hearing any votes for changing it, I'll leave it be.

>> I'm not sure about this one.  It definitely seems bogus that \dRp+ is
>> omitting the owner column, but I'm less excited about duplicating the
>> pubname.

> It’s indeed a bit silly to duplicate the information like that.

The minimum change here seems to be to add the owner column, so I did
that.
        regards, tom lane