Re: psql: Add \dL to show languages - Mailing list pgsql-hackers
From | Andreas Karlsson |
---|---|
Subject | Re: psql: Add \dL to show languages |
Date | |
Msg-id | 1295141184.17167.3.camel@jansson Whole thread Raw |
In response to | psql: Add \dL to show languages (Josh Kupershmidt <schmiddy@gmail.com>) |
Responses |
Re: psql: Add \dL to show languages
(Magnus Hagander <magnus@hagander.net>)
Re: psql: Add \dL to show languages (Josh Kupershmidt <schmiddy@gmail.com>) |
List | pgsql-hackers |
Hi Josh, Here is my review of this patch for the commitfest. Review of https://commitfest.postgresql.org/action/patch_view?id=439 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. 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"? Should we include a column in \dL+ for the laninline function (DO blocks)? 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; 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
Attachment
pgsql-hackers by date: