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
