patch: Allow \dd to show constraint comments

On Tue, May 17, 2011 at 10:27 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

Hi all,

Attached is a simple patch addressing the TODO item "Allow \dd to show
constraint comments". If you have comments on various constraints
(column, foreign key, primary key, unique, exclusion), they should
show up via \dd now.

Some example SQL is attached to create two tables with a variety of
constraints and constraint comments. With the patch, \dd should then
produce something like this:

                           Object descriptions
 Schema |         Name         |   Object   |         Description
--------+----------------------+------------+------------------------------
 public | bar_c_excl           | constraint | exclusion constraint comment
 public | bar_pkey             | constraint | two column pkey comment
 public | bar_uname_check      | constraint | constraint for bar
 public | bar_uname_fkey       | constraint | fkey comment
 public | uname_check_not_null | constraint | not null comment
 public | uname_cons           | constraint | sanity check for uname
 public | uname_uniq_cons      | constraint | unique constraint comment
(7 rows)

whereas without the patch, you should see nothing.

At the risk of opening a can of worms, if we're going to fix \dd,
shouldn't we fix it completely, and include comments on ALL the object
types that can have them? IIRC it's missing a bunch, not just
constraints.

Another thought is that I wonder if it's really useful to have a
backslash commands that dumps out comments on many different object
types. In some cases, e.g. \db+, we include the description for the
object in the output of the backslash command that lists objects just
of that type, which seems like a better design. Of course we have no
backslash command for constraints anyway....

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

On Thu, May 19, 2011 at 10:26 AM, Robert Haas <robertmhaas@gmail.com> wrote:

At the risk of opening a can of worms, if we're going to fix \dd,
shouldn't we fix it completely, and include comments on ALL the object
types that can have them?  IIRC it's missing a bunch, not just
constraints.

You opened this can up, so I hope you like worms ;-) Let's break down
the objects that the COMMENT docs say one can comment on. [Disclaimer:
It's likely I've made some mistakes in this list, data was gleaned
mostly from perusing describe.c]

1.) Comments displayed by \dd:

TABLE
AGGREGATE
OPERATOR
RULE
FUNCTION
INDEX
SEQUENCE
TRIGGER
TYPE
VIEW

2.) Comments displayed in the backslash commands for the object:

AGGREGATE \da
COLUMN \d+ tablename
COLLATION \dO
DATABASE \l+
EXTENSION \dx
FUNCTION \df+
FOREIGN TABLE \d+ tablename (I think?)
LARGE OBJECT \dl
ROLE \dg+
SCHEMA \dn+
TABLESPACE \db+
TYPE \dT
TEXT SEARCH CONFIGURATION \dF
TEXT SEARCH DICTIONARY \dFd
TEXT SEARCH PARSER \dFp
TEXT SEARCH TEMPLATE \dFt

3.) Objects for which we don't display the comments anywhere:
CAST
CONSTRAINT (addressed by this patch)
CONVERSION
DOMAIN
PROCEDURAL LANGUAGE

4.) My eyes are starting to glaze over, and I'm too lazy to figure out
how or if comments for these objects are handled:
FOREIGN DATA WRAPPER
OPERATOR CLASS
OPERATOR FAMILY
SERVER

Another thought is that I wonder if it's really useful to have a
backslash commands that dumps out comments on many different object
types.

ISTM that \dd is best suited as a command to show the comments for
objects for which we don't have an individual backslash command, or
objects for which it's not practical to show the comment in the
backslash command.

In some cases, e.g. \db+, we include the description for the
object in the output of the backslash command that lists objects just
of that type, which seems like a better design.

I agree that's the way to go for objects where such display is
feasible (the backslash command produces columnar output, and we can
just stick in a "comment" column).

I wouldn't want to try to pollute, say, \d+ with comments about
tables, rules, triggers, etc. But for the few objects displayed by
both \dd and the object's respective backslash command (aggregates,
types, and functions), I would be in favor of ripping out the \dd
display.

Of course we have no
backslash command for constraints anyway....

Precisely, and I think there's a solid argument for putting
constraints into bucket 1 above, as this patch does, since there's no
good room to display constraint comments inside \d+, and there's no
backslash command specifically for constraints.

I was kind of hoping to avoid dealing with this can of worms with this
simple patch, which by itself seems uncontroversial. If there's
consensus that \dd and the other backslash commands need further
reworking, I can probably devote a little more time to this. But let's
not have the perfect be the enemy of the good.

Josh

On Thu, May 19, 2011 at 10:36 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

Precisely, and I think there's a solid argument for putting
constraints into bucket 1 above, as this patch does, since there's no
good room to display constraint comments inside \d+, and there's no
backslash command specifically for constraints.

I was kind of hoping to avoid dealing with this can of worms with this
simple patch, which by itself seems uncontroversial. If there's
consensus that \dd and the other backslash commands need further
reworking, I can probably devote a little more time to this. But let's
not have the perfect be the enemy of the good.

Frankly, I think \dd is a horrid kludge which has about as much chance
of being useful as a fireproof candle. I don't really object to the
patch at hand: it'll probably solve your problem, or you wouldn't have
bothered writing the patch. At the same time, I can't shake the
feeling that it solves your problem mostly by accident. Clearly, you
have more than no comments on constraints (or you wouldn't care) and
you must also have few enough constraints on the types of objects
which \dd has randomly decided to care to make it feasible to look at
one big list and pick out the information you're interested in. It's
hard to work up a lot of enthusiasm for that being a common situation,
even though, as you say, this certainly isn't making anything any
worse.

I continue to think that the right fix for this problem is the one I
proposed here:

http://archives.postgresql.org/pgsql-hackers/2010-09/msg01080.php

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

On Sun, May 22, 2011 at 11:33 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, May 19, 2011 at 10:36 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

Precisely, and I think there's a solid argument for putting
constraints into bucket 1 above, as this patch does, since there's no
good room to display constraint comments inside \d+, and there's no
backslash command specifically for constraints.

I was kind of hoping to avoid dealing with this can of worms with this
simple patch, which by itself seems uncontroversial. If there's
consensus that \dd and the other backslash commands need further
reworking, I can probably devote a little more time to this. But let's
not have the perfect be the enemy of the good.

Frankly, I think \dd is a horrid kludge which has about as much chance
of being useful as a fireproof candle.  I don't really object to the
patch at hand: it'll probably solve your problem, or you wouldn't have
bothered writing the patch.  At the same time, I can't shake the
feeling that it solves your problem mostly by accident.  Clearly, you
have more than no comments on constraints (or you wouldn't care) and
you must also have few enough constraints on the types of objects
which \dd has randomly decided to care to make it feasible to look at
one big list and pick out the information you're interested in.

Well actually, I got into messing with this solely from the Todo list.
Which, of course, neglected to mention the thread about pg_comments,
or the other objects missing from \dd.

It's
hard to work up a lot of enthusiasm for that being a common situation,
even though, as you say, this certainly isn't making anything any
worse.

I continue to think that the right fix for this problem is the one I
proposed here:

http://archives.postgresql.org/pgsql-hackers/2010-09/msg01080.php

Yeah, I'm on board with you about the utility of a pg_comments system
view. I don't buy Tom's quick dismissal of the idea; to recap, he
complained that:

| Unless you propose to break psql's hard-won backwards compatibility,
| this isn't going to accomplish anything towards making describe.c
| simpler or shorter.

Well, the real problem here, as I see it, is:
a.) We are schizophrenic about which comments are displayed by \dd
and which are displayed by other backslash commands. And some comments
aren't yet displayed anywhere, making the COMMENT ON syntax for them
basically useless (what good is a comment no one can see without
digging around in the system catalogs by hand..)
b.) One can comment on something like 32 different types of objects;
so if we actually fixed all the holes in \dd, it could be a real
nuisance trying to grep through its output to find the comments for
the objects you're actually interested in. Which leads to the
desirability of having a system view you could construct ad-hoc
queries against.

If we were to introduce pg_comments in 9.2, I would ideally want us to
fix up \dd to work better against older server versions (i.e. the
original patch, plus some more work) as well, so the complaint about
backwards compatibility shouldn't be a concern.

And Tom complained:

| Also, it seems to me that what you've mostly done
| is to move complexity from describe.c (where the query can be fixed
| easily if it's found to be broken) to system_views.sql (where it cannot
| be changed without an initdb).

Well, the complexity in describe.c doesn't bother me too badly, as
long as commands do what they're supposed to. The real reason to move
the logic into a system view, IMO, would be for ad-hoc queries, with
utility to tools other than psql a secondary win.

And if we followed Tom's logic about system views being bad ipso
facto, we should want to rip out all non-critical system views (no, I
don't want this). I would agree that if we want to create pg_comments
we should make sure that, at the least, it displays comments for all
object types from the get-go, given Tom's valid warning about the
impossibility of upgrading a system view within a minor version.

Your pg_comments.patch doesn't apply to git head anymore -- would you
be interested in resurrecting this code for 9.2, assuming we can get
support for this idea?

Josh

On Mon, May 23, 2011 at 10:13 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

Well actually, I got into messing with this solely from the Todo list.
Which, of course, neglected to mention the thread about pg_comments,
or the other objects missing from \dd.

Heh. Sounds like updating the Todo list would be a good place to start.

Well, the real problem here, as I see it, is:
 a.) We are schizophrenic about which comments are displayed by \dd
and which are displayed by other backslash commands. And some comments
aren't yet displayed anywhere, making the COMMENT ON syntax for them
basically useless (what good is a comment no one can see without
digging around in the system catalogs by hand..)
 b.) One can comment on something like 32 different types of objects;
so if we actually fixed all the holes in \dd, it could be a real
nuisance trying to grep through its output to find the comments for
the objects you're actually interested in. Which leads to the
desirability of having a system view you could construct ad-hoc
queries against.

+1.

If we were to introduce pg_comments in 9.2, I would ideally want us to
fix up \dd  to work better against older server versions (i.e. the
original patch, plus some more work) as well, so the complaint about
backwards compatibility shouldn't be a concern.

I'd be OK with someone working on that, but can't get riled up about
it myself. We have stuff that we fix in every release that doesn't
work in older releases, and psql-9.2 compatibility with backend<=9.1
for a backslash command that's barely been updated this millenium is
not likely to rise to the top of my list of things to worry about.

And if we followed Tom's logic about system views being bad ipso
facto, we should want to rip out all non-critical system views (no, I
don't want this). I would agree that if we want to create pg_comments
we should make sure that, at the least, it displays comments for all
object types from the get-go, given Tom's valid warning about the
impossibility of upgrading a system view within a minor version.

Darn straight. Sounds like a job for the regression tests.

Your pg_comments.patch doesn't apply to git head anymore -- would you
be interested in resurrecting this code for 9.2, assuming we can get
support for this idea?

Yeah, I don't think it would be too hard to rebase; or you or someone
else might even want to pick it up. :-)

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

On Mon, May 23, 2011 at 10:56 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, May 23, 2011 at 10:13 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

Well actually, I got into messing with this solely from the Todo list.
Which, of course, neglected to mention the thread about pg_comments,
or the other objects missing from \dd.

Heh.  Sounds like updating the Todo list would be a good place to start.

Yeah, fixed that, at least.

[snip]

Your pg_comments.patch doesn't apply to git head anymore -- would you
be interested in resurrecting this code for 9.2, assuming we can get
support for this idea?

Yeah, I don't think it would be too hard to rebase; or you or someone
else might even want to pick it up.   :-)

Attached is a rebased patch. From a quick look, it seems that most of
the object types missing from \dd are already covered by pg_comments
(cast, constraint, conversion, domain, language, operator class,
operator family). A few objects would probably still need to be added
(foreign data wrapper, server).

I'm not sure how much time I'll have in the next CF, so I'd rather not
take up this patch. But I should be able to at least review.

Besides the few concerns and missing bits noted by Robert in the
original thread[1]http://archives.postgresql.org/pgsql-hackers/2010-09/msg01080.php, one concern I have is how easy it will be for
users to directly query pg_comments for common types of queries, e.g.
looking for comments attached to non-system functions, given the few
thousand rows in that view. I wonder whether it'd be worthwhile to
have two views: pg_user_comments and pg_all_comments, similar to how
we have pg_stat_user_tables and pg_stat_all_tables. We could just say
that folks should just use \dd for looking at non-system objects, but
a significant reason for this whole exercise is that \dd isn't cutting
it.

Josh

--
[1]: http://archives.postgresql.org/pgsql-hackers/2010-09/msg01080.php

On Tue, May 24, 2011 at 10:31 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

Attached is a rebased patch. From a quick look, it seems that most of
the object types missing from \dd are already covered by pg_comments
(cast, constraint, conversion, domain, language, operator class,
operator family). A few objects would probably still need to be added
(foreign data wrapper, server).

Here's another update to this patch. Includes:
* rudimentary doc page for pg_comments
* 'foreign data wrapper' and 'server' comment types now included in
pg_comments; regression test updated

Still TODO:
* psql's \dd should read from pg_comments. But I think we need some
simple way to distinguish comments on system objects from non-system
objects, which we'd need for differentiating \dd from \ddS, not to
mention being useful for ad-hoc queries. I'm open to ideas here.

Josh

Excerpts from Josh Kupershmidt's message of dom jun 05 16:36:57 -0400 2011:

On Tue, May 24, 2011 at 10:31 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

Attached is a rebased patch. From a quick look, it seems that most of
the object types missing from \dd are already covered by pg_comments
(cast, constraint, conversion, domain, language, operator class,
operator family). A few objects would probably still need to be added
(foreign data wrapper, server).

Here's another update to this patch. Includes:
* rudimentary doc page for pg_comments
* 'foreign data wrapper' and 'server' comment types now included in
pg_comments; regression test updated

Hmm, if we're going to have pg_comments as a syntactic sugar kind of
thing, it should output things in format immediately useful to the user,
i.e. relation/column/etc names and not OIDs. The OIDs would force you
to do lots of joins just to make it readable. Maybe you should have a
column for the class of object the comment applies to, but as a name and
not a regclass. And then a column for names that each comment applies
to. (We're still struggling to get a useful pg_locks display). I mean,
if OIDs are good for you and you're OK with doing a few joins, why not
go to the underlying catalogs in the first place?

(IMHO anyway -- what do others think?)

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Mon, Jun 6, 2011 at 1:03 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Excerpts from Josh Kupershmidt's message of dom jun 05 16:36:57 -0400 2011:

On Tue, May 24, 2011 at 10:31 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

Attached is a rebased patch. From a quick look, it seems that most of
the object types missing from \dd are already covered by pg_comments
(cast, constraint, conversion, domain, language, operator class,
operator family). A few objects would probably still need to be added
(foreign data wrapper, server).

Here's another update to this patch. Includes:
 * rudimentary doc page for pg_comments
 * 'foreign data wrapper' and 'server' comment types now included in
pg_comments; regression test updated

Hmm, if we're going to have pg_comments as a syntactic sugar kind of
thing, it should output things in format immediately useful to the user,
i.e. relation/column/etc names and not OIDs.  The OIDs would force you
to do lots of joins just to make it readable.

Well, that's basically what this is doing. See the objname/objtype
columns. It's intended that the output of this view should match the
format that COMMENT takes as input. But propagating the OIDs through
is sensible as well, because sometimes people may want to do other
joins, filtering, etc.

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

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Jun 6, 2011 at 1:03 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Hmm, if we're going to have pg_comments as a syntactic sugar kind of
thing, it should output things in format immediately useful to the user,
i.e. relation/column/etc names and not OIDs. �The OIDs would force you
to do lots of joins just to make it readable.

Well, that's basically what this is doing. See the objname/objtype
columns. It's intended that the output of this view should match the
format that COMMENT takes as input. But propagating the OIDs through
is sensible as well, because sometimes people may want to do other
joins, filtering, etc.

Is it also propagating the catalog OID through? Because joining on OID
alone is not to be trusted.

I tend to agree with Alvaro's viewpoint here: anybody who wants to deal
directly in OIDs is better off joining directly to pg_description, and
not going through the rather large overhead that this view is going to
impose. So we should just make this a purely user-friendly view and be
done with it, not try to create an amalgam that serves neither purpose
well.

regards, tom lane

On Mon, Jun 6, 2011 at 2:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Jun 6, 2011 at 1:03 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Hmm, if we're going to have pg_comments as a syntactic sugar kind of
thing, it should output things in format immediately useful to the user,
i.e. relation/column/etc names and not OIDs.  The OIDs would force you
to do lots of joins just to make it readable.

Well, that's basically what this is doing.  See the objname/objtype
columns.  It's intended that the output of this view should match the
format that COMMENT takes as input.  But propagating the OIDs through
is sensible as well, because sometimes people may want to do other
joins, filtering, etc.

Is it also propagating the catalog OID through?  Because joining on OID
alone is not to be trusted.

Yep.

I tend to agree with Alvaro's viewpoint here: anybody who wants to deal
directly in OIDs is better off joining directly to pg_description, and
not going through the rather large overhead that this view is going to
impose.  So we should just make this a purely user-friendly view and be
done with it, not try to create an amalgam that serves neither purpose
well.

Possibly. On the other hand we have things like pg_tables floating
around that are basically useless because you can't get the OID
easily. The information_schema suffers from this problem as well. I
get what you're saying, but I think we should think two or three times
very carefully before throwing away the OID in a situation where it
can easily be provided.

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

I checked the v4 patch.

At first, I noticed three missing object classes although COMMENT ON allows to
set a description on 'collation', 'extension' and 'foreign table'.
In addition, this pg_comments system view supports 'access method' class, but
we cannot set a comment on access methods using COMMENT ON statement.

Regarding to the data-type of objnamespace, how about an idea to define a new
data type such as 'regschema' and cast objnamespace into this type?
If we have such data type, user can reference string expression of schema name,
and also available to use OID joins.

Thanks,

2011/6/5 Josh Kupershmidt <schmiddy@gmail.com>:

On Tue, May 24, 2011 at 10:31 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

Attached is a rebased patch. From a quick look, it seems that most of
the object types missing from \dd are already covered by pg_comments
(cast, constraint, conversion, domain, language, operator class,
operator family). A few objects would probably still need to be added
(foreign data wrapper, server).

Here's another update to this patch. Includes:
 * rudimentary doc page for pg_comments
 * 'foreign data wrapper' and 'server' comment types now included in
pg_comments; regression test updated

Still TODO:
 * psql's \dd should read from pg_comments. But I think we need some
simple way to distinguish comments on system objects from non-system
objects, which we'd need for differentiating \dd from \ddS, not to
mention being useful for ad-hoc queries. I'm open to ideas here.

Josh

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Sat, Jun 18, 2011 at 10:53 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

At first, I noticed three missing object classes although COMMENT ON allows to
set a description on 'collation', 'extension' and 'foreign table'.

Good catch. Attached patch adds these types in.

In addition, this pg_comments system view supports 'access method' class, but
we cannot set a comment on access methods using COMMENT ON statement.

Well, there are comments for the built-in access methods, i.e.

test=# select * from pg_comments WHERE objtype = 'access method';
objoid | classoid | objsubid | objtype | objnamespace | objname
| description
--------+----------+----------+---------------+--------------+---------+----------------------------
403 | 2601 | 0 | access method | | btree
| b-tree index access method
405 | 2601 | 0 | access method | | hash
| hash index access method
783 | 2601 | 0 | access method | | gist
| GiST index access method
2742 | 2601 | 0 | access method | | gin
| GIN index access method
(4 rows)

So I think it's useful to leave objtype = 'access method' in
pg_comments. I guess the assumption is that the only time you would
need to tweak a comment on an access method is if you're building your
own, and if so you can enter the comment into the catalogs manually.

Regarding to the data-type of objnamespace, how about an idea to define a new
data type such as 'regschema' and cast objnamespace into this type?
If we have such data type, user can reference string expression of schema name,
and also available to use OID joins.

Are you suggesting we leave the structure of pg_comments unchanged,
but introduce a new 'regschema' type so that if users want to easily
display the schema name of an object, they can just do:

SELECT objnamespace::regschema, ...
FROM pg_comments WHERE ... ;

That seems reasonable to me.

Josh

On Sat, Jun 18, 2011 at 1:43 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

Regarding to the data-type of objnamespace, how about an idea to define a new
data type such as 'regschema' and cast objnamespace into this type?
If we have such data type, user can reference string expression of schema name,
and also available to use OID joins.

Are you suggesting we leave the structure of pg_comments unchanged,
but introduce a new 'regschema' type so that if users want to easily
display the schema name of an object, they can just do:

 SELECT objnamespace::regschema, ...
   FROM pg_comments WHERE ... ;

That seems reasonable to me.

In the past I think the feeling has been that reg* types are primarily
useful for objects with schema-qualified names. Translating a table
OID into a table name is actually sort of non-trivial, at least if you
want to schema-qualify its name when and only when necessary. The use
case seems quite a bit weaker for schemas, since you can easily pull
the right value from pg_namespace with a subquery if you are so
inclined.

It is perhaps worth noting that the patch now under discussion on this
thread does something quite a lot different than what the subject
would suggest.

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

I think the v5 patch should be marked as 'Ready for Committer'

2011/6/18 Josh Kupershmidt <schmiddy@gmail.com>:

On Sat, Jun 18, 2011 at 10:53 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

In addition, this pg_comments system view supports 'access method' class, but
we cannot set a comment on access methods using COMMENT ON statement.

Well, there are comments for the built-in access methods, i.e.

Oops, I missed the comments on initdb stage.

Regarding to the data-type of objnamespace, how about an idea to define a new
data type such as 'regschema' and cast objnamespace into this type?
If we have such data type, user can reference string expression of schema name,
and also available to use OID joins.

Are you suggesting we leave the structure of pg_comments unchanged,
but introduce a new 'regschema' type so that if users want to easily
display the schema name of an object, they can just do:

 SELECT objnamespace::regschema, ...
   FROM pg_comments WHERE ... ;

That seems reasonable to me.

Yes, however, it should be discussed in another thread as Robert said.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Sun, Jun 19, 2011 at 3:25 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

2011/6/18 Josh Kupershmidt <schmiddy@gmail.com>:
I think the v5 patch should be marked as 'Ready for Committer'

I think we still need to handle my "Still TODO" concerns noted
upthread. I don't have a lot of time this weekend due to a family
event, but I was mulling over putting in a "is_system" boolean column
into pg_comments and then fixing psql's \dd[S] to use pg_comments. But
I am of course open to other suggestions.

Josh

On Thu, May 19, 2011 at 9:36 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

On Thu, May 19, 2011 at 10:26 AM, Robert Haas <robertmhaas@gmail.com> wrote:

At the risk of opening a can of worms, if we're going to fix \dd,
shouldn't we fix it completely, and include comments on ALL the object
types that can have them?  IIRC it's missing a bunch, not just
constraints.

You opened this can up, so I hope you like worms ;-) Let's break down
the objects that the COMMENT docs say one can comment on. [Disclaimer:
It's likely I've made some mistakes in this list, data was gleaned
mostly from perusing describe.c]

1.) Comments displayed by \dd:

 TABLE
 AGGREGATE
 OPERATOR
 RULE
 FUNCTION
 INDEX
 SEQUENCE
 TRIGGER
 TYPE
 VIEW

2.) Comments displayed in the backslash commands for the object:

 AGGREGATE                     \da
 COLUMN                        \d+ tablename
 COLLATION                     \dO
 DATABASE                      \l+
 EXTENSION                     \dx
 FUNCTION                      \df+
 FOREIGN TABLE                 \d+ tablename (I think?)
 LARGE OBJECT                  \dl
 ROLE                          \dg+
 SCHEMA                        \dn+
 TABLESPACE                    \db+
 TYPE                          \dT
 TEXT SEARCH CONFIGURATION     \dF
 TEXT SEARCH DICTIONARY        \dFd
 TEXT SEARCH PARSER            \dFp
 TEXT SEARCH TEMPLATE          \dFt

3.) Objects for which we don't display the comments anywhere:
 CAST
 CONSTRAINT (addressed by this patch)
 CONVERSION
 DOMAIN
 PROCEDURAL LANGUAGE

4.) My eyes are starting to glaze over, and I'm too lazy to figure out
how or if comments for these objects are handled:
 FOREIGN DATA WRAPPER
 OPERATOR CLASS
 OPERATOR FAMILY
 SERVER

Another thought is that I wonder if it's really useful to have a
backslash commands that dumps out comments on many different object
types.

ISTM that \dd is best suited as a command to show the comments for
objects for which we don't have an individual backslash command, or
objects for which it's not practical to show the comment in the
backslash command.

In some cases, e.g. \db+, we include the description for the
object in the output of the backslash command that lists objects just
of that type, which seems like a better design.

