Re: patch: Allow \dd to show constraint comments

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