On Sat, Jan 15, 2011 at 8:26 PM, Andreas Karlsson <andr...@proxel.se> wrote: > Hi Josh, > > Here is my review of this patch for the commitfest. > > Review of https://commitfest.postgresql.org/action/patch_view?id=439
Thanks a lot for the review! > Contents and Purpose > ==================== > > This patch adds the \dL command in psql to list the procedual languages. > > To me this seems like a useful addition to the commands in psql and \dL > is also a quite sane name for it which follows the established > conventions. > > Documentation of the new command is included and looks good. > > Runing it > ========= > > Patch did not apply cleanly against HEAD so fixed it. > > I tested the comamnds, \dL, \dLS, \dL+, \dLS+ and they wroked as I > expected. Support for patterns worked too and while not that useful it > was not much code either. The psql completion worked fine too. Yeah, IIRC Fernando included pattern-completion in the original patch, and a reviewer said roughly the same thing -- it's probably overkill, but since it's just a small amount of code and it's already in there, no big deal. > Some things I noticed when using it though. > > I do not like the use of parentheses in the usage description "list > (procedural) languages". Why not have it simply as "list procedural > languages"? I agree. > Should we include a column in \dL+ for the laninline function (DO > blocks)? Hrm, I guess that could be useful for the verbose output at least. > Performance > =========== > > Quite irrelevant to this patch. :) > > Coding > ====== > > In general the code looks good and follows conventions except for some > whitesapce errors that I cleaned up. > > * Trailing whitespace in src/bin/psql/describe.c. > * Incorrect indenation, spaces vs tabs in psql/describe.c and > psql/tab-complete.c. > * Removed empty line after return in listLanguages to match other > functions. > > The comment for the function is not that descriptive but it is enough > and fits with the other functions. > > Another things is that generated SQL needs its formatting fixed up in my > oppinion. I recommend looking at the query built by \dL by using psql > -E. Here is an example how the query looks for \dL+ > > SELECT l.lanname AS "Procedural Language", > pg_catalog.pg_get_userbyid(l.lanowner) as "Owner", > l.lanpltrusted AS "Trusted", > l.lanplcallfoid::regprocedure AS "Call Handler", > l.lanvalidator::regprocedure AS "Validator", > NOT l.lanispl AS "System Language", > pg_catalog.array_to_string(l.lanacl, E'\n') AS "Access privileges" FROM > pg_catalog.pg_language l > ORDER BY 1; Sorry, I don't understand.. what's wrong with the formatting of this query? In terms of whitespace, it looks pretty similar to listDomains() directly below listLanguages() in describe.c. > > Conclusion > ========== > > The patch looks useful, worked, and there were no bugs obvious to me. > The code also looks good and in line with other functions doing similar > things. There are just some small issues that I think should be resolved > before committing it: laninline, format of the built query and the usage > string. > > Attached is a version that applies to current HEAD and with whitespace > fixed. > > Regards, > Andreas Thanks for the cleaned up patch. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