I agree that's the way to go for objects where such display is
feasible (the backslash command produces columnar output, and we can
just stick in a "comment" column).

I wouldn't want to try to pollute, say, \d+ with comments about
tables, rules, triggers, etc. But for the few objects displayed by
both \dd and the object's respective backslash command (aggregates,
types, and functions), I would be in favor of ripping out the \dd
display.

Of course we have no
backslash command for constraints anyway....

Precisely, and I think there's a solid argument for putting
constraints into bucket 1 above, as this patch does, since there's no
good room to display constraint comments inside \d+, and there's no
backslash command specifically for constraints.

I was kind of hoping to avoid dealing with this can of worms with this
simple patch, which by itself seems uncontroversial. If there's
consensus that \dd and the other backslash commands need further
reworking, I can probably devote a little more time to this. But let's
not have the perfect be the enemy of the good.

Patch applies clean, does what it is supposed to do, and matches other
conventions in describe.c Passing to committer. pg_comments may be
a better way to go, but that is a problem for another day...

merlin

On Sun, Jun 19, 2011 at 11:59 AM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

I think we still need to handle my "Still TODO" concerns noted
upthread. I don't have a lot of time this weekend due to a family
event, but I was mulling over putting in a "is_system" boolean column
into pg_comments and then fixing psql's \dd[S] to use pg_comments. But
I am of course open to other suggestions.

I went ahead and did it this way, though I wasn't 100% sure about some
of the guesses for the "is_system" column I made. I assumed the
following types should have is_system = true: casts, roles, databases,
access methods, tablespaces. I assumed is_system = false for large
objects. For the rest, I just used whether the schema name of the
object was 'pg_catalog' or 'information_schema'.

With the is_system column in place, I was able to make psql's \dd
actually use pg_comments.

I had to tweak the pg_proc.h entry for pg_opfamily_is_visible to play
nice with the recent "transform function" change to that file.

Issues still to be resolved:

1.) For now, I'm just ignoring the issue of visibility checks; I
didn't see a simple way to support these checks \dd was doing:

processSQLNamePattern(pset.db, &buf, pattern, true, false,
"n.nspname", "p.proname", NULL,
"pg_catalog.pg_function_is_visible(p.oid)");

I'm a bit leery of adding an "is_visible" column into pg_comments, but
I'm not sure I have a feasible workaround if we do want to keep this
visibility check. Maybe a big CASE statement for the last argument of
processSQLNamePattern() would work...

2.) Since we're mucking with \dd, I think now might be a good time to
re-examine what types of objects are displayed by \dd. I suggest we
explicitly advertise \dd as being only for objects which have comments
_not_ displayed by the object's individual backslash command.
Currently, the comment for objectDescription() claims

* Note: This only lists things that actually have a description. For
complete
* lists of things, there are other \d? commands.

but this claim is bogus; from the object type breakdown in my second
post on this thread, the de facto purpose of \dd is really to show
comments not displayed elsewhere.

I'd like to adjust what object types should have comments displayed in
their respective backslash commands instead. I went ahead and removed
the "aggregate" type from \dd, since those comments are already
displayed by \da, but I'd like to tweak things a bit further.

For instance, \dL, \dew, and \des could be improved to display
comments for LANGUAGE, FOREIGN DATA WRAPPER, and FOREIGN DATA SERVER
respectively. Etc.

3.) I haven't tried to fix up the indentation from my whacking around
in describe.c yet. The view definition in system_views.sql might need
to be re-formatted as well to match its surroundings.

An updated patch is attached; I've marked it WIP since there are the
listed issues outstanding. I'd appreciated feedback particularly on
the way forward for 1.) and 2.), and whether my guesses for the
"is_system" column are OK.

Josh

On Wed, Jun 29, 2011 at 12:15 PM, Merlin Moncure <mmoncure@gmail.com> wrote:

Patch applies clean, does what it is supposed to do, and matches other
conventions in describe.c  Passing to committer.   pg_comments may be
a better way to go, but that is a problem for another day...

Thanks for the review, and sorry for my tardiness in getting back into
this thread. Since the describe.c patch is already written and
reviewed, I agree it's worthwhile to commit, even though the
pg_comments patch aims to stomp on this approach. (pg_comments might
need some further baking time..)

Josh