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
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 5f61561..30d4507 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*************** testdb=>
*** 1249,1254 ****
--- 1249,1269 ----
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>\dL[S+] [ <link linkend="APP-PSQL-patterns"><replaceable class="parameter">pattern</replaceable></link> ]</literal></term>
+ <listitem>
+ <para>
+ Lists all procedural languages. If <replaceable
+ class="parameter">pattern</replaceable>
+ is specified, only languages whose names match the pattern are listed.
+ By default, only user-created languages
+ are shown; supply the <literal>S</literal> modifier to include system
+ objects. If <literal>+</literal> is appended to the command name, each
+ language is listed with its call handler, validator, access privileges,
+ and whether it is a system object.
+ </para>
+ </listitem>
+ </varlistentry>
<varlistentry>
<term><literal>\dn[S+] [ <link linkend="APP-PSQL-patterns"><replaceable class="parameter">pattern</replaceable></link> ]</literal></term>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 962c13c..301dc11 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*************** exec_command(const char *cmd,
*** 416,421 ****
--- 416,424 ----
case 'l':
success = do_lo_list();
break;
+ case 'L':
+ success = listLanguages(pattern, show_verbose, show_system);
+ break;
case 'n':
success = listSchemas(pattern, show_verbose, show_system);
break;
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 205190f..e41453b 100644
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*************** listTables(const char *tabtypes, const c
*** 2566,2571 ****
--- 2566,2635 ----
}
+ /* \dL
+ *
+ * Describes Languages.
+ */
+ bool
+ listLanguages(const char *pattern, bool verbose, bool showSystem)
+ {
+ PQExpBufferData buf;
+ PGresult *res;
+ printQueryOpt myopt = pset.popt;
+
+ initPQExpBuffer(&buf);
+
+ printfPQExpBuffer(&buf,
+ "SELECT l.lanname AS \"%s\",\n",
+ gettext_noop("Procedural Language"));
+ if (pset.sversion >= 80300)
+ appendPQExpBuffer(&buf,
+ " pg_catalog.pg_get_userbyid(l.lanowner) as \"%s\",\n",
+ gettext_noop("Owner"));
+
+ appendPQExpBuffer(&buf,
+ " l.lanpltrusted AS \"%s\"",
+ gettext_noop("Trusted"));
+
+ if (verbose)
+ {
+ appendPQExpBuffer(&buf,
+ ",\n l.lanplcallfoid::regprocedure AS \"%s\",\n"
+ " l.lanvalidator::regprocedure AS \"%s\",\n"
+ " NOT l.lanispl AS \"%s\",\n",
+ gettext_noop("Call Handler"),
+ gettext_noop("Validator"),
+ gettext_noop("System Language"));
+ printACLColumn(&buf, "l.lanacl");
+ }
+
+ appendPQExpBuffer(&buf,
+ " FROM pg_catalog.pg_language l\n");
+
+ processSQLNamePattern(pset.db, &buf, pattern, false, false,
+ NULL, "l.lanname", NULL, NULL);
+
+ if (!showSystem && !pattern)
+ appendPQExpBuffer(&buf, "WHERE lanplcallfoid != 0");
+
+ appendPQExpBuffer(&buf, " ORDER BY 1;");
+
+ res = PSQLexec(buf.data, false);
+ termPQExpBuffer(&buf);
+ if (!res)
+ return false;
+
+ myopt.nullPrint = NULL;
+ myopt.title = _("List of languages");
+ myopt.translate_header = true;
+
+ printQuery(res, &myopt, pset.queryFout, pset.logfile);
+
+ PQclear(res);
+ return true;
+ }
+
+
/*
* \dD
*
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index 2029ef8..4e80bcf 100644
*** a/src/bin/psql/describe.h
--- b/src/bin/psql/describe.h
*************** extern bool listUserMappings(const char
*** 84,88 ****
--- 84,90 ----
/* \det */
extern bool listForeignTables(const char *pattern, bool verbose);
+ /* \dL */
+ extern bool listLanguages(const char *pattern, bool verbose, bool showSystem);
#endif /* DESCRIBE_H */
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 96c85a2..55986f1 100644
*** a/src/bin/psql/help.c
--- b/src/bin/psql/help.c
*************** slashUsage(unsigned short int pager)
*** 211,216 ****
--- 211,217 ----
fprintf(output, _(" \\dg[+] [PATTERN] list roles\n"));
fprintf(output, _(" \\di[S+] [PATTERN] list indexes\n"));
fprintf(output, _(" \\dl list large objects, same as \\lo_list\n"));
+ fprintf(output, _(" \\dL[S+] [PATTERN] list (procedural) languages\n"));
fprintf(output, _(" \\dn[+] [PATTERN] list schemas\n"));
fprintf(output, _(" \\do[S] [PATTERN] list operators\n"));
fprintf(output, _(" \\dp [PATTERN] list table, view, and sequence access privileges\n"));
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 8c15838..84c68a7 100644
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*************** psql_completion(char *text, int start, i
*** 713,719 ****
static const char *const backslash_commands[] = {
"\\a", "\\connect", "\\conninfo", "\\C", "\\cd", "\\copy", "\\copyright",
"\\d", "\\da", "\\db", "\\dc", "\\dC", "\\dd", "\\dD", "\\des", "\\det", "\\deu", "\\dew", "\\df",
! "\\dF", "\\dFd", "\\dFp", "\\dFt", "\\dg", "\\di", "\\dl",
"\\dn", "\\do", "\\dp", "\\drds", "\\ds", "\\dS", "\\dt", "\\dT", "\\dv", "\\du",
"\\e", "\\echo", "\\ef", "\\encoding",
"\\f", "\\g", "\\h", "\\help", "\\H", "\\i", "\\l",
--- 713,719 ----
static const char *const backslash_commands[] = {
"\\a", "\\connect", "\\conninfo", "\\C", "\\cd", "\\copy", "\\copyright",
"\\d", "\\da", "\\db", "\\dc", "\\dC", "\\dd", "\\dD", "\\des", "\\det", "\\deu", "\\dew", "\\df",
! "\\dF", "\\dFd", "\\dFp", "\\dFt", "\\dg", "\\di", "\\dl", "\\dL",
"\\dn", "\\do", "\\dp", "\\drds", "\\ds", "\\dS", "\\dt", "\\dT", "\\dv", "\\du",
"\\e", "\\echo", "\\ef", "\\encoding",
"\\f", "\\g", "\\h", "\\help", "\\H", "\\i", "\\l",
*************** psql_completion(char *text, int start, i
*** 2680,2685 ****
--- 2680,2687 ----
else if (strncmp(prev_wd, "\\di", strlen("\\di")) == 0)
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_indexes, NULL);
+ else if (strncmp(prev_wd, "\\dL", strlen("\\dL")) == 0)
+ COMPLETE_WITH_QUERY(Query_for_list_of_languages);
else if (strncmp(prev_wd, "\\dn", strlen("\\dn")) == 0)
COMPLETE_WITH_QUERY(Query_for_list_of_schemas);
else if (strncmp(prev_wd, "\\dp", strlen("\\dp")) == 0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
