On Fri, Jul 22, 2011 at 12:44 PM, Robert Haas <robertmh...@gmail.com> wrote: > On Thu, Jul 21, 2011 at 9:17 PM, Josh Kupershmidt <schmi...@gmail.com> wrote: >> Here's a small patch against branch 8.4 to mention support for COMMENT >> ON index_name.column_name. > > I am not in favor of this - because we'd also need to mention every > other relkind that can support comments. I think if we want to do > something here we should change it to say relation_name, and then > clarify what that means further down. Similarly with the patch for > master. > > Also, if we're going to make a change here, we probably should make > sure it matches the actual behavior. In master, that's to allow > comments on columns of tables, views, composite types, and foreign > tables.
That seems like a good way to document this; patch for master updated. I avoided mucking with the documentation for COMMENT ON RULE and COMMENT ON TRIGGER this time; they both say "table" when they really mean "table or view", but maybe trying to differentiate between "table", "table_or_view", and "relation" will make things overly complicated. >> Also, a patch against master to: >> * get rid of the bogus "Description" outputs for \d+ sequence_name >> and \d+ index_name > > This part looks OK, but instead of doing a negative test (not-index, > not-sequence) let's have it do a positive test, for the same types > comment.c allows. Right, fixed. >> And while I'm messing with this, some further nitpicks about psql not >> addressed by these patches: >> * The "Storage" column for \d+ sequence_name is correct, I suppose, >> but repetitive > > I'm OK with removing that. Hrm, would it be better to keep that Storage bit around in some non-repetitive form, maybe on its own line below the table output? >> * The "Type" column for \dv+ view_name, \di+ index_name, \ds+ >> sequence_name , etc. seems borderline useless.. shouldn't you know >> what type you're looking at based on the backslash command you're >> using? > > Not really. You can do something like this, for example: > > \dti+ > > ...to show both indexes and tables. I see. Didn't know about that trick. Josh
diff --git a/doc/src/sgml/ref/comment.sgml b/doc/src/sgml/ref/comment.sgml index ab12614..736907e 100644 *** a/doc/src/sgml/ref/comment.sgml --- b/doc/src/sgml/ref/comment.sgml *************** COMMENT ON *** 26,32 **** AGGREGATE <replaceable class="PARAMETER">agg_name</replaceable> (<replaceable class="PARAMETER">agg_type</replaceable> [, ...] ) | CAST (<replaceable>source_type</replaceable> AS <replaceable>target_type</replaceable>) | COLLATION <replaceable class="PARAMETER">object_name</replaceable> | ! COLUMN <replaceable class="PARAMETER">table_name</replaceable>.<replaceable class="PARAMETER">column_name</replaceable> | CONSTRAINT <replaceable class="PARAMETER">constraint_name</replaceable> ON <replaceable class="PARAMETER">table_name</replaceable> | CONVERSION <replaceable class="PARAMETER">object_name</replaceable> | DATABASE <replaceable class="PARAMETER">object_name</replaceable> | --- 26,32 ---- AGGREGATE <replaceable class="PARAMETER">agg_name</replaceable> (<replaceable class="PARAMETER">agg_type</replaceable> [, ...] ) | CAST (<replaceable>source_type</replaceable> AS <replaceable>target_type</replaceable>) | COLLATION <replaceable class="PARAMETER">object_name</replaceable> | ! COLUMN <replaceable class="PARAMETER">relation_name</replaceable>.<replaceable class="PARAMETER">column_name</replaceable> | CONSTRAINT <replaceable class="PARAMETER">constraint_name</replaceable> ON <replaceable class="PARAMETER">table_name</replaceable> | CONVERSION <replaceable class="PARAMETER">object_name</replaceable> | DATABASE <replaceable class="PARAMETER">object_name</replaceable> | *************** COMMENT ON *** 97,105 **** <variablelist> <varlistentry> - <term><replaceable class="parameter">object_name</replaceable></term> - <term><replaceable class="parameter">table_name.column_name</replaceable></term> <term><replaceable class="parameter">agg_name</replaceable></term> <term><replaceable class="parameter">constraint_name</replaceable></term> <term><replaceable class="parameter">function_name</replaceable></term> <term><replaceable class="parameter">operator_name</replaceable></term> --- 97,104 ---- <variablelist> <varlistentry> <term><replaceable class="parameter">agg_name</replaceable></term> + <term><replaceable class="parameter">object_name</replaceable></term> <term><replaceable class="parameter">constraint_name</replaceable></term> <term><replaceable class="parameter">function_name</replaceable></term> <term><replaceable class="parameter">operator_name</replaceable></term> *************** COMMENT ON *** 143,148 **** --- 142,158 ---- </para> </listitem> </varlistentry> + + <varlistentry> + <term><replaceable>relation_name.column_name</replaceable></term> + <listitem> + <para> + For comments on columns, the name of the relation and column. Column + comments may be used with tables, views, composite types, and + foreign tables. + </para> + </listitem> + </varlistentry> <varlistentry> <term><replaceable class="parameter">argmode</replaceable></term> diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c index 682cf8a..dda7097 100644 *** a/src/bin/psql/describe.c --- b/src/bin/psql/describe.c *************** describeOneTableDetails(const char *sche *** 1295,1302 **** appendPQExpBuffer(&buf, "\n NULL AS attcollation"); if (tableinfo.relkind == 'i') appendPQExpBuffer(&buf, ",\n pg_catalog.pg_get_indexdef(a.attrelid, a.attnum, TRUE) AS indexdef"); ! if (verbose) ! appendPQExpBuffer(&buf, ",\n a.attstorage, pg_catalog.col_description(a.attrelid, a.attnum)"); appendPQExpBuffer(&buf, "\nFROM pg_catalog.pg_attribute a"); appendPQExpBuffer(&buf, "\nWHERE a.attrelid = '%s' AND a.attnum > 0 AND NOT a.attisdropped", oid); appendPQExpBuffer(&buf, "\nORDER BY a.attnum;"); --- 1295,1311 ---- appendPQExpBuffer(&buf, "\n NULL AS attcollation"); if (tableinfo.relkind == 'i') appendPQExpBuffer(&buf, ",\n pg_catalog.pg_get_indexdef(a.attrelid, a.attnum, TRUE) AS indexdef"); ! if (verbose) { ! appendPQExpBuffer(&buf, ",\n a.attstorage"); ! /* In 9.0+, we have column comments for: relations, views, composite ! * types (not handled here), and foreign tables (c.f. CommentObject() ! * in comment.c). ! */ ! if (tableinfo.relkind == 'r' || tableinfo.relkind == 'v' || ! tableinfo.relkind == 'f' || tableinfo.relkind == 'c') ! appendPQExpBuffer(&buf, ", pg_catalog.col_description(a.attrelid, a.attnum)"); ! } ! appendPQExpBuffer(&buf, "\nFROM pg_catalog.pg_attribute a"); appendPQExpBuffer(&buf, "\nWHERE a.attrelid = '%s' AND a.attnum > 0 AND NOT a.attisdropped", oid); appendPQExpBuffer(&buf, "\nORDER BY a.attnum;"); *************** describeOneTableDetails(const char *sche *** 1379,1385 **** if (verbose) { headers[cols++] = gettext_noop("Storage"); ! headers[cols++] = gettext_noop("Description"); } printTableInit(&cont, &myopt, title.data, cols, numrows); --- 1388,1397 ---- if (verbose) { headers[cols++] = gettext_noop("Storage"); ! /* Column comments, if the relkind supports this feature. */ ! if (tableinfo.relkind == 'r' || tableinfo.relkind == 'v' || ! tableinfo.relkind == 'c' || tableinfo.relkind == 'f') ! headers[cols++] = gettext_noop("Description"); } printTableInit(&cont, &myopt, title.data, cols, numrows); *************** describeOneTableDetails(const char *sche *** 1471,1478 **** (storage[0] == 'e' ? "external" : "???")))), false, false); ! printTableAddCell(&cont, PQgetvalue(res, i, firstvcol + 1), ! false, false); } } --- 1483,1493 ---- (storage[0] == 'e' ? "external" : "???")))), false, false); ! /* Column comments, if the relkind supports this feature. */ ! if (tableinfo.relkind == 'r' || tableinfo.relkind == 'v' || ! tableinfo.relkind == 'c' || tableinfo.relkind == 'f') ! printTableAddCell(&cont, PQgetvalue(res, i, firstvcol + 1), ! false, false); } }
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers