> 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