Thread: psql: display of object comments
Hi all, I would like to fix psql's incomplete and inconsistent handling of object comments. I think this subject can be handled separately from the pg_comments discussion, and we can focus on the appearance/behavior of psql commands here. First, As brought up recently [1], comments for the following object types are not currently displayed by any psql backslash command: CAST CONSTRAINT CONVERSION FOREIGN DATA WRAPPER PROCEDURAL LANGUAGE SERVER In the case of PROCEDURAL LANGUAGE, it was mere oversight that \dL did not display comments, and I'm not aware of any reason why we're not doing so for the other objects above. I'd like to propose adding an additional column to the respective psql backslash commands for the above object types which will display any comment associated with the object. Second, comments of the following object types would IMO be better displayed in the object's individual backslash command, and not in \dd as they are now: DOMAIN Third, comments for the following objects are displayed by both \dd and the object's individual backslash command: AGGREGATE FOREIGN TABLE (via \d+) INDEX FUNCTION OPERATOR SEQUENCE TABLE VIEW Seems like there's no need to add more noise to \dd for these cases. Fourth, there is a bug in at least the display of comments for indexes using \d+ (the "Description" is empty when it shouldn't be). As a summary, here is how I am proposing we display object comments [2]: 1.) Object comments displayed by \dd: CONSTRAINT OPERATOR CLASS OPERATOR FAMILY RULE TRIGGER 2.) Object comments displayed in the backslash commands for the object: AGGREGATE \da CAST \dC+ COLLATION \dO COLUMN \d+ tablename CONVERSION \dc DATABASE \l+ DOMAIN \dD and \dT+ EXTENSION \dx FOREIGN DATA WRAPPER \dew FOREIGN TABLE \det and \d+ FUNCTION \df+ INDEX \di+ and \d+ LARGE OBJECT \dl OPERATOR \do PROCEDURAL LANGUAGE \dL ROLE \dg+ SCHEMA \dn+ SEQUENCE \ds+ and \d+ SERVER \des TABLE \dt+ and \d+ TABLESPACE \db+ TYPE \dT TEXT SEARCH CONFIGURATION \dF TEXT SEARCH DICTIONARY \dFd TEXT SEARCH PARSER \dFp TEXT SEARCH TEMPLATE \dFt VIEW \dv+ A few notes: a.) As you can see, there's not much consistency in whether we display object comments only in verbose mode or not. I'm open to suggestions on how to tweak/standardize this. b.) I'd rather not have any overlap between groups 1.) and 2.). c.) \dd doesn't yet display all the object types it should (operator class & family are missing); that's slated to be fixed with the pg_comments patch though. Patch attached and comments welcome; will add to next CF barring objections. I've also attached a test .sql file I was using to make up a bunch of comments, it might save you some time if you're interested in trying the patch out. -- [1] http://archives.postgresql.org/pgsql-hackers/2011-05/msg00991.php [2] The object types broken down here should cover all comment-able types on http://www.postgresql.org/docs/9.1/static/sql-comment.html
Attachment
On Sat, Jul 9, 2011 at 1:16 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote: Attached is an updated version of this patch, lifted out of the recent pg_comments patch. With this v2 patch, \dd should properly show just its five object types, and the psql documentation and help strings should be fixed. > Fourth, there is a bug in at least the display of comments for indexes > using \d+ (the "Description" is empty when it shouldn't be). BTW, this gripe is being dealt with separately in thread "psql: bogus descriptions displayed by \d+". Josh
Attachment
On Fri, Jul 22, 2011 at 10:44 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote: > On Sat, Jul 9, 2011 at 1:16 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote: > > Attached is an updated version of this patch, lifted out of the recent > pg_comments patch. With this v2 patch, \dd should properly show just > its five object types, and the psql documentation and help strings > should be fixed. I took a look at this patch today and I think some of these queries are not quite right. When you do a left join against pg_description, you have this sort of thing in the WHERE clause: (d.objsubid IS NULL OR d.objsubid = 0) I think what you actually want is "AND d.objsubid = 0" in the LEFT JOIN's "ON" clause. Then you are, in effect, only left joining against the rows from pg_description where objsubid = 0, and null-extending if none such is found. I think that's what you want. I think you can remove the XXX comments, too. Unless I'm misunderstanding something, using the table to test visibility for constraints, rules, and triggers seems just right, and opclasses and opfamilies you have a suitable function available, so those don't seem problematic. Or am I confused? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Jul 25, 2011 at 12:27 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Jul 22, 2011 at 10:44 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote: >> On Sat, Jul 9, 2011 at 1:16 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote: >> >> Attached is an updated version of this patch, lifted out of the recent >> pg_comments patch. With this v2 patch, \dd should properly show just >> its five object types, and the psql documentation and help strings >> should be fixed. > > I took a look at this patch today and I think some of these queries > are not quite right. When you do a left join against pg_description, > you have this sort of thing in the WHERE clause: > > (d.objsubid IS NULL OR d.objsubid = 0) > > I think what you actually want is "AND d.objsubid = 0" in the LEFT > JOIN's "ON" clause. Then you are, in effect, only left joining > against the rows from pg_description where objsubid = 0, and > null-extending if none such is found. I think that's what you want. Thanks for taking a look at this. Yeah, that is what I was going for. I believe my way worked, since pg_description declares objsubid NOT NULL, so checking whether d.objsubid IS NULL in the where clause should just match null-extended rows. But anyway, your version is clearer, changed. > I think you can remove the XXX comments, too. Unless I'm > misunderstanding something, using the table to test visibility for > constraints, rules, and triggers seems just right, and opclasses and > opfamilies you have a suitable function available, so those don't seem > problematic. Or am I confused? I bet you're right; at least, it seems to work reasonably in the testing I've done. I had just left those comments in there since they were in all the sub-queries of objectDescription(), but now seems like a good time to get rid of them. Josh
Attachment
On Tue, Jul 26, 2011 at 8:38 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote: > [new patch] I've committed the portion of this that displays comments on languages and casts. For domains and conversions, I am wondering if we should display the comments only when + is specified, since the output is fairly wide already. For foreign data wrappers, foreign servers, and foreign tables, I am wondering if there is any particular rule we should adhere to in terms of where the description shows up in the output column list. It doesn't seem entirely consistent the way you've done it here, but maybe you've put more thought into it than I have. I haven't reviewed the part that changes the \dd output in detail yet (sorry). Residual patch attached... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Thu, Aug 4, 2011 at 12:26 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Jul 26, 2011 at 8:38 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote: >> [new patch] > > I've committed the portion of this that displays comments on languages > and casts. Thanks! > For domains and conversions, I am wondering if we should display the > comments only when + is specified, since the output is fairly wide > already. I wasn't sure whether there was some sort of precedent for whether comments should be displayed only in verbose mode, but looking through the existing backslash commands, it seems reasonable to make it verbose-only if the output is already pushing 80 characters for typical usage (object names and other column outputs of lengths typically encountered). A few existing backslash commands, such as \dn and maybe \db, don't exactly follow this precedent. Not sure if we want to bother adjusting the existing commands to be consistent in this regard. Defining "typical usage" is pretty wishy-washy, so I'm not real inclined to try messing with the existing commands. > For foreign data wrappers, foreign servers, and foreign tables, I am > wondering if there is any particular rule we should adhere to in terms > of where the description shows up in the output column list. It > doesn't seem entirely consistent the way you've done it here, but > maybe you've put more thought into it than I have. Hrm, what wasn't consistent? I intended to just put the "Description" column at the end of the outputs for \dew, \des, and \det, which seems to be the way other commands handle this. Though now that I double check, I notice that the verbose modes of these commands include extra columns which come after "Description", and it might be better to force "Description" to stay at the end in those cases, the way that \dT[+] and \dFt[+] do. Though perhaps you're complaining about something different -- \dL isn't forcing "Description" to the end in verbose mode. Josh
On Thu, Aug 4, 2011 at 10:24 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote: > A few existing backslash commands, such as \dn and maybe \db, don't > exactly follow this precedent. Not sure if we want to bother adjusting > the existing commands to be consistent in this regard. Defining > "typical usage" is pretty wishy-washy, so I'm not real inclined to try > messing with the existing commands. Me neither. >> For foreign data wrappers, foreign servers, and foreign tables, I am >> wondering if there is any particular rule we should adhere to in terms >> of where the description shows up in the output column list. It >> doesn't seem entirely consistent the way you've done it here, but >> maybe you've put more thought into it than I have. > > Hrm, what wasn't consistent? I intended to just put the "Description" > column at the end of the outputs for \dew, \des, and \det, which seems > to be the way other commands handle this. Though now that I double > check, I notice that the verbose modes of these commands include extra > columns which come after "Description", and it might be better to > force "Description" to stay at the end in those cases, the way that > \dT[+] and \dFt[+] do. Though perhaps you're complaining about > something different -- \dL isn't forcing "Description" to the end in > verbose mode. Oh, I see. I was right - you have thought about this more than I have. :-) I guess my vote is to make the SQL/MED stuff show the description only in verbose mode, and always at the end; and revise what we did with \dL to put the description at the end even in verbose mode. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Aug 5, 2011 at 8:32 AM, Robert Haas <robertmhaas@gmail.com> wrote: > I guess my vote is to make the SQL/MED stuff show the description only > in verbose mode, and always at the end; and revise what we did with > \dL to put the description at the end even in verbose mode. Yeah, that sounds fine to me. I've revised the residual patch to do so, as well as incorporated your earlier suggestion about having \dD and \dc only display descriptions in verbose-mode. I did more testing, and found and fixed some brokenness related to d.objsubid I had introduced into the listConversions() query, as well as improved the version checking in objectDescription(). Updated patch attached. Josh
Attachment
On Fri, Aug 5, 2011 at 7:25 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote: > On Fri, Aug 5, 2011 at 8:32 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> I guess my vote is to make the SQL/MED stuff show the description only >> in verbose mode, and always at the end; and revise what we did with >> \dL to put the description at the end even in verbose mode. > > Yeah, that sounds fine to me. I've revised the residual patch to do > so, as well as incorporated your earlier suggestion about having \dD > and \dc only display descriptions in verbose-mode. > > I did more testing, and found and fixed some brokenness related to > d.objsubid I had introduced into the listConversions() query, as well > as improved the version checking in objectDescription(). Updated patch > attached. OK, I've now committed most of this, with some additions to the documentation. Remaining bits attached. I am a bit confused as to why we have both \det and \dE. They seem redundant. Shouldn't we rip one of those out? IMHO, \det should be the one to go, as it could be useful to do, e.g. \dtvE, which isn't going to work with the \det syntax. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Mon, Aug 8, 2011 at 4:34 PM, Robert Haas <robertmhaas@gmail.com> wrote: > OK, I've now committed most of this, with some additions to the > documentation. Remaining bits attached. Looks good, thanks. > I am a bit confused as to why we have both \det and \dE. They seem > redundant. Shouldn't we rip one of those out? IMHO, \det should be > the one to go, as it could be useful to do, e.g. \dtvE, which isn't > going to work with the \det syntax. Yeah, I was wondering that myself. At first I thought maybe someone added in one without being aware of the other, but it looks like both \dE and \det got added in by commit 0d692a0dc9f0e532c67c577187fe5d7d323cb95b. They are using different queries internally (pg_class vs. pg_foreign_table), but I doubt end users would care about that. Or perhaps the author just wanted a command name similar to \dew and \des. +1 for getting rid of one of them; I don't really care which one gets the axe. Though we should make sure to be able to show all possible columns in whichever command we keep (i.e. add "Options" column to \dE+ if we keep that one). BTW, I haven't bothered setting up functioning foreign tables yet, but is "Size" always going to be 0 bytes in \dE+? Josh
On Mon, Aug 8, 2011 at 6:01 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote: > (i.e. add "Options" column to \dE+ if we keep that one). Oh nevermind, "Options" is displayed by \d+ foreign_table_name. Josh
(2011/08/09 7:01), Josh Kupershmidt wrote: >> I am a bit confused as to why we have both \det and \dE. They seem >> redundant. Shouldn't we rip one of those out? IMHO, \det should be >> the one to go, as it could be useful to do, e.g. \dtvE, which isn't >> going to work with the \det syntax. > > Yeah, I was wondering that myself. At first I thought maybe someone > added in one without being aware of the other, but it looks like both > \dE and \det got added in by commit > 0d692a0dc9f0e532c67c577187fe5d7d323cb95b. They are using different > queries internally (pg_class vs. pg_foreign_table), but I doubt end > users would care about that. Or perhaps the author just wanted a > command name similar to \dew and \des. I'm the author of that patch, sorry for confusion. May I explain the background of implementing those command? :) Basically, during implementing foreign table support, I tried to follow the existing design. I found two backslash command groups in psql, \de[wsu] and \d[tvsT], which are relevant to foreign table. Former is a group for listing relation (table, view, sequence and type), and they have common output format. Users would use them (sometimes with combination like \dtv) to list only specified type of relations. OTOH latter is a group for listing SQL/MED objects (wrapper, server and user mapping), and commands in that group have different output columns. Users would use them to see detail of FDW options, maybe mainly FDW options. They had been implemented in different ways then, and I kept implementation similar to existing codes for each. But now, as you say, they seem redundant and inconsistent each other. > +1 for getting rid of one of them; I don't really care which one gets > the axe. Though we should make sure to be able to show all possible > columns in whichever command we keep (i.e. add "Options" column to > \dE+ if we keep that one). I'm not sure whether getting rid of one of them is better. But maybe \dE is useless than \det, because former doesn't shows more information than simple \d. > BTW, I haven't bothered setting up > functioning foreign tables yet, but is "Size" always going to be 0 > bytes in \dE+? Yes, "Size" is total file size calculated with pg_table_size(), but foreign table has no data file. Regards, -- Shigeru Hanada
2011/8/9 Shigeru Hanada <shigeru.hanada@gmail.com>: > I'm the author of that patch, sorry for confusion. May I explain the > background of implementing those command? :) > > Basically, during implementing foreign table support, I tried to follow > the existing design. > > I found two backslash command groups in psql, \de[wsu] and \d[tvsT], > which are relevant to foreign table. Former is a group for listing > relation (table, view, sequence and type), and they have common output > format. Users would use them (sometimes with combination like \dtv) to > list only specified type of relations. OTOH latter is a group for > listing SQL/MED objects (wrapper, server and user mapping), and commands > in that group have different output columns. Users would use them to > see detail of FDW options, maybe mainly FDW options. > > They had been implemented in different ways then, and I kept > implementation similar to existing codes for each. But now, as you say, > they seem redundant and inconsistent each other. > >> +1 for getting rid of one of them; I don't really care which one gets >> the axe. Though we should make sure to be able to show all possible >> columns in whichever command we keep (i.e. add "Options" column to >> \dE+ if we keep that one). > > I'm not sure whether getting rid of one of them is better. > But maybe \dE is useless than \det, because former doesn't shows more > information than simple \d. Looking at this again, I see that they each have certain advantages. \det shows more information that is specific to foreign tables; but I don't want to get rid of \dE because it's useful to be able to do stuff like \dtE to see tables and foreign tables in a single listing. So maybe we should just leave this well enough alone. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Aug 8, 2011 at 6:01 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote: > On Mon, Aug 8, 2011 at 4:34 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> OK, I've now committed most of this, with some additions to the >> documentation. Remaining bits attached. > > Looks good, thanks. And now I've committed (nearly) all of what remains. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company