> On 19 Jun 2017, at 17:32, Tom Lane <t...@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
0001-Free-allocated-memory-when-2-patterns-used.patch
Description: Binary data
0002-Use-consistent-function-comments-for-metacommands.patch
Description: Binary data
0003-Server-version-check.patch
Description: Binary data
0004-Include-all-details-from-normal-in-verbose-more-for-.patch
Description: Binary data
0005-Use-PQExpBuffer-for-all-table-titles.patch
Description: Binary data
0006-Improve-consistency-in-object-not-found-notices-in-p.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers