Re: patch: Allow \dd to show constraint comments

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.

Josh

On Tue, May 17, 2011 at 10:27 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Thu, May 19, 2011 at 10:36 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Mon, May 23, 2011 at 10:13 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)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], 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(at)gmail(dot)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(at)gmail(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)gmail(dot)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(at)gmail(dot)com> writes:
> On Mon, Jun 6, 2011 at 1:03 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Jun 6, 2011 at 1:03 PM, Alvaro Herrera
>> <alvherre(at)commandprompt(dot)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(at)gmail(dot)com>:
> On Tue, May 24, 2011 at 10:31 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sat, Jun 18, 2011 at 10:53 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)gmail(dot)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(at)gmail(dot)com>:
> On Sat, Jun 18, 2011 at 10:53 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Sun, Jun 19, 2011 at 3:25 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2011/6/18 Josh Kupershmidt <schmiddy(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Thu, May 19, 2011 at 10:26 AM, Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)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

On Wed, Jun 29, 2011 at 12:15 PM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>> 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...

I am inclined to say that we should reject this patch as it stands.
With or without pg_comments, I think we need a plan for \dd, and
adding one object type is not a plan. The closest thing I've seen to
a plan is this comment from Josh:

--
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.
--

If someone wants to implement that, I'm OK with it, though I think we
should also consider the alternative of abolishing \dd and just always
display the comments via the per-object type commands (e.g. \d+ would
display the table, constraint, trigger, and rule comments). I don't
want to, as Josh says, let the perfect be the enemy of the good, but
if we change this as proposed we're just going to end up changing it
again. That's going to be more work than just doing it once, and if
it happens over the course of multiple releases, then it creates more
annoyance for our users, too. I don't really think this is such a
large project that we can't get it right in one try.

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

On Thu, Jul 7, 2011 at 10:00 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

[review of original, small patch to add another type to \dd's output]

> I am inclined to say that we should reject this patch as it stands.

That's totally OK - that original patch was of marginal use given the
larger brokenness of \dd.

> With or without pg_comments, I think we need a plan for \dd, and
> adding one object type is not a plan.  The closest thing I've seen to
> a plan is this comment from Josh:
>
> --
> 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.
> --
>
> If someone wants to implement that, I'm OK with it, though I think we
> should also consider the alternative of abolishing \dd and just always
> display the comments via the per-object type commands (e.g. \d+ would
> display the table, constraint, trigger, and rule comments).

And I'm quite happy you said that: I'm just about to post part 1 of a
patch to start fixing up the individual backslash commands which
clearly should be showing comments, but aren't. I'm thinking that
clarifying which backslash commands should show object comments, and
which types must be left for \dd to clean up can be handled separately
from pg_comments. And my preference is to whittle down \dd to as
little as necessary (if it could be gotten rid of entirely, even
better, but I doubt that's going to fly).

> I don't
> want to, as Josh says, let the perfect be the enemy of the good, but
> if we change this as proposed we're just going to end up changing it
> again.  That's going to be more work than just doing it once, and if
> it happens over the course of multiple releases, then it creates more
> annoyance for our users, too.  I don't really think this is such a
> large project that we can't get it right in one try.

Fair enough. If the pg_comments patch does go down in flames, I can
circle back and patch up the rest of the holes in \dd.

Josh

On Thu, Jul 7, 2011 at 9:00 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jun 29, 2011 at 12:15 PM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>>> 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...
>
> I am inclined to say that we should reject this patch as it stands.
> With or without pg_comments, I think we need a plan for \dd, and
> adding one object type is not a plan.  The closest thing I've seen to
> a plan is this comment from Josh:
>
> --
> 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.
> --
>
> If someone wants to implement that, I'm OK with it, though I think we
> should also consider the alternative of abolishing \dd and just always
> display the comments via the per-object type commands (e.g. \d+ would
> display the table, constraint, trigger, and rule comments).  I don't
> want to, as Josh says, let the perfect be the enemy of the good, but
> if we change this as proposed we're just going to end up changing it
> again.  That's going to be more work than just doing it once, and if
> it happens over the course of multiple releases, then it creates more
> annoyance for our users, too.  I don't really think this is such a
> large project that we can't get it right in one try.

no argument.

merlin

Josh,

> Fair enough. If the pg_comments patch does go down in flames, I can
> circle back and patch up the rest of the holes in \dd.

I am unable to figure out the status of the pg_comments patch from this
thread. What's going on with it?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Jul 15, 2011, at 3:48 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> Josh,
>
>> Fair enough. If the pg_comments patch does go down in flames, I can
>> circle back and patch up the rest of the holes in \dd.
>
> I am unable to figure out the status of the pg_comments patch from this
> thread. What's going on with it?

I'll review it in the next few days.

...Robert

On Sat, Jul 2, 2011 at 8:37 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> On Sun, Jun 19, 2011 at 11:59 AM, Josh Kupershmidt <schmiddy(at)gmail(dot)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'.

It seems funny to have is_system = true unconditionally for any object
type. Why'd you do it that way? Or maybe I should ask, why true
rather than false?

> 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.

It seems like we ought to have this function for symmetry regardless
of what happens to the rest of this, so I extracted and committed this
part.

> 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...

Yeah... or we could add a function pg_object_is_visible(classoid,
objectoid) that would dispatch to the relevant visibility testing
function based on object type. Not sure if that's too much of a
kludge, but it wouldn't be useful only here: you could probably use it
in combination with pg_locks, for example.

The real problem with the is_system column is that it seems to be
entirely targeted around what psql happens to feel like doing. I'm
pretty sure we'll regret it if we choose to go that route.

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

On Sun, Jul 17, 2011 at 11:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Jul 2, 2011 at 8:37 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>> On Sun, Jun 19, 2011 at 11:59 AM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> It seems funny to have is_system = true unconditionally for any object
> type.  Why'd you do it that way?  Or maybe I should ask, why true
> rather than false?

Thanks for the review!

Well, I was hoping to go by the existing psql backslash commands'
notions about what qualifies as "system" and what doesn't; that worked
OK for commands which supported the 'S' modifier, but not all do. For
objects like tablespaces, where you must be a superuser to create one,
it seems like they should all be considered is_system = true, on the
vague premise that superuser => system. Other objects like roles are
admittedly murkier, and I didn't find any precedent inside describe.c
to help make such distinctions.

But this is probably all a moot point, see below.

>> 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.
>
> It seems like we ought to have this function for symmetry regardless
> of what happens to the rest of this, so I extracted and committed this
> part.

Good idea.

>> 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...
>
> Yeah... or we could add a function pg_object_is_visible(classoid,
> objectoid) that would dispatch to the relevant visibility testing
> function based on object type.  Not sure if that's too much of a
> kludge, but it wouldn't be useful only here: you could probably use it
> in combination with pg_locks, for example.

Something like that might work, though an easy escape is just leaving
the query style of \dd as it was originally (i.e. querying the various
catalogs directly, and not using pg_comments): discussed a bit in the
recent pg_comments thread w/ Josh Berkus.

> The real problem with the is_system column is that it seems to be
> entirely targeted around what psql happens to feel like doing.  I'm
> pretty sure we'll regret it if we choose to go that route.

Yeah, it did turn out more messy than I had hoped, and I'm not sure
it'd be possible to iron out the semantics of is_system in a way that
leaves everyone happy. Probably best to just rip it out if \dd won't
need it.

I'll try to send an updated patch by this weekend.

Josh

On Mon, Jul 18, 2011 at 10:57 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> Well, I was hoping to go by the existing psql backslash commands'
> notions about what qualifies as "system" and what doesn't; that worked
> OK for commands which supported the 'S' modifier, but not all do. For
> objects like tablespaces, where you must be a superuser to create one,
> it seems like they should all be considered is_system = true, on the
> vague premise that superuser => system. Other objects like roles are
> admittedly murkier, and I didn't find any precedent inside describe.c
> to help make such distinctions.

I think that the idea is that system objects are the ones that the
user probably doesn't care to see most of the time - i.e. the ones
that are "built in". But...

> But this is probably all a moot point, see below.

...yes.

>>> 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...
>>
>> Yeah... or we could add a function pg_object_is_visible(classoid,
>> objectoid) that would dispatch to the relevant visibility testing
>> function based on object type.  Not sure if that's too much of a
>> kludge, but it wouldn't be useful only here: you could probably use it
>> in combination with pg_locks, for example.
>
> Something like that might work, though an easy escape is just leaving
> the query style of \dd as it was originally (i.e. querying the various
> catalogs directly, and not using pg_comments): discussed a bit in the
> recent pg_comments thread w/ Josh Berkus.

That's another approach. It seems a bit lame, but...

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

On Mon, Jul 18, 2011 at 11:15 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jul 18, 2011 at 10:57 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>>>> 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...
>>>
>>> Yeah... or we could add a function pg_object_is_visible(classoid,
>>> objectoid) that would dispatch to the relevant visibility testing
>>> function based on object type.  Not sure if that's too much of a
>>> kludge, but it wouldn't be useful only here: you could probably use it
>>> in combination with pg_locks, for example.
>>
>> Something like that might work, though an easy escape is just leaving
>> the query style of \dd as it was originally (i.e. querying the various
>> catalogs directly, and not using pg_comments): discussed a bit in the
>> recent pg_comments thread w/ Josh Berkus.
>
> That's another approach.  It seems a bit lame, but...

I went ahead and patched up \dd to display its five object types with
its old-style rooting around in catalogs. I played around again with
the idea of having \dd query pg_comments, but gave up when I realized:

1. We might not be saving much complexity in \dd's query
2. Taking the is_system column out was probably good for pg_comments,
but exacerbates point 1.), not to mention the visibility testing that
would have to be done somehow.
3. The "objname" column of pg_comments is intentionally different
than the "Name" column displayed by \dd; the justification for this
was that \dd's "Name" display wasn't actually useful to recreate the
call to COMMENT ON, but this difference in pg_comments would make it
pretty tricky to keep \dd's "Name" the same
4. I still would like to get rid of \dd entirely, thus it seems less
important whether it uses pg_comments. It's down to five object types
now; I think that triggers, constraints, and rules could feasibly be
incorporated into \d+ output as Robert suggested upthread, and perhaps
operator class & operator family could get their own backslash
commands.

Some fixes:
* shuffled the query components in describe.c's objectDescription()
so they're alphabetized by object type
* untabified pg_comments in system_views.sql to match its surroundings
* the WHERE d.objsubid = 0 was being omitted in one or two spots,
* the objects with descriptions coming from pg_shdescription, which
does not have the objsubid column, were using NULL::integer instead of
0, as all the other non-column object types should have. This seemed
undesirable, and counter to what the doc page claimed.
* fixed up psql's documentation and help string for \dd

Updated patch attached, along with a revised SQL script to make
testing easier. I can add this to the next CF.

Note, there is a separate thread[1] with just the psql changes broken
out, if it's helpful to consider the psql changes separately from
pg_comments. I do need to update the patch posted there with this
latest set of changes.

Josh
--
[1] http://archives.postgresql.org/pgsql-hackers/2011-07/msg00459.php

[Resending with gzip'ed patch this time, I think the last attempt got eaten.]

On Mon, Jul 18, 2011 at 11:15 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jul 18, 2011 at 10:57 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>>>> 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...
>>>
>>> Yeah... or we could add a function pg_object_is_visible(classoid,
>>> objectoid) that would dispatch to the relevant visibility testing
>>> function based on object type.  Not sure if that's too much of a
>>> kludge, but it wouldn't be useful only here: you could probably use it
>>> in combination with pg_locks, for example.
>>
>> Something like that might work, though an easy escape is just leaving
>> the query style of \dd as it was originally (i.e. querying the various
>> catalogs directly, and not using pg_comments): discussed a bit in the
>> recent pg_comments thread w/ Josh Berkus.
>
> That's another approach.  It seems a bit lame, but...

I went ahead and patched up \dd to display its five object types with
its old-style rooting around in catalogs. I played around again with
the idea of having \dd query pg_comments, but gave up when I realized:

 1. We might not be saving much complexity in \dd's query
 2. Taking the is_system column out was probably good for pg_comments,
but exacerbates point 1.), not to mention the visibility testing that
would have to be done somehow.
 3. The "objname" column of pg_comments is intentionally different
than the "Name" column displayed by \dd; the justification for this
was that \dd's "Name" display wasn't actually useful to recreate the
call to COMMENT ON, but this difference in pg_comments would make it
pretty tricky to keep \dd's "Name" the same
 4. I still would like to get rid of \dd entirely, thus it seems less
important whether it uses pg_comments. It's down to five object types
now; I think that triggers, constraints, and rules could feasibly be
incorporated into \d+ output as Robert suggested upthread, and perhaps
operator class & operator family could get their own backslash
commands.

Some fixes:
 * shuffled the query components in describe.c's objectDescription()
so they're alphabetized by object type
 * untabified pg_comments in system_views.sql to match its surroundings
 * the WHERE d.objsubid = 0 was being omitted in one or two spots,
 * the objects with descriptions coming from pg_shdescription, which
does not have the objsubid column, were using NULL::integer instead of
0, as all the other non-column object types should have. This seemed
undesirable, and counter to what the doc page claimed.
 * fixed up psql's documentation and help string for \dd

Updated patch attached, along with a revised SQL script to make
testing easier. I can add this to the next CF.

Note, there is a separate thread[1] with just the psql changes broken
out, if it's helpful to consider the psql changes separately from
pg_comments. I do need to update the patch posted there with this
latest set of changes.

Josh
--
[1] http://archives.postgresql.org/pgsql-hackers/2011-07/msg00459.php

On Wed, Jul 20, 2011 at 11:38 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:

> Updated patch attached, along with a revised SQL script to make
> testing easier. I can add this to the next CF.
>
> Note, there is a separate thread[1] with just the psql changes broken
> out, if it's helpful to consider the psql changes separately from
> pg_comments. I do need to update the patch posted there with this
> latest set of changes.

The psql changes mentioned above have been committed, so the only
piece remaining is the pg_comments view. I did some light touching up
of the changes in catalogs.sgml: clarify that 'objoid' comes from
either pg_description or pg_shdescription, and try to use periods a
bit more consistently.

Rebased and updated patch attached.

Josh

On 18 August 2011 00:22, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> On Wed, Jul 20, 2011 at 11:38 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>
>> Updated patch attached, along with a revised SQL script to make
>> testing easier. I can add this to the next CF.
>>
>> Note, there is a separate thread[1] with just the psql changes broken
>> out, if it's helpful to consider the psql changes separately from
>> pg_comments. I do need to update the patch posted there with this
>> latest set of changes.
>
> The psql changes mentioned above have been committed, so the only
> piece remaining is the pg_comments view. I did some light touching up
> of the changes in catalogs.sgml: clarify that 'objoid' comes from
> either pg_description or pg_shdescription, and try to use periods a
> bit more consistently.
>
> Rebased and updated patch attached.

Just tested this out on current master. I tried this on every object
capable of having a comment, and the view reports all of them with the
correct details. Doc changes look fine, except for some reason you
removed a full-stop (period) from after "For all other object types,
this column is zero."

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Sep 10, 2011 at 7:47 PM, Thom Brown <thom(at)linux(dot)com> wrote:
> Just tested this out on current master.  I tried this on every object
> capable of having a comment, and the view reports all of them with the
> correct details.  Doc changes look fine, except for some reason you
> removed a full-stop (period) from after "For all other object types,
> this column is zero."

Thanks for the review. Looks like I got confused about where I was
whacking around in catalogs.sgml, good catch of a spurious change.
Fixed patch attached.

Josh