Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files
Daniel Gustafsson writes: > On 27 Jul 2017, at 19:40, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files
> On 27 Jul 2017, at 19:40, Tom Lane wrote: > > Daniel Gustafsson writes: >>> On 19 Jun 2017, at 17:32, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files
Daniel Gustafsson writes: >> On 19 Jun 2017, at 17:32, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files
> On 19 Jun 2017, at 17:32, Tom Lane 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
0001-Free-allocated-memory-when-2-patterns-used.patch
Description: Binary data
0002-Use-consistent-function-comments
Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files
"David G. Johnston" writes: > On Mon, Jun 19, 2017 at 2:53 PM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files
On Mon, Jun 19, 2017 at 2:53 PM, Tom Lane wrote:
> "David G. Johnston" 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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files
"David G. Johnston" 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files
On Mon, Jun 19, 2017 at 2:25 PM, Peter Eisentraut 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files
Peter Eisentraut 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 relations Schema | Name | Type | Owner +---+---+-- public | fool1 | table | postgres public | 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql's \d and \dt are sending their complaints to different output files
Dilip Kumar writes:
> On Mon, Jun 19, 2017 at 6:30 PM, Oleksandr Shulgin
> 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] psql's \d and \dt are sending their complaints to different output files
Hello Hackers, 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. By browsing and grepping src/bin/psql/describe.c I can see that psql_error() (used in \d) is prevalent, as opposed to fprintf() to pset.queryFout (used in \dt), but then it's a question if it should be treated as an error or not. 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. Regards, -- Alex
