Re: patch: Allow \dd to show constraint comments

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