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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
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 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
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"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?
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
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 normallyNote 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
Attachments:
0001-Free-allocated-memory-when-2-patterns-used.patchapplication/octet-stream; name=0001-Free-allocated-memory-when-2-patterns-used.patchDownload
From d578eb8812f19dd4cfa4254c1417230805c31fb8 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Mon, 26 Jun 2017 13:41:29 +0200
Subject: [PATCH 1/6] Free allocated memory when 2 patterns used
Ensure to free pattern2 aswell when allocated.
---
src/bin/psql/command.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 14c64208ca..0468bc37f6 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -806,6 +806,9 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
pattern2 = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
success = listDbRoleSettings(pattern, pattern2);
+
+ if (pattern2)
+ free(pattern2);
}
else
status = PSQL_CMD_UNKNOWN;
--
2.13.0.rc0.45.ge2cb6ab.dirty
0002-Use-consistent-function-comments-for-metacommands.patchapplication/octet-stream; name=0002-Use-consistent-function-comments-for-metacommands.patchDownload
From 1f8ba11198633617e588990c8fdabf668a5f2e3c Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Mon, 26 Jun 2017 13:42:18 +0200
Subject: [PATCH 2/6] Use consistent function comments for metacommands
/* /* \dfoo
* \dfoo instead of *
---
src/bin/psql/describe.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index e6833eced5..2ac03533bc 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -54,7 +54,8 @@ static bool listOneExtensionContents(const char *extname, const char *oid);
*/
-/* \da
+/*
+ * \da
* Takes an optional regexp to select particular aggregates
*/
bool
@@ -131,7 +132,8 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem)
return true;
}
-/* \dA
+/*
+ * \dA
* Takes an optional regexp to select particular access methods
*/
bool
@@ -198,7 +200,8 @@ describeAccessMethods(const char *pattern, bool verbose)
return true;
}
-/* \db
+/*
+ * \db
* Takes an optional regexp to select particular tablespaces
*/
bool
@@ -283,7 +286,8 @@ describeTablespaces(const char *pattern, bool verbose)
}
-/* \df
+/*
+ * \df
* Takes an optional regexp to select particular functions.
*
* As with \d, you can specify the kinds of functions you want:
@@ -696,7 +700,8 @@ describeTypes(const char *pattern, bool verbose, bool showSystem)
}
-/* \do
+/*
+ * \do
* Describe operators
*/
bool
@@ -5040,7 +5045,8 @@ listOneExtensionContents(const char *extname, const char *oid)
return true;
}
-/* \dRp
+/*
+ * \dRp
* Lists publications.
*
* Takes an optional regexp to select particular publications
@@ -5106,7 +5112,8 @@ listPublications(const char *pattern)
return true;
}
-/* \dRp+
+/*
+ * \dRp+
* Describes publications including the contents.
*
* Takes an optional regexp to select particular publications
@@ -5227,7 +5234,8 @@ describePublications(const char *pattern)
return true;
}
-/* \dRs
+/*
+ * \dRs
* Describes subscriptions.
*
* Takes an optional regexp to select particular subscriptions
--
2.13.0.rc0.45.ge2cb6ab.dirty
0003-Server-version-check.patchapplication/octet-stream; name=0003-Server-version-check.patchDownload
From 1efae71988c8392d8c7f045325e97dd4bcbbc8c2 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Mon, 26 Jun 2017 13:46:35 +0200
Subject: [PATCH 3/6] Server version check
---
src/bin/psql/describe.c | 44 ++++++++++++++++++++++----------------------
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 2ac03533bc..85fc22d942 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3230,34 +3230,34 @@ listDbRoleSettings(const char *pattern, const char *pattern2)
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
+ bool havewhere;
initPQExpBuffer(&buf);
- if (pset.sversion >= 90000)
- {
- bool havewhere;
-
- printfPQExpBuffer(&buf, "SELECT rolname AS \"%s\", datname AS \"%s\",\n"
- "pg_catalog.array_to_string(setconfig, E'\\n') AS \"%s\"\n"
- "FROM pg_db_role_setting AS s\n"
- "LEFT JOIN pg_database ON pg_database.oid = setdatabase\n"
- "LEFT JOIN pg_roles ON pg_roles.oid = setrole\n",
- gettext_noop("Role"),
- gettext_noop("Database"),
- gettext_noop("Settings"));
- havewhere = processSQLNamePattern(pset.db, &buf, pattern, false, false,
- NULL, "pg_roles.rolname", NULL, NULL);
- processSQLNamePattern(pset.db, &buf, pattern2, havewhere, false,
- NULL, "pg_database.datname", NULL, NULL);
- appendPQExpBufferStr(&buf, "ORDER BY 1, 2;");
- }
- else
+ if (pset.sversion < 90000)
{
- fprintf(pset.queryFout,
- _("No per-database role settings support in this server version.\n"));
- return false;
+ char sverbuf[32];
+
+ psql_error("The server (version %s) does not support per-database role settings.\n",
+ formatPGVersionNumber(pset.sversion, false,
+ sverbuf, sizeof(sverbuf)));
+ return true;
}
+ printfPQExpBuffer(&buf, "SELECT rolname AS \"%s\", datname AS \"%s\",\n"
+ "pg_catalog.array_to_string(setconfig, E'\\n') AS \"%s\"\n"
+ "FROM pg_db_role_setting AS s\n"
+ "LEFT JOIN pg_database ON pg_database.oid = setdatabase\n"
+ "LEFT JOIN pg_roles ON pg_roles.oid = setrole\n",
+ gettext_noop("Role"),
+ gettext_noop("Database"),
+ gettext_noop("Settings"));
+ havewhere = processSQLNamePattern(pset.db, &buf, pattern, false, false,
+ NULL, "pg_roles.rolname", NULL, NULL);
+ processSQLNamePattern(pset.db, &buf, pattern2, havewhere, false,
+ NULL, "pg_database.datname", NULL, NULL);
+ appendPQExpBufferStr(&buf, "ORDER BY 1, 2;");
+
res = PSQLexec(buf.data);
if (!res)
return false;
--
2.13.0.rc0.45.ge2cb6ab.dirty
0004-Include-all-details-from-normal-in-verbose-more-for-.patchapplication/octet-stream; name=0004-Include-all-details-from-normal-in-verbose-more-for-.patchDownload
From d0500fae854076aeff45fd81c8eac5d6b3556e11 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Mon, 26 Jun 2017 13:50:58 +0200
Subject: [PATCH 4/6] Include all details from normal in verbose more for \dRp
In general the meta commands include additional information when in
verbose mode together with what was printed in normal mode. Include
the name and owner in \dRp+ to keep consistency with \dRp.
---
src/bin/psql/describe.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 85fc22d942..24d77f84f2 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -5138,8 +5138,9 @@ describePublications(const char *pattern)
initPQExpBuffer(&buf);
printfPQExpBuffer(&buf,
- "SELECT oid, pubname, puballtables, pubinsert,\n"
- " pubupdate, pubdelete\n"
+ "SELECT oid, pubname, "
+ " pg_catalog.pg_get_userbyid(pubowner) AS owner,"
+ " puballtables, pubinsert, pubupdate, pubdelete "
"FROM pg_catalog.pg_publication\n");
processSQLNamePattern(pset.db, &buf, pattern, false, false,
@@ -5158,13 +5159,13 @@ describePublications(const char *pattern)
for (i = 0; i < PQntuples(res); i++)
{
const char align = 'l';
- int ncols = 4;
+ int ncols = 6;
int nrows = 1;
int tables = 0;
PGresult *tabres;
char *pubid = PQgetvalue(res, i, 0);
char *pubname = PQgetvalue(res, i, 1);
- bool puballtables = strcmp(PQgetvalue(res, i, 2), "t") == 0;
+ bool puballtables = strcmp(PQgetvalue(res, i, 3), "t") == 0;
int j;
PQExpBufferData title;
printTableOpt myopt = pset.popt.topt;
@@ -5174,15 +5175,19 @@ describePublications(const char *pattern)
printfPQExpBuffer(&title, _("Publication %s"), pubname);
printTableInit(&cont, &myopt, title.data, ncols, nrows);
+ printTableAddHeader(&cont, gettext_noop("Name"), true, align);
+ printTableAddHeader(&cont, gettext_noop("Owner"), true, align);
printTableAddHeader(&cont, gettext_noop("All tables"), true, align);
printTableAddHeader(&cont, gettext_noop("Inserts"), true, align);
printTableAddHeader(&cont, gettext_noop("Updates"), true, align);
printTableAddHeader(&cont, gettext_noop("Deletes"), true, align);
+ printTableAddCell(&cont, PQgetvalue(res, i, 1), false, false);
printTableAddCell(&cont, PQgetvalue(res, i, 2), false, false);
printTableAddCell(&cont, PQgetvalue(res, i, 3), false, false);
printTableAddCell(&cont, PQgetvalue(res, i, 4), false, false);
printTableAddCell(&cont, PQgetvalue(res, i, 5), false, false);
+ printTableAddCell(&cont, PQgetvalue(res, i, 6), false, false);
if (!puballtables)
{
--
2.13.0.rc0.45.ge2cb6ab.dirty
0005-Use-PQExpBuffer-for-all-table-titles.patchapplication/octet-stream; name=0005-Use-PQExpBuffer-for-all-table-titles.patchDownload
From 03e0062c55d10ce0ce2ca770d68b8f54a3550994 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Mon, 26 Jun 2017 14:39:18 +0200
Subject: [PATCH 5/6] Use PQExpBuffer for all table titles
Rather than snprintf()'ing into a fixed buffer for meta command table
titles in some cases, always use a PQExpBuffer as was done for most
other.
---
src/bin/psql/describe.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 24d77f84f2..f72670eb23 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -4126,7 +4126,7 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
{
PQExpBufferData buf;
PGresult *res;
- char title[1024];
+ PQExpBufferData title;
printQueryOpt myopt = pset.popt;
static const bool translate_columns[] = {true, false, false};
@@ -4182,11 +4182,13 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
return false;
myopt.nullPrint = NULL;
+ initPQExpBuffer(&title);
if (nspname)
- sprintf(title, _("Text search parser \"%s.%s\""), nspname, prsname);
+ printfPQExpBuffer(&title, _("Text search parser \"%s.%s\""),
+ nspname, prsname);
else
- sprintf(title, _("Text search parser \"%s\""), prsname);
- myopt.title = title;
+ printfPQExpBuffer(&title, _("Text search parser \"%s\""), prsname);
+ myopt.title = title.data;
myopt.footers = NULL;
myopt.topt.default_footer = false;
myopt.translate_header = true;
@@ -4214,11 +4216,13 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
return false;
myopt.nullPrint = NULL;
+ resetPQExpBuffer(&title);
if (nspname)
- sprintf(title, _("Token types for parser \"%s.%s\""), nspname, prsname);
+ printfPQExpBuffer(&title, _("Token types for parser \"%s.%s\""),
+ nspname, prsname);
else
- sprintf(title, _("Token types for parser \"%s\""), prsname);
- myopt.title = title;
+ printfPQExpBuffer(&title, _("Token types for parser \"%s\""), prsname);
+ myopt.title = title.data;
myopt.footers = NULL;
myopt.topt.default_footer = true;
myopt.translate_header = true;
@@ -4227,6 +4231,7 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
+ termPQExpBuffer(&title);
PQclear(res);
return true;
}
@@ -5017,7 +5022,7 @@ listOneExtensionContents(const char *extname, const char *oid)
{
PQExpBufferData buf;
PGresult *res;
- char title[1024];
+ PQExpBufferData title;
printQueryOpt myopt = pset.popt;
initPQExpBuffer(&buf);
@@ -5035,12 +5040,14 @@ listOneExtensionContents(const char *extname, const char *oid)
return false;
myopt.nullPrint = NULL;
- snprintf(title, sizeof(title), _("Objects in extension \"%s\""), extname);
- myopt.title = title;
+ initPQExpBuffer(&title);
+ printfPQExpBuffer(&title, _("Objects in extension \"%s\""), extname);
+ myopt.title = title.data;
myopt.translate_header = true;
printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
+ termPQExpBuffer(&title);
PQclear(res);
return true;
}
--
2.13.0.rc0.45.ge2cb6ab.dirty
0006-Improve-consistency-in-object-not-found-notices-in-p.patchapplication/octet-stream; name=0006-Improve-consistency-in-object-not-found-notices-in-p.patchDownload
From d382a1933f3b5875c8e7f5b688b9104543263d3c Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Mon, 26 Jun 2017 17:26:47 +0200
Subject: [PATCH 6/6] Improve consistency in "object not found" notices in psql
When a \dXXX metacommand failed to find an object, there were some
variability on the response. This aims to make it more consistent by
applying:
* Use psql_error() for all messages regarding objects not being found.
It's arguably not an error per se, but at least printing all notices
to the same output channel makes it consistent.
* Formulate the notices in the same way for all objects
* Guard against pattern being NULL
* Never be completely silent unless in QUIET mode, add not found notice
on the object which lacked it
---
src/bin/psql/describe.c | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f72670eb23..30ca13d024 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1322,8 +1322,13 @@ describeTableDetails(const char *pattern, bool verbose, bool showSystem)
if (PQntuples(res) == 0)
{
if (!pset.quiet)
- psql_error("Did not find any relation named \"%s\".\n",
- pattern);
+ {
+ if (pattern)
+ psql_error("Did not find any relation named \"%s\".\n",
+ pattern);
+ else
+ psql_error("Did not find any relations.\n");
+ }
PQclear(res);
return false;
}
@@ -3265,9 +3270,9 @@ listDbRoleSettings(const char *pattern, const char *pattern2)
if (PQntuples(res) == 0 && !pset.quiet)
{
if (pattern)
- fprintf(pset.queryFout, _("No matching settings found.\n"));
+ psql_error("Did not find any settings matching \"%s\".\n", pattern);
else
- fprintf(pset.queryFout, _("No settings found.\n"));
+ psql_error("Did not find any settings.\n");
}
else
{
@@ -3430,9 +3435,9 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
if (PQntuples(res) == 0 && !pset.quiet)
{
if (pattern)
- fprintf(pset.queryFout, _("No matching relations found.\n"));
+ psql_error("Did not find any relation named \"%s\".\n", pattern);
else
- fprintf(pset.queryFout, _("No relations found.\n"));
+ psql_error("Did not find any relations.\n");
}
else
{
@@ -5163,6 +5168,21 @@ describePublications(const char *pattern)
return false;
}
+ if (PQntuples(res) == 0)
+ {
+ if (!pset.quiet)
+ {
+ if (pattern)
+ psql_error("Did not find publication named \"%s\".\n", pattern);
+ else
+ psql_error("Did not find any publications.\n");
+ }
+
+ termPQExpBuffer(&buf);
+ PQclear(res);
+ return false;
+ }
+
for (i = 0; i < PQntuples(res); i++)
{
const char align = 'l';
--
2.13.0.rc0.45.ge2cb6ab.dirty
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers