Re: psql: Add \dL to show languages - Mailing list pgsql-hackers

On Sat, Jan 15, 2011 at 8:26 PM, Andreas Karlsson <andreas@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