Re: psql: display of object comments

Hi all,

I would like to fix psql's incomplete and inconsistent handling of
object comments. I think this subject can be handled separately from
the pg_comments discussion, and we can focus on the
appearance/behavior of psql commands here.

First, As brought up recently [1], comments for the following object
types are not currently displayed by any psql backslash command:

CAST
CONSTRAINT
CONVERSION
FOREIGN DATA WRAPPER
PROCEDURAL LANGUAGE
SERVER

In the case of PROCEDURAL LANGUAGE, it was mere oversight that \dL did
not display comments, and I'm not aware of any reason why we're not
doing so for the other objects above. I'd like to propose adding an
additional column to the respective psql backslash commands for the
above object types which will display any comment associated with the
object.

Second, comments of the following object types would IMO be better
displayed in the object's individual backslash command, and not in \dd
as they are now:

DOMAIN

Third, comments for the following objects are displayed by both \dd
and the object's individual backslash command:

AGGREGATE
FOREIGN TABLE (via \d+)
INDEX
FUNCTION
OPERATOR
SEQUENCE
TABLE
VIEW

Seems like there's no need to add more noise to \dd for these cases.

Fourth, there is a bug in at least the display of comments for indexes
using \d+ (the "Description" is empty when it shouldn't be).

As a summary, here is how I am proposing we display object comments [2]:

1.) Object comments displayed by \dd:

CONSTRAINT
OPERATOR CLASS
OPERATOR FAMILY
RULE
TRIGGER

2.) Object comments displayed in the backslash commands for the object:

AGGREGATE \da
CAST \dC+
COLLATION \dO
COLUMN \d+ tablename
CONVERSION \dc
DATABASE \l+
DOMAIN \dD and \dT+
EXTENSION \dx
FOREIGN DATA WRAPPER \dew
FOREIGN TABLE \det and \d+
FUNCTION \df+
INDEX \di+ and \d+
LARGE OBJECT \dl
OPERATOR \do
PROCEDURAL LANGUAGE \dL
ROLE \dg+
SCHEMA \dn+
SEQUENCE \ds+ and \d+
SERVER \des
TABLE \dt+ and \d+
TABLESPACE \db+
TYPE \dT
TEXT SEARCH CONFIGURATION \dF
TEXT SEARCH DICTIONARY \dFd
TEXT SEARCH PARSER \dFp
TEXT SEARCH TEMPLATE \dFt
VIEW \dv+

A few notes:
a.) As you can see, there's not much consistency in whether we
display object comments only in verbose mode or not. I'm open to
suggestions on how to tweak/standardize this.
b.) I'd rather not have any overlap between groups 1.) and 2.).
c.) \dd doesn't yet display all the object types it should (operator
class & family are missing); that's slated to be fixed with the
pg_comments patch though.

Patch attached and comments welcome; will add to next CF barring
objections. I've also attached a test .sql file I was using to make up
a bunch of comments, it might save you some time if you're interested
in trying the patch out.

--
[1] http://archives.postgresql.org/pgsql-hackers/2011-05/msg00991.php
[2] The object types broken down here should cover all comment-able
types on http://www.postgresql.org/docs/9.1/static/sql-comment.html

On Sat, Jul 9, 2011 at 1:16 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:

Attached is an updated version of this patch, lifted out of the recent
pg_comments patch. With this v2 patch, \dd should properly show just
its five object types, and the psql documentation and help strings
should be fixed.

> Fourth, there is a bug in at least the display of comments for indexes
> using \d+ (the "Description" is empty when it shouldn't be).

BTW, this gripe is being dealt with separately in thread "psql: bogus
descriptions displayed by \d+".

Josh

On Fri, Jul 22, 2011 at 10:44 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> On Sat, Jul 9, 2011 at 1:16 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>
> Attached is an updated version of this patch, lifted out of the recent
> pg_comments patch. With this v2 patch, \dd should properly show just
> its five object types, and the psql documentation and help strings
> should be fixed.

I took a look at this patch today and I think some of these queries
are not quite right. When you do a left join against pg_description,
you have this sort of thing in the WHERE clause:

(d.objsubid IS NULL OR d.objsubid = 0)

I think what you actually want is "AND d.objsubid = 0" in the LEFT
JOIN's "ON" clause. Then you are, in effect, only left joining
against the rows from pg_description where objsubid = 0, and
null-extending if none such is found. I think that's what you want.

I think you can remove the XXX comments, too. Unless I'm
misunderstanding something, using the table to test visibility for
constraints, rules, and triggers seems just right, and opclasses and
opfamilies you have a suitable function available, so those don't seem
problematic. Or am I confused?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jul 25, 2011 at 12:27 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jul 22, 2011 at 10:44 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>> On Sat, Jul 9, 2011 at 1:16 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>>
>> Attached is an updated version of this patch, lifted out of the recent
>> pg_comments patch. With this v2 patch, \dd should properly show just
>> its five object types, and the psql documentation and help strings
>> should be fixed.
>
> I took a look at this patch today and I think some of these queries
> are not quite right.  When you do a left join against pg_description,
> you have this sort of thing in the WHERE clause:
>
> (d.objsubid IS NULL OR d.objsubid = 0)
>
> I think what you actually want is "AND d.objsubid = 0" in the LEFT
> JOIN's "ON" clause.  Then you are, in effect, only left joining
> against the rows from pg_description where objsubid = 0, and
> null-extending if none such is found.  I think that's what you want.

Thanks for taking a look at this.

Yeah, that is what I was going for. I believe my way worked, since
pg_description declares objsubid NOT NULL, so checking whether
d.objsubid IS NULL in the where clause should just match null-extended
rows. But anyway, your version is clearer, changed.

> I think you can remove the XXX comments, too.  Unless I'm
> misunderstanding something, using the table to test visibility for
> constraints, rules, and triggers seems just right, and opclasses and
> opfamilies you have a suitable function available, so those don't seem
> problematic.  Or am I confused?

I bet you're right; at least, it seems to work reasonably in the
testing I've done. I had just left those comments in there since they
were in all the sub-queries of objectDescription(), but now seems like
a good time to get rid of them.

Josh

On Tue, Jul 26, 2011 at 8:38 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> [new patch]

I've committed the portion of this that displays comments on languages
and casts.

For domains and conversions, I am wondering if we should display the
comments only when + is specified, since the output is fairly wide
already.

For foreign data wrappers, foreign servers, and foreign tables, I am
wondering if there is any particular rule we should adhere to in terms
of where the description shows up in the output column list. It
doesn't seem entirely consistent the way you've done it here, but
maybe you've put more thought into it than I have.

I haven't reviewed the part that changes the \dd output in detail yet (sorry).

Residual patch attached...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Aug 4, 2011 at 12:26 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jul 26, 2011 at 8:38 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>> [new patch]
>
> I've committed the portion of this that displays comments on languages
> and casts.

Thanks!

> For domains and conversions, I am wondering if we should display the
> comments only when + is specified, since the output is fairly wide
> already.

I wasn't sure whether there was some sort of precedent for whether
comments should be displayed only in verbose mode, but looking through
the existing backslash commands, it seems reasonable to make it
verbose-only if the output is already pushing 80 characters for
typical usage (object names and other column outputs of lengths
typically encountered).

A few existing backslash commands, such as \dn and maybe \db, don't
exactly follow this precedent. Not sure if we want to bother adjusting
the existing commands to be consistent in this regard. Defining
"typical usage" is pretty wishy-washy, so I'm not real inclined to try
messing with the existing commands.

> For foreign data wrappers, foreign servers, and foreign tables, I am
> wondering if there is any particular rule we should adhere to in terms
> of where the description shows up in the output column list.  It
> doesn't seem entirely consistent the way you've done it here, but
> maybe you've put more thought into it than I have.

Hrm, what wasn't consistent? I intended to just put the "Description"
column at the end of the outputs for \dew, \des, and \det, which seems
to be the way other commands handle this. Though now that I double
check, I notice that the verbose modes of these commands include extra
columns which come after "Description", and it might be better to
force "Description" to stay at the end in those cases, the way that
\dT[+] and \dFt[+] do. Though perhaps you're complaining about
something different -- \dL isn't forcing "Description" to the end in
verbose mode.

Josh

On Thu, Aug 4, 2011 at 10:24 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> A few existing backslash commands, such as \dn and maybe \db, don't
> exactly follow this precedent. Not sure if we want to bother adjusting
> the existing commands to be consistent in this regard. Defining
> "typical usage" is pretty wishy-washy, so I'm not real inclined to try
> messing with the existing commands.

Me neither.

>> For foreign data wrappers, foreign servers, and foreign tables, I am
>> wondering if there is any particular rule we should adhere to in terms
>> of where the description shows up in the output column list.  It
>> doesn't seem entirely consistent the way you've done it here, but
>> maybe you've put more thought into it than I have.
>
> Hrm, what wasn't consistent? I intended to just put the "Description"
> column at the end of the outputs for \dew, \des, and \det, which seems
> to be the way other commands handle this. Though now that I double
> check, I notice that the verbose modes of these commands include extra
> columns which come after "Description", and it might be better to
> force "Description" to stay at the end in those cases, the way that
> \dT[+] and \dFt[+] do. Though perhaps you're complaining about
> something different -- \dL isn't forcing "Description" to the end in
> verbose mode.

Oh, I see. I was right - you have thought about this more than I have. :-)

I guess my vote is to make the SQL/MED stuff show the description only
in verbose mode, and always at the end; and revise what we did with
\dL to put the description at the end even in verbose mode.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Aug 5, 2011 at 8:32 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I guess my vote is to make the SQL/MED stuff show the description only
> in verbose mode, and always at the end; and revise what we did with
> \dL to put the description at the end even in verbose mode.

Yeah, that sounds fine to me. I've revised the residual patch to do
so, as well as incorporated your earlier suggestion about having \dD
and \dc only display descriptions in verbose-mode.

I did more testing, and found and fixed some brokenness related to
d.objsubid I had introduced into the listConversions() query, as well
as improved the version checking in objectDescription(). Updated patch
attached.

Josh

On Fri, Aug 5, 2011 at 7:25 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> On Fri, Aug 5, 2011 at 8:32 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I guess my vote is to make the SQL/MED stuff show the description only
>> in verbose mode, and always at the end; and revise what we did with
>> \dL to put the description at the end even in verbose mode.
>
> Yeah, that sounds fine to me. I've revised the residual patch to do
> so, as well as incorporated your earlier suggestion about having \dD
> and \dc only display descriptions in verbose-mode.
>
> I did more testing, and found and fixed some brokenness related to
> d.objsubid I had introduced into the listConversions() query, as well
> as improved the version checking in objectDescription(). Updated patch
> attached.

OK, I've now committed most of this, with some additions to the
documentation. Remaining bits attached.

I am a bit confused as to why we have both \det and \dE. They seem
redundant. Shouldn't we rip one of those out? IMHO, \det should be
the one to go, as it could be useful to do, e.g. \dtvE, which isn't
going to work with the \det syntax.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Aug 8, 2011 at 4:34 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> OK, I've now committed most of this, with some additions to the
> documentation.  Remaining bits attached.

Looks good, thanks.

> I am a bit confused as to why we have both \det and \dE.  They seem
> redundant.  Shouldn't we rip one of those out?  IMHO, \det should be
> the one to go, as it could be useful to do, e.g. \dtvE, which isn't
> going to work with the \det syntax.

Yeah, I was wondering that myself. At first I thought maybe someone
added in one without being aware of the other, but it looks like both
\dE and \det got added in by commit
0d692a0dc9f0e532c67c577187fe5d7d323cb95b. They are using different
queries internally (pg_class vs. pg_foreign_table), but I doubt end
users would care about that. Or perhaps the author just wanted a
command name similar to \dew and \des.

+1 for getting rid of one of them; I don't really care which one gets
the axe. Though we should make sure to be able to show all possible
columns in whichever command we keep (i.e. add "Options" column to
\dE+ if we keep that one). BTW, I haven't bothered setting up
functioning foreign tables yet, but is "Size" always going to be 0
bytes in \dE+?

Josh

On Mon, Aug 8, 2011 at 6:01 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> (i.e. add "Options" column to \dE+ if we keep that one).

Oh nevermind, "Options" is displayed by \d+ foreign_table_name.

Josh

(2011/08/09 7:01), Josh Kupershmidt wrote:
>> I am a bit confused as to why we have both \det and \dE. They seem
>> redundant. Shouldn't we rip one of those out? IMHO, \det should be
>> the one to go, as it could be useful to do, e.g. \dtvE, which isn't
>> going to work with the \det syntax.
>
> Yeah, I was wondering that myself. At first I thought maybe someone
> added in one without being aware of the other, but it looks like both
> \dE and \det got added in by commit
> 0d692a0dc9f0e532c67c577187fe5d7d323cb95b. They are using different
> queries internally (pg_class vs. pg_foreign_table), but I doubt end
> users would care about that. Or perhaps the author just wanted a
> command name similar to \dew and \des.

I'm the author of that patch, sorry for confusion. May I explain the
background of implementing those command? :)

Basically, during implementing foreign table support, I tried to follow
the existing design.

I found two backslash command groups in psql, \de[wsu] and \d[tvsT],
which are relevant to foreign table. Former is a group for listing
relation (table, view, sequence and type), and they have common output
format. Users would use them (sometimes with combination like \dtv) to
list only specified type of relations. OTOH latter is a group for
listing SQL/MED objects (wrapper, server and user mapping), and commands
in that group have different output columns. Users would use them to
see detail of FDW options, maybe mainly FDW options.

They had been implemented in different ways then, and I kept
implementation similar to existing codes for each. But now, as you say,
they seem redundant and inconsistent each other.

> +1 for getting rid of one of them; I don't really care which one gets
> the axe. Though we should make sure to be able to show all possible
> columns in whichever command we keep (i.e. add "Options" column to
> \dE+ if we keep that one).

I'm not sure whether getting rid of one of them is better.
But maybe \dE is useless than \det, because former doesn't shows more
information than simple \d.

> BTW, I haven't bothered setting up
> functioning foreign tables yet, but is "Size" always going to be 0
> bytes in \dE+?

Yes, "Size" is total file size calculated with pg_table_size(), but
foreign table has no data file.

Regards,
--
Shigeru Hanada

2011/8/9 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> I'm the author of that patch, sorry for confusion.  May I explain the
> background of implementing those command? :)
>
> Basically, during implementing foreign table support, I tried to follow
> the existing design.
>
> I found two backslash command groups in psql, \de[wsu] and \d[tvsT],
> which are relevant to foreign table.  Former is a group for listing
> relation (table, view, sequence and type), and they have common output
> format.  Users would use them (sometimes with combination like \dtv) to
> list only specified type of relations.  OTOH latter is a group for
> listing SQL/MED objects (wrapper, server and user mapping), and commands
> in that group have different output columns.  Users would use them to
> see detail of FDW options, maybe mainly FDW options.
>
> They had been implemented in different ways then, and I kept
> implementation similar to existing codes for each.  But now, as you say,
> they seem redundant and inconsistent each other.
>
>> +1 for getting rid of one of them; I don't really care which one gets
>> the axe. Though we should make sure to be able to show all possible
>> columns in whichever command we keep (i.e. add "Options" column to
>> \dE+ if we keep that one).
>
> I'm not sure whether getting rid of one of them is better.
> But maybe \dE is useless than \det, because former doesn't shows more
> information than simple \d.

Looking at this again, I see that they each have certain advantages.
\det shows more information that is specific to foreign tables; but I
don't want to get rid of \dE because it's useful to be able to do
stuff like \dtE to see tables and foreign tables in a single listing.
So maybe we should just leave this well enough alone.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Aug 8, 2011 at 6:01 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> On Mon, Aug 8, 2011 at 4:34 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> OK, I've now committed most of this, with some additions to the
>> documentation.  Remaining bits attached.
>
> Looks good, thanks.

And now I've committed (nearly) all of what remains.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company