psql: \dl+ to list large objects privileges
Hello,
While working through the documentation I found an empty cell in the
table for the large objects privilege display with the psql command [1].
And indeed the \dl command does not show privileges. And there is no
modifier + for it.
This patch adds a + modifier to the \dl command and also to the \lo_list
command to display privilege information on large objects.
I decided to move the do_lo_list function to describe.c in order to use
the printACLColumn helper function. And at the same time I renamed
do_lo_list to listLargeObjects to unify with the names of other similar
functions.
I don't like how I handled the + modifier in the \lo_list command. But I
don't know how to do better now. This is the second time I've programmed
in C. The first time was the 'Hello World' program. So maybe something
is done wrong.
If it's interesting, I can add the patch to commitfest.
1.
https://www.postgresql.org/docs/devel/ddl-priv.html#PRIVILEGES-SUMMARY-TABLE
--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company
Attachments:
lo-list-acl.patchtext/x-patch; charset=UTF-8; name=lo-list-acl.patchDownload
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index e0ffb020bf..374515bcb2 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2094,7 +2094,7 @@ REVOKE ALL ON accounts FROM PUBLIC;
<entry><literal>LARGE OBJECT</literal></entry>
<entry><literal>rw</literal></entry>
<entry>none</entry>
- <entry></entry>
+ <entry><literal>\dl+</literal></entry>
</row>
<row>
<entry><literal>SCHEMA</literal></entry>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index fcab5c0d51..de47ef528e 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1681,11 +1681,13 @@ testdb=>
<varlistentry>
- <term><literal>\dl</literal></term>
+ <term><literal>\dl[+]</literal></term>
<listitem>
<para>
This is an alias for <command>\lo_list</command>, which shows a
- list of large objects.
+ list of large objects. If <literal>+</literal> is appended
+ to the command name, each large object is listed with its
+ associated permissions, if any.
</para>
</listitem>
</varlistentry>
@@ -2588,12 +2590,15 @@ lo_import 152801
</varlistentry>
<varlistentry>
- <term><literal>\lo_list</literal></term>
+ <term><literal>\lo_list[+]</literal></term>
<listitem>
<para>
Shows a list of all <productname>PostgreSQL</productname>
large objects currently stored in the database,
along with any comments provided for them.
+ If <literal>+</literal> is appended to the command name,
+ each large object is listed with its associated permissions,
+ if any.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 49d4c0e3ce..5251b0bab8 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -807,7 +807,7 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
success = describeRoles(pattern, show_verbose, show_system);
break;
case 'l':
- success = do_lo_list();
+ success = listLargeObjects(show_verbose);
break;
case 'L':
success = listLanguages(pattern, show_verbose, show_system);
@@ -1936,7 +1936,10 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
}
else if (strcmp(cmd + 3, "list") == 0)
- success = do_lo_list();
+ success = listLargeObjects(false);
+
+ else if (strcmp(cmd + 3, "list+") == 0)
+ success = listLargeObjects(true);
else if (strcmp(cmd + 3, "unlink") == 0)
{
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 90ff649be7..bb04e2d5f7 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6828,3 +6828,65 @@ listOpFamilyFunctions(const char *access_method_pattern,
PQclear(res);
return true;
}
+
+/*
+ * \dl or \lo_list
+ * Lists large objects in database
+ */
+bool
+listLargeObjects(bool verbose)
+{
+ PQExpBufferData buf;
+ PGresult *res;
+ printQueryOpt myopt = pset.popt;
+
+ initPQExpBuffer(&buf);
+
+ if (pset.sversion >= 90000)
+ {
+ printfPQExpBuffer(&buf,
+ "SELECT oid as \"%s\",\n"
+ " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n ",
+ gettext_noop("ID"),
+ gettext_noop("Owner"));
+
+ if (verbose)
+ {
+ printACLColumn(&buf, "lomacl");
+ appendPQExpBufferStr(&buf, ",\n ");
+ }
+
+ appendPQExpBuffer(&buf,
+ "pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
+ "FROM pg_catalog.pg_largeobject_metadata\n"
+ "ORDER BY oid",
+ gettext_noop("Description"));
+
+ }
+ else
+ {
+ printfPQExpBuffer(&buf,
+ "SELECT loid as \"%s\",\n"
+ " pg_catalog.obj_description(loid, 'pg_largeobject') as \"%s\"\n"
+ "FROM (SELECT DISTINCT loid FROM pg_catalog.pg_largeobject) x\n"
+ "ORDER BY 1",
+ gettext_noop("ID"),
+ gettext_noop("Description"));
+ }
+
+ res = PSQLexec(buf.data);
+ termPQExpBuffer(&buf);
+ if (!res)
+ return false;
+
+ myopt.topt.tuples_only = false;
+ myopt.nullPrint = NULL;
+ myopt.title = _("Large objects");
+ myopt.translate_header = true;
+
+ printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
+
+ PQclear(res);
+ return true;
+}
+
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index 71b320f1fc..53738cc0cb 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -139,5 +139,8 @@ extern bool listOpFamilyOperators(const char *accessMethod_pattern,
extern bool listOpFamilyFunctions(const char *access_method_pattern,
const char *family_pattern, bool verbose);
+/* \dl or \lo_list */
+extern bool listLargeObjects(bool verbose);
+
#endif /* DESCRIBE_H */
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index d3fda67edd..7f1c9bc912 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -248,7 +248,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _(" \\dFt[+] [PATTERN] list text search templates\n"));
fprintf(output, _(" \\dg[S+] [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[+] list large objects, same as \\lo_list\n"));
fprintf(output, _(" \\dL[S+] [PATTERN] list procedural languages\n"));
fprintf(output, _(" \\dm[S+] [PATTERN] list materialized views\n"));
fprintf(output, _(" \\dn[S+] [PATTERN] list schemas\n"));
@@ -324,7 +324,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _("Large Objects\n"));
fprintf(output, _(" \\lo_export LOBOID FILE\n"
" \\lo_import FILE [COMMENT]\n"
- " \\lo_list\n"
+ " \\lo_list[+]\n"
" \\lo_unlink LOBOID large object operations\n"));
ClosePager(output);
diff --git a/src/bin/psql/large_obj.c b/src/bin/psql/large_obj.c
index c15fcc0885..243875be83 100644
--- a/src/bin/psql/large_obj.c
+++ b/src/bin/psql/large_obj.c
@@ -262,55 +262,3 @@ do_lo_unlink(const char *loid_arg)
return true;
}
-
-
-
-/*
- * do_lo_list()
- *
- * Show all large objects in database with comments
- */
-bool
-do_lo_list(void)
-{
- PGresult *res;
- char buf[1024];
- printQueryOpt myopt = pset.popt;
-
- if (pset.sversion >= 90000)
- {
- snprintf(buf, sizeof(buf),
- "SELECT oid as \"%s\",\n"
- " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
- " pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
- " FROM pg_catalog.pg_largeobject_metadata "
- " ORDER BY oid",
- gettext_noop("ID"),
- gettext_noop("Owner"),
- gettext_noop("Description"));
- }
- else
- {
- snprintf(buf, sizeof(buf),
- "SELECT loid as \"%s\",\n"
- " pg_catalog.obj_description(loid, 'pg_largeobject') as \"%s\"\n"
- "FROM (SELECT DISTINCT loid FROM pg_catalog.pg_largeobject) x\n"
- "ORDER BY 1",
- gettext_noop("ID"),
- gettext_noop("Description"));
- }
-
- res = PSQLexec(buf);
- if (!res)
- return false;
-
- myopt.topt.tuples_only = false;
- myopt.nullPrint = NULL;
- myopt.title = _("Large objects");
- myopt.translate_header = true;
-
- printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
-
- PQclear(res);
- return true;
-}
diff --git a/src/bin/psql/large_obj.h b/src/bin/psql/large_obj.h
index 003acbf52c..3172a7704d 100644
--- a/src/bin/psql/large_obj.h
+++ b/src/bin/psql/large_obj.h
@@ -11,6 +11,5 @@
bool do_lo_export(const char *loid_arg, const char *filename_arg);
bool do_lo_import(const char *filename_arg, const char *comment_arg);
bool do_lo_unlink(const char *loid_arg);
-bool do_lo_list(void);
#endif /* LARGE_OBJ_H */
On 31 Aug 2021, at 16:14, Pavel Luzanov <p.luzanov@postgrespro.ru> wrote:
If it's interesting, I can add the patch to commitfest.
Please do, if it was interesting enough for you to write it, it’s interesting
enough to be in the commitfest.
--
Daniel Gustafsson https://vmware.com/
On 31.08.2021 17:35, Daniel Gustafsson wrote:
Please do, if it was interesting enough for you to write it, it’s
interesting enough to be in the commitfest.
Thanks, added to the commitfest.
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
Hi,
thank you for the patch, I personally think it is a useful addition and thus it
gets my vote. However, I also think that the proposed code will need some
changes.
On a high level I will recommend the addition of tests. There are similar tests
present in:
./src/test/regress/sql/psql.sql
I will also inquire as to the need for renaming the function `do_lo_list` to
`listLargeObjects` and its move to describe.c. from large_obj.c. In itself it is
not necessarily a blocking point, though it will require some strong arguments
for doing so.
Applying the patch, generates several whitespace warnings. It will be helpful
if those warnings are removed.
The patch contains:
case 'l':
- success = do_lo_list();
+ success = listLargeObjects(show_verbose);It might be of some interest to consider in the above to check the value of the
next character in command or emit an error if not valid. Such a pattern can be
found in the same switch block as for example:
switch (cmd[2])
{
case '\0':
case '+':
<snip>
success = ...
</snip>
break;
default:
status = PSQL_CMD_UNKNOWN;
break;
}
The patch contains:
else if (strcmp(cmd + 3, "list") == 0)
- success = do_lo_list();
+ success = listLargeObjects(false);
+
+ else if (strcmp(cmd + 3, "list+") == 0)
+ success = listLargeObjects(true);In a fashion similar to `exec_command_list`, it might be interesting to consider
expressing the above as:
show_verbose = strchr(cmd, '+') ? true : false;
<snip>
else if (strcmp(cmd + 3, "list") == 0
success = do_lo_list(show_verbose);
Thoughts?
Cheers,
//Georgios
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
Hi,
There is something I forgot to mention in my previous review.
Typically when describing a thing in psql, it is the column "Description" that
belongs in the verbose version. For example listing indexes produces:
List of relations
Schema | Name | Type | Owner | Table
and the verbose version:
List of relations
Schema | Name | Type | Owner | Table | Persistence | Access method | Size | Description
Since '\dl' already contains description, it might be worthwhile to consider to add the column `Access privileges`
without introducing a verbose version.
Thoughts?
Cheers,
//Georgios
Hello,
Thank you very much for review.
Since '\dl' already contains description, it might be worthwhile to consider to add the column `Access privileges`
without introducing a verbose version.
I thought about it.
The reason why I decided to add the verbose version is because of
backward compatibility. Perhaps the appearance of a new column in an
existing command may become undesirable to someone.
If we don't worry about backward compatibility, the patch will be
easier. But I'm not sure this is the right approach.
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company
Hello,
Thank you very mush for review.
I will prepare a new version of the patch according to your comments.
For now, I will answer this question:
I will also inquire as to the need for renaming the function `do_lo_list` to
`listLargeObjects` and its move to describe.c. from large_obj.c. In itself it is
not necessarily a blocking point, though it will require some strong arguments
for doing so.
I understand that I needed a good reason for such actions.
On the one hand all the commands for working with large objects are in
large_obj.c. On the other hand, all commands for displaying the contents
of system catalogs are in describe.c. The function do_lo_list belongs to
both groups.
The main reason for moving the function to describe.c is that I wanted
to use the printACLColumn function to display lomacl column.
printACLColumn function is used in all the other commands to display
privileges and this function is locally defined in describe.c and there
is no reason to make in public.
Another option is to duplicate the printACLColumn function (or its
contents) in large_obj.c. This seemed wrong to me.
Is it any other way?
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company
Hi,
On 03.09.2021 15:25, Georgios Kokolatos wrote:
On a high level I will recommend the addition of tests. There are similar tests
Tests added.
Applying the patch, generates several whitespace warnings. It will be helpful
if those warnings are removed.
I know this is a silly mistake, and after reading this article[1]https://wiki.postgresql.org/wiki/Creating_Clean_Patches I
tried to remove the extra spaces.
Can you tell me, please, how can you get such warnings?
The patch contains:
case 'l': - success = do_lo_list(); + success = listLargeObjects(show_verbose);It might be of some interest to consider in the above to check the value of the
next character in command or emit an error if not valid. Such a pattern can be
found in the same switch block as for example:switch (cmd[2])
{
case '\0':
case '+':
<snip>
success = ...
</snip>
break;
default:
status = PSQL_CMD_UNKNOWN;
break;
}
Check added.
The patch contains:
else if (strcmp(cmd + 3, "list") == 0) - success = do_lo_list(); + success = listLargeObjects(false); + + else if (strcmp(cmd + 3, "list+") == 0) + success = listLargeObjects(true);In a fashion similar to `exec_command_list`, it might be interesting to consider
expressing the above as:show_verbose = strchr(cmd, '+') ? true : false;
<snip>
else if (strcmp(cmd + 3, "list") == 0
success = do_lo_list(show_verbose);
I rewrote this part.
New version attached.
[1]: https://wiki.postgresql.org/wiki/Creating_Clean_Patches
--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company
Attachments:
lo-list-acl-v2.patchtext/x-patch; charset=UTF-8; name=lo-list-acl-v2.patchDownload
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index e0ffb020bf..374515bcb2 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2094,7 +2094,7 @@ REVOKE ALL ON accounts FROM PUBLIC;
<entry><literal>LARGE OBJECT</literal></entry>
<entry><literal>rw</literal></entry>
<entry>none</entry>
- <entry></entry>
+ <entry><literal>\dl+</literal></entry>
</row>
<row>
<entry><literal>SCHEMA</literal></entry>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index fcab5c0d51..de47ef528e 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1681,11 +1681,13 @@ testdb=>
<varlistentry>
- <term><literal>\dl</literal></term>
+ <term><literal>\dl[+]</literal></term>
<listitem>
<para>
This is an alias for <command>\lo_list</command>, which shows a
- list of large objects.
+ list of large objects. If <literal>+</literal> is appended
+ to the command name, each large object is listed with its
+ associated permissions, if any.
</para>
</listitem>
</varlistentry>
@@ -2588,12 +2590,15 @@ lo_import 152801
</varlistentry>
<varlistentry>
- <term><literal>\lo_list</literal></term>
+ <term><literal>\lo_list[+]</literal></term>
<listitem>
<para>
Shows a list of all <productname>PostgreSQL</productname>
large objects currently stored in the database,
along with any comments provided for them.
+ If <literal>+</literal> is appended to the command name,
+ each large object is listed with its associated permissions,
+ if any.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 49d4c0e3ce..f3645cab96 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -807,7 +807,16 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
success = describeRoles(pattern, show_verbose, show_system);
break;
case 'l':
- success = do_lo_list();
+ switch (cmd[2])
+ {
+ case '\0':
+ case '+':
+ success = listLargeObjects(show_verbose);
+ break;
+ default:
+ status = PSQL_CMD_UNKNOWN;
+ break;
+ }
break;
case 'L':
success = listLanguages(pattern, show_verbose, show_system);
@@ -1901,11 +1910,13 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
{
char *opt1,
*opt2;
+ bool show_verbose;
opt1 = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
opt2 = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
+ show_verbose = strchr(cmd, '+') ? true : false;
if (strcmp(cmd + 3, "export") == 0)
{
@@ -1935,8 +1946,8 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
}
}
- else if (strcmp(cmd + 3, "list") == 0)
- success = do_lo_list();
+ else if (strcmp(cmd + 3, "list") == 0 || strcmp(cmd + 3, "list+") == 0)
+ success = listLargeObjects(show_verbose);
else if (strcmp(cmd + 3, "unlink") == 0)
{
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 90ff649be7..a079ce9e36 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6828,3 +6828,64 @@ listOpFamilyFunctions(const char *access_method_pattern,
PQclear(res);
return true;
}
+
+/*
+ * \dl or \lo_list
+ * Lists large objects in database
+ */
+bool
+listLargeObjects(bool verbose)
+{
+ PQExpBufferData buf;
+ PGresult *res;
+ printQueryOpt myopt = pset.popt;
+
+ initPQExpBuffer(&buf);
+
+ if (pset.sversion >= 90000)
+ {
+ printfPQExpBuffer(&buf,
+ "SELECT oid as \"%s\",\n"
+ " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n ",
+ gettext_noop("ID"),
+ gettext_noop("Owner"));
+
+ if (verbose)
+ {
+ printACLColumn(&buf, "lomacl");
+ appendPQExpBufferStr(&buf, ",\n ");
+ }
+
+ appendPQExpBuffer(&buf,
+ "pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
+ "FROM pg_catalog.pg_largeobject_metadata\n"
+ "ORDER BY oid",
+ gettext_noop("Description"));
+
+ }
+ else
+ {
+ printfPQExpBuffer(&buf,
+ "SELECT loid as \"%s\",\n"
+ " pg_catalog.obj_description(loid, 'pg_largeobject') as \"%s\"\n"
+ "FROM (SELECT DISTINCT loid FROM pg_catalog.pg_largeobject) x\n"
+ "ORDER BY 1",
+ gettext_noop("ID"),
+ gettext_noop("Description"));
+ }
+
+ res = PSQLexec(buf.data);
+ termPQExpBuffer(&buf);
+ if (!res)
+ return false;
+
+ myopt.topt.tuples_only = false;
+ myopt.nullPrint = NULL;
+ myopt.title = _("Large objects");
+ myopt.translate_header = true;
+
+ printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
+
+ PQclear(res);
+ return true;
+}
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index 71b320f1fc..53738cc0cb 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -139,5 +139,8 @@ extern bool listOpFamilyOperators(const char *accessMethod_pattern,
extern bool listOpFamilyFunctions(const char *access_method_pattern,
const char *family_pattern, bool verbose);
+/* \dl or \lo_list */
+extern bool listLargeObjects(bool verbose);
+
#endif /* DESCRIBE_H */
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index d3fda67edd..7f1c9bc912 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -248,7 +248,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _(" \\dFt[+] [PATTERN] list text search templates\n"));
fprintf(output, _(" \\dg[S+] [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[+] list large objects, same as \\lo_list\n"));
fprintf(output, _(" \\dL[S+] [PATTERN] list procedural languages\n"));
fprintf(output, _(" \\dm[S+] [PATTERN] list materialized views\n"));
fprintf(output, _(" \\dn[S+] [PATTERN] list schemas\n"));
@@ -324,7 +324,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _("Large Objects\n"));
fprintf(output, _(" \\lo_export LOBOID FILE\n"
" \\lo_import FILE [COMMENT]\n"
- " \\lo_list\n"
+ " \\lo_list[+]\n"
" \\lo_unlink LOBOID large object operations\n"));
ClosePager(output);
diff --git a/src/bin/psql/large_obj.c b/src/bin/psql/large_obj.c
index c15fcc0885..243875be83 100644
--- a/src/bin/psql/large_obj.c
+++ b/src/bin/psql/large_obj.c
@@ -262,55 +262,3 @@ do_lo_unlink(const char *loid_arg)
return true;
}
-
-
-
-/*
- * do_lo_list()
- *
- * Show all large objects in database with comments
- */
-bool
-do_lo_list(void)
-{
- PGresult *res;
- char buf[1024];
- printQueryOpt myopt = pset.popt;
-
- if (pset.sversion >= 90000)
- {
- snprintf(buf, sizeof(buf),
- "SELECT oid as \"%s\",\n"
- " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
- " pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
- " FROM pg_catalog.pg_largeobject_metadata "
- " ORDER BY oid",
- gettext_noop("ID"),
- gettext_noop("Owner"),
- gettext_noop("Description"));
- }
- else
- {
- snprintf(buf, sizeof(buf),
- "SELECT loid as \"%s\",\n"
- " pg_catalog.obj_description(loid, 'pg_largeobject') as \"%s\"\n"
- "FROM (SELECT DISTINCT loid FROM pg_catalog.pg_largeobject) x\n"
- "ORDER BY 1",
- gettext_noop("ID"),
- gettext_noop("Description"));
- }
-
- res = PSQLexec(buf);
- if (!res)
- return false;
-
- myopt.topt.tuples_only = false;
- myopt.nullPrint = NULL;
- myopt.title = _("Large objects");
- myopt.translate_header = true;
-
- printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
-
- PQclear(res);
- return true;
-}
diff --git a/src/bin/psql/large_obj.h b/src/bin/psql/large_obj.h
index 003acbf52c..3172a7704d 100644
--- a/src/bin/psql/large_obj.h
+++ b/src/bin/psql/large_obj.h
@@ -11,6 +11,5 @@
bool do_lo_export(const char *loid_arg, const char *filename_arg);
bool do_lo_import(const char *filename_arg, const char *comment_arg);
bool do_lo_unlink(const char *loid_arg);
-bool do_lo_list(void);
#endif /* LARGE_OBJ_H */
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 1b2f6bc418..62002e4ec3 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -5179,3 +5179,49 @@ List of access methods
pg_catalog | && | anyarray | anyarray | boolean | overlaps
(1 row)
+-- check printing info about large objects
+CREATE ROLE lo_test;
+SELECT lo_create(42);
+ lo_create
+-----------
+ 42
+(1 row)
+
+ALTER LARGE OBJECT 42 OWNER TO lo_test;
+GRANT SELECT ON LARGE OBJECT 42 TO public;
+\dl
+ Large objects
+ ID | Owner | Description
+----+---------+-------------
+ 42 | lo_test |
+(1 row)
+
+\dl+
+ Large objects
+ ID | Owner | Access privileges | Description
+----+---------+--------------------+-------------
+ 42 | lo_test | lo_test=rw/lo_test+|
+ | | =r/lo_test |
+(1 row)
+
+\lo_list
+ Large objects
+ ID | Owner | Description
+----+---------+-------------
+ 42 | lo_test |
+(1 row)
+
+\lo_list+
+ Large objects
+ ID | Owner | Access privileges | Description
+----+---------+--------------------+-------------
+ 42 | lo_test | lo_test=rw/lo_test+|
+ | | =r/lo_test |
+(1 row)
+
+\dl-
+invalid command \dl-
+\lo_list-
+invalid command \lo_list-
+\lo_unlink 42
+DROP ROLE lo_test;
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 68121d171c..43b38213f6 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1241,3 +1241,18 @@ drop role regress_partitioning_role;
\dfa bit* small*
\do - pg_catalog.int4
\do && anyarray *
+
+-- check printing info about large objects
+CREATE ROLE lo_test;
+SELECT lo_create(42);
+ALTER LARGE OBJECT 42 OWNER TO lo_test;
+GRANT SELECT ON LARGE OBJECT 42 TO public;
+\dl
+\dl+
+\lo_list
+\lo_list+
+\dl-
+\lo_list-
+\lo_unlink 42
+DROP ROLE lo_test;
+
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Sunday, September 5th, 2021 at 21:47, Pavel Luzanov <p.luzanov@postgrespro.ru> wrote:
Hi,
Hi,
On 03.09.2021 15:25, Georgios Kokolatos wrote:
On a high level I will recommend the addition of tests. There are similar tests
Tests added.
Thanks! The tests look good. A minor nitpick would be to also add a comment on the
large object to verify that it is picked up correctly.
Also:
+\lo_unlink 42
+DROP ROLE lo_test;
+That last empty line can be removed.
Applying the patch, generates several whitespace warnings. It will be helpful
if those warnings are removed.I know this is a silly mistake, and after reading this article[1] I tried to remove the extra spaces.
Can you tell me, please, how can you get such warnings?
I only mentioned it because I thought you might find it useful.
I apply patches by `git apply` and executing the command on the latest version
of your patch, produces:
$ git apply lo-list-acl-v2.patch
lo-list-acl-v2.patch:349: new blank line at EOF.
+
warning: 1 line adds whitespace errors
The same can be observed highlighted by executing `git diff --color` as
recommended in the the article you linked.
The patch contains:
case 'l': - success = do_lo_list(); + success = listLargeObjects(show_verbose);It might be of some interest to consider in the above to check the value of the
next character in command or emit an error if not valid. Such a pattern can be
found in the same switch block as for example:switch (cmd[2])
{
case '\0':
case '+':
<snip>
success = ...
</snip>
break;
default:
status = PSQL_CMD_UNKNOWN;
break;
}Check added.
Thanks.
The patch contains:
else if (strcmp(cmd + 3, "list") == 0) - success = do_lo_list(); + success = listLargeObjects(false); + + else if (strcmp(cmd + 3, "list+") == 0) + success = listLargeObjects(true);In a fashion similar to `exec_command_list`, it might be interesting to consider
expressing the above as:show_verbose = strchr(cmd, '+') ? true : false;
<snip>
else if (strcmp(cmd + 3, "list") == 0
success = do_lo_list(show_verbose);I rewrote this part.
Thank you. It looks good to me.
New version attached.
The new version looks good to me.
I did spend a bit of time considering the addition of the verbose version of
the command. I understand your reasoning explained upstream in the same thread.
However, I am not entirely convinced. The columns in consideration are,
"Description" and "Access Privileges". Within the describe commands there are
four different options, listed and explained bellow:
commands where description is found in verbose
\d \dA \dc \dd \des \df \dFd \dFt \di \dL \dn \dO \dP \dPt \dt \du \dx \dy \da
\db \dC \dD \det \dew \dFp \dg \dl \dm \do \dPi \dS \dT
commands where description is not found in verbose (including object
description)
\dd \dFd \dFt \dL \dx \da \dF \dFp \dl \do \dT
commands where access privileges is found in verbose
\def \dD \des
commands where access privileges is not found in verbose (including access
privilege describing)
\ddp \dp \des \df \dL \dn \db \dD \dew \dl \dT
With the above list, I would argue that it feels more natural to include
the "Access Privileges" column in the non verbose version and be done with
the verbose version all together.
My apologies for the back and forth on this detail.
Thoughts?
Cheers,
//Georgios
Show quoted text
[1] https://wiki.postgresql.org/wiki/Creating_Clean_Patches
--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company
Hi,
On 06.09.2021 14:39, gkokolatos@pm.me wrote:
I apply patches by `git apply` and executing the command on the latest version
of your patch, produces:$ git apply lo-list-acl-v2.patch
lo-list-acl-v2.patch:349: new blank line at EOF.
+
warning: 1 line adds whitespace errors
Thanks, this is what I was looking for. The patch command doesn't show these warnings
(or I don't know the right way for use it).
I did spend a bit of time considering the addition of the verbose version of
the command. I understand your reasoning explained upstream in the same thread.
However, I am not entirely convinced. The columns in consideration are,
"Description" and "Access Privileges". Within the describe commands there are
four different options, listed and explained bellow:commands where description is found in verbose
\d \dA \dc \dd \des \df \dFd \dFt \di \dL \dn \dO \dP \dPt \dt \du \dx \dy \da
\db \dC \dD \det \dew \dFp \dg \dl \dm \do \dPi \dS \dTcommands where description is not found in verbose (including object
description)
\dd \dFd \dFt \dL \dx \da \dF \dFp \dl \do \dTcommands where access privileges is found in verbose
\def \dD \descommands where access privileges is not found in verbose (including access
privilege describing)
\ddp \dp \des \df \dL \dn \db \dD \dew \dl \dTWith the above list, I would argue that it feels more natural to include
the "Access Privileges" column in the non verbose version and be done with
the verbose version all together.
My thoughts.
For most object types, the Description column is shown only in the verbose
version of the commands. But there are several object types,
including Large Objects, for which the description is shown in the normal version.
Both are valid options, so the Description column for large objects stays
in the normal version of the command.
Regarding the display of access privileges.
Instances of object types for which you can manage the access privileges
are listed in Table 5.2 [1]https://www.postgresql.org/docs/devel/ddl-priv.html#PRIVILEGES-SUMMARY-TABLE.
For clarity, I will only show the first and last columns:
Table 5.2. Summary of Access Privileges
Object Type psql Command
------------------------------ ------------
DATABASE \l
DOMAIN \dD+
FUNCTION or PROCEDURE \df+
FOREIGN DATA WRAPPER \dew+
FOREIGN SERVER \des+
LANGUAGE \dL+
LARGE OBJECT
SCHEMA \dn+
SEQUENCE \dp
TABLE (and table-like objects) \dp
Table column \dp
TABLESPACE \db+
TYPE \dT+
By the way, after seeing an empty cell for Large Objects in this table,
I decided to make a patch.
Note that the \dp command is specially designed to display access privileges,
so you don't need to pay attention to the lack of a + sign for it.
It turns out that all commands use the verbose version (or special command)
to display access privileges. Except the \l command for the databases.
Now the question.
Should we add a second exception and display access privileges
for large objects with the \dl command or do the verbose version
like most other commands: \dl+
?
If you still think that there is no need for a verbose version,
I will drop it and add an 'Access privileges' column to the normal command.
[1]: https://www.postgresql.org/docs/devel/ddl-priv.html#PRIVILEGES-SUMMARY-TABLE
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company
Hi,
On 06.09.2021 14:39, gkokolatos@pm.me wrote:
Thanks! The tests look good. A minor nitpick would be to also add a comment on the
large object to verify that it is picked up correctly.Also:
+\lo_unlink 42 +DROP ROLE lo_test; +That last empty line can be removed.
The new version adds a comment to a large object and removes the last empty line.
--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company
Attachments:
lo-list-acl-v3.patchtext/x-patch; charset=UTF-8; name=lo-list-acl-v3.patchDownload
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index e0ffb020bf..374515bcb2 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2094,7 +2094,7 @@ REVOKE ALL ON accounts FROM PUBLIC;
<entry><literal>LARGE OBJECT</literal></entry>
<entry><literal>rw</literal></entry>
<entry>none</entry>
- <entry></entry>
+ <entry><literal>\dl+</literal></entry>
</row>
<row>
<entry><literal>SCHEMA</literal></entry>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index fcab5c0d51..de47ef528e 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1681,11 +1681,13 @@ testdb=>
<varlistentry>
- <term><literal>\dl</literal></term>
+ <term><literal>\dl[+]</literal></term>
<listitem>
<para>
This is an alias for <command>\lo_list</command>, which shows a
- list of large objects.
+ list of large objects. If <literal>+</literal> is appended
+ to the command name, each large object is listed with its
+ associated permissions, if any.
</para>
</listitem>
</varlistentry>
@@ -2588,12 +2590,15 @@ lo_import 152801
</varlistentry>
<varlistentry>
- <term><literal>\lo_list</literal></term>
+ <term><literal>\lo_list[+]</literal></term>
<listitem>
<para>
Shows a list of all <productname>PostgreSQL</productname>
large objects currently stored in the database,
along with any comments provided for them.
+ If <literal>+</literal> is appended to the command name,
+ each large object is listed with its associated permissions,
+ if any.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 49d4c0e3ce..f3645cab96 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -807,7 +807,16 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
success = describeRoles(pattern, show_verbose, show_system);
break;
case 'l':
- success = do_lo_list();
+ switch (cmd[2])
+ {
+ case '\0':
+ case '+':
+ success = listLargeObjects(show_verbose);
+ break;
+ default:
+ status = PSQL_CMD_UNKNOWN;
+ break;
+ }
break;
case 'L':
success = listLanguages(pattern, show_verbose, show_system);
@@ -1901,11 +1910,13 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
{
char *opt1,
*opt2;
+ bool show_verbose;
opt1 = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
opt2 = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
+ show_verbose = strchr(cmd, '+') ? true : false;
if (strcmp(cmd + 3, "export") == 0)
{
@@ -1935,8 +1946,8 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
}
}
- else if (strcmp(cmd + 3, "list") == 0)
- success = do_lo_list();
+ else if (strcmp(cmd + 3, "list") == 0 || strcmp(cmd + 3, "list+") == 0)
+ success = listLargeObjects(show_verbose);
else if (strcmp(cmd + 3, "unlink") == 0)
{
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 90ff649be7..a079ce9e36 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6828,3 +6828,64 @@ listOpFamilyFunctions(const char *access_method_pattern,
PQclear(res);
return true;
}
+
+/*
+ * \dl or \lo_list
+ * Lists large objects in database
+ */
+bool
+listLargeObjects(bool verbose)
+{
+ PQExpBufferData buf;
+ PGresult *res;
+ printQueryOpt myopt = pset.popt;
+
+ initPQExpBuffer(&buf);
+
+ if (pset.sversion >= 90000)
+ {
+ printfPQExpBuffer(&buf,
+ "SELECT oid as \"%s\",\n"
+ " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n ",
+ gettext_noop("ID"),
+ gettext_noop("Owner"));
+
+ if (verbose)
+ {
+ printACLColumn(&buf, "lomacl");
+ appendPQExpBufferStr(&buf, ",\n ");
+ }
+
+ appendPQExpBuffer(&buf,
+ "pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
+ "FROM pg_catalog.pg_largeobject_metadata\n"
+ "ORDER BY oid",
+ gettext_noop("Description"));
+
+ }
+ else
+ {
+ printfPQExpBuffer(&buf,
+ "SELECT loid as \"%s\",\n"
+ " pg_catalog.obj_description(loid, 'pg_largeobject') as \"%s\"\n"
+ "FROM (SELECT DISTINCT loid FROM pg_catalog.pg_largeobject) x\n"
+ "ORDER BY 1",
+ gettext_noop("ID"),
+ gettext_noop("Description"));
+ }
+
+ res = PSQLexec(buf.data);
+ termPQExpBuffer(&buf);
+ if (!res)
+ return false;
+
+ myopt.topt.tuples_only = false;
+ myopt.nullPrint = NULL;
+ myopt.title = _("Large objects");
+ myopt.translate_header = true;
+
+ printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
+
+ PQclear(res);
+ return true;
+}
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index 71b320f1fc..53738cc0cb 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -139,5 +139,8 @@ extern bool listOpFamilyOperators(const char *accessMethod_pattern,
extern bool listOpFamilyFunctions(const char *access_method_pattern,
const char *family_pattern, bool verbose);
+/* \dl or \lo_list */
+extern bool listLargeObjects(bool verbose);
+
#endif /* DESCRIBE_H */
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index d3fda67edd..7f1c9bc912 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -248,7 +248,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _(" \\dFt[+] [PATTERN] list text search templates\n"));
fprintf(output, _(" \\dg[S+] [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[+] list large objects, same as \\lo_list\n"));
fprintf(output, _(" \\dL[S+] [PATTERN] list procedural languages\n"));
fprintf(output, _(" \\dm[S+] [PATTERN] list materialized views\n"));
fprintf(output, _(" \\dn[S+] [PATTERN] list schemas\n"));
@@ -324,7 +324,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _("Large Objects\n"));
fprintf(output, _(" \\lo_export LOBOID FILE\n"
" \\lo_import FILE [COMMENT]\n"
- " \\lo_list\n"
+ " \\lo_list[+]\n"
" \\lo_unlink LOBOID large object operations\n"));
ClosePager(output);
diff --git a/src/bin/psql/large_obj.c b/src/bin/psql/large_obj.c
index c15fcc0885..243875be83 100644
--- a/src/bin/psql/large_obj.c
+++ b/src/bin/psql/large_obj.c
@@ -262,55 +262,3 @@ do_lo_unlink(const char *loid_arg)
return true;
}
-
-
-
-/*
- * do_lo_list()
- *
- * Show all large objects in database with comments
- */
-bool
-do_lo_list(void)
-{
- PGresult *res;
- char buf[1024];
- printQueryOpt myopt = pset.popt;
-
- if (pset.sversion >= 90000)
- {
- snprintf(buf, sizeof(buf),
- "SELECT oid as \"%s\",\n"
- " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
- " pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
- " FROM pg_catalog.pg_largeobject_metadata "
- " ORDER BY oid",
- gettext_noop("ID"),
- gettext_noop("Owner"),
- gettext_noop("Description"));
- }
- else
- {
- snprintf(buf, sizeof(buf),
- "SELECT loid as \"%s\",\n"
- " pg_catalog.obj_description(loid, 'pg_largeobject') as \"%s\"\n"
- "FROM (SELECT DISTINCT loid FROM pg_catalog.pg_largeobject) x\n"
- "ORDER BY 1",
- gettext_noop("ID"),
- gettext_noop("Description"));
- }
-
- res = PSQLexec(buf);
- if (!res)
- return false;
-
- myopt.topt.tuples_only = false;
- myopt.nullPrint = NULL;
- myopt.title = _("Large objects");
- myopt.translate_header = true;
-
- printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
-
- PQclear(res);
- return true;
-}
diff --git a/src/bin/psql/large_obj.h b/src/bin/psql/large_obj.h
index 003acbf52c..3172a7704d 100644
--- a/src/bin/psql/large_obj.h
+++ b/src/bin/psql/large_obj.h
@@ -11,6 +11,5 @@
bool do_lo_export(const char *loid_arg, const char *filename_arg);
bool do_lo_import(const char *filename_arg, const char *comment_arg);
bool do_lo_unlink(const char *loid_arg);
-bool do_lo_list(void);
#endif /* LARGE_OBJ_H */
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 1b2f6bc418..00ec76d728 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -5179,3 +5179,51 @@ List of access methods
pg_catalog | && | anyarray | anyarray | boolean | overlaps
(1 row)
+-- check printing info about large objects
+-- one large object with OID=42 and owner lo_test is expected in the output
+CREATE ROLE lo_test;
+SELECT lo_create(42);
+ lo_create
+-----------
+ 42
+(1 row)
+
+ALTER LARGE OBJECT 42 OWNER TO lo_test;
+COMMENT ON LARGE OBJECT 42 IS 'the answer to the ultimate question of life';
+GRANT SELECT ON LARGE OBJECT 42 TO public;
+\dl
+ Large objects
+ ID | Owner | Description
+----+---------+---------------------------------------------
+ 42 | lo_test | the answer to the ultimate question of life
+(1 row)
+
+\dl+
+ Large objects
+ ID | Owner | Access privileges | Description
+----+---------+--------------------+---------------------------------------------
+ 42 | lo_test | lo_test=rw/lo_test+| the answer to the ultimate question of life
+ | | =r/lo_test |
+(1 row)
+
+\lo_list
+ Large objects
+ ID | Owner | Description
+----+---------+---------------------------------------------
+ 42 | lo_test | the answer to the ultimate question of life
+(1 row)
+
+\lo_list+
+ Large objects
+ ID | Owner | Access privileges | Description
+----+---------+--------------------+---------------------------------------------
+ 42 | lo_test | lo_test=rw/lo_test+| the answer to the ultimate question of life
+ | | =r/lo_test |
+(1 row)
+
+\dl-
+invalid command \dl-
+\lo_list-
+invalid command \lo_list-
+\lo_unlink 42
+DROP ROLE lo_test;
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 68121d171c..ceb1555170 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1241,3 +1241,19 @@ drop role regress_partitioning_role;
\dfa bit* small*
\do - pg_catalog.int4
\do && anyarray *
+
+-- check printing info about large objects
+-- one large object with OID=42 and owner lo_test is expected in the output
+CREATE ROLE lo_test;
+SELECT lo_create(42);
+ALTER LARGE OBJECT 42 OWNER TO lo_test;
+COMMENT ON LARGE OBJECT 42 IS 'the answer to the ultimate question of life';
+GRANT SELECT ON LARGE OBJECT 42 TO public;
+\dl
+\dl+
+\lo_list
+\lo_list+
+\dl-
+\lo_list-
+\lo_unlink 42
+DROP ROLE lo_test;
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed
Hi, I think this is an interesting patch. +1
I tested it for the latest version, and it works well.
Hi,
Thank you for testing.
As far as I understand, for the patch to move forward, someone has to become a reviewer
and change the status in the commitfest app.
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company
Show quoted text
On 18.09.2021 05:41, Neil Chen wrote:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passedHi, I think this is an interesting patch. +1
I tested it for the latest version, and it works well.
On Tue, Aug 31, 2021 at 05:14:12PM +0300, Pavel Luzanov wrote:
I decided to move the do_lo_list function to describe.c in order to use the
printACLColumn helper function. And at the same time I renamed do_lo_list to
listLargeObjects to unify with the names of other similar functions.
The tabs were changed to spaces when you moved the function.
I suggest to move the function in a separate 0001 commit, which makes no code
changes other than moving from one file to another.
A committer would probably push them as a single patch, but this makes it
easier to read and review the changes in 0002.
Possibly like git diff HEAD~:src/bin/psql/large_obj.c src/bin/psql/describe.c
+ if (pset.sversion >= 90000)
Since a few weeks ago, psql no longer supports server versions before 9.2, so
the "if" branch can go away.
I don't like how I handled the + modifier in the \lo_list command. But I
don't know how to do better now. This is the second time I've programmed in
C. The first time was the 'Hello World' program. So maybe something is done
wrong.
I think everywhere else just uses verbose = strchr(cmd, '+') != 0;
--
Justin
Justin Pryzby <pryzby@telsasoft.com> writes:
I suggest to move the function in a separate 0001 commit, which makes no code
changes other than moving from one file to another.
A committer would probably push them as a single patch, but this makes it
easier to read and review the changes in 0002.
Yeah, I agree with that idea. It's really tough to notice small changes
by hand when the entire code block has been moved somewhere else.
Since a few weeks ago, psql no longer supports server versions before 9.2, so
the "if" branch can go away.
And, in fact, the patch is no longer applying per the cfbot, because
that hasn't been done.
To move things along a bit, I split the patch more or less as Justin
suggests and brought it up to HEAD. I did not study the command.c
changes, but the rest of it seems okay, with one exception: I don't like
the test case much at all. For one thing, it will fail in the buildfarm
because you didn't adhere to the convention that roles created by a
regression test must be named regress_something. For another, there's
considerable overlap with testing done in largeobject.sql, which
already creates some commented large objects. That means there's
an undesirable ordering dependency --- if somebody wanted to run
largeobject.sql first, the expected output of psql.sql would change.
I wonder if we shouldn't put these new tests in largeobject.sql instead.
(That would mean there are two expected-files to change, which is a bit
tedious, but it's not very hard.)
regards, tom lane
Attachments:
0001-move-and-rename-do_lo_list.patchtext/x-diff; charset=us-ascii; name=0001-move-and-rename-do_lo_list.patchDownload
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index fb3bab9494..0d64757899 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -811,7 +811,7 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
success = describeRoles(pattern, show_verbose, show_system);
break;
case 'l':
- success = do_lo_list();
+ success = listLargeObjects();
break;
case 'L':
success = listLanguages(pattern, show_verbose, show_system);
@@ -1963,7 +1963,7 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
}
else if (strcmp(cmd + 3, "list") == 0)
- success = do_lo_list();
+ success = listLargeObjects();
else if (strcmp(cmd + 3, "unlink") == 0)
{
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index c28788e84f..0d92f3a80b 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6455,3 +6455,39 @@ listOpFamilyFunctions(const char *access_method_pattern,
PQclear(res);
return true;
}
+
+/*
+ * \dl or \lo_list
+ * Lists large objects
+ */
+bool
+listLargeObjects(void)
+{
+ PGresult *res;
+ char buf[1024];
+ printQueryOpt myopt = pset.popt;
+
+ snprintf(buf, sizeof(buf),
+ "SELECT oid as \"%s\",\n"
+ " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
+ " pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
+ " FROM pg_catalog.pg_largeobject_metadata "
+ " ORDER BY oid",
+ gettext_noop("ID"),
+ gettext_noop("Owner"),
+ gettext_noop("Description"));
+
+ res = PSQLexec(buf);
+ if (!res)
+ return false;
+
+ myopt.topt.tuples_only = false;
+ myopt.nullPrint = NULL;
+ myopt.title = _("Large objects");
+ myopt.translate_header = true;
+
+ printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
+
+ PQclear(res);
+ return true;
+}
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index 71b320f1fc..3a55e1e4ed 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -139,5 +139,7 @@ extern bool listOpFamilyOperators(const char *accessMethod_pattern,
extern bool listOpFamilyFunctions(const char *access_method_pattern,
const char *family_pattern, bool verbose);
+/* \dl or \lo_list */
+extern bool listLargeObjects(void);
#endif /* DESCRIBE_H */
diff --git a/src/bin/psql/large_obj.c b/src/bin/psql/large_obj.c
index 10e47c87ac..243875be83 100644
--- a/src/bin/psql/large_obj.c
+++ b/src/bin/psql/large_obj.c
@@ -262,42 +262,3 @@ do_lo_unlink(const char *loid_arg)
return true;
}
-
-
-
-/*
- * do_lo_list()
- *
- * Show all large objects in database with comments
- */
-bool
-do_lo_list(void)
-{
- PGresult *res;
- char buf[1024];
- printQueryOpt myopt = pset.popt;
-
- snprintf(buf, sizeof(buf),
- "SELECT oid as \"%s\",\n"
- " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
- " pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
- " FROM pg_catalog.pg_largeobject_metadata "
- " ORDER BY oid",
- gettext_noop("ID"),
- gettext_noop("Owner"),
- gettext_noop("Description"));
-
- res = PSQLexec(buf);
- if (!res)
- return false;
-
- myopt.topt.tuples_only = false;
- myopt.nullPrint = NULL;
- myopt.title = _("Large objects");
- myopt.translate_header = true;
-
- printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
-
- PQclear(res);
- return true;
-}
diff --git a/src/bin/psql/large_obj.h b/src/bin/psql/large_obj.h
index 003acbf52c..3172a7704d 100644
--- a/src/bin/psql/large_obj.h
+++ b/src/bin/psql/large_obj.h
@@ -11,6 +11,5 @@
bool do_lo_export(const char *loid_arg, const char *filename_arg);
bool do_lo_import(const char *filename_arg, const char *comment_arg);
bool do_lo_unlink(const char *loid_arg);
-bool do_lo_list(void);
#endif /* LARGE_OBJ_H */
0002-print-large-object-acls.patchtext/x-diff; charset=us-ascii; name=0002-print-large-object-acls.patchDownload
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 64d9030652..22f6c5c7ab 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2146,7 +2146,7 @@ REVOKE ALL ON accounts FROM PUBLIC;
<entry><literal>LARGE OBJECT</literal></entry>
<entry><literal>rw</literal></entry>
<entry>none</entry>
- <entry></entry>
+ <entry><literal>\dl+</literal></entry>
</row>
<row>
<entry><literal>SCHEMA</literal></entry>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae38d3dcc3..1ab200a4ad 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1681,11 +1681,14 @@ testdb=>
<varlistentry>
- <term><literal>\dl</literal></term>
+ <term><literal>\dl[+]</literal></term>
<listitem>
<para>
This is an alias for <command>\lo_list</command>, which shows a
list of large objects.
+ If <literal>+</literal> is appended to the command name,
+ each large object is listed with its associated permissions,
+ if any.
</para>
</listitem>
</varlistentry>
@@ -2610,12 +2613,15 @@ lo_import 152801
</varlistentry>
<varlistentry>
- <term><literal>\lo_list</literal></term>
+ <term><literal>\lo_list[+]</literal></term>
<listitem>
<para>
Shows a list of all <productname>PostgreSQL</productname>
large objects currently stored in the database,
along with any comments provided for them.
+ If <literal>+</literal> is appended to the command name,
+ each large object is listed with its associated permissions,
+ if any.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 0d64757899..ceedcc860c 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -811,7 +811,16 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
success = describeRoles(pattern, show_verbose, show_system);
break;
case 'l':
- success = listLargeObjects();
+ switch (cmd[2])
+ {
+ case '\0':
+ case '+':
+ success = listLargeObjects(show_verbose);
+ break;
+ default:
+ status = PSQL_CMD_UNKNOWN;
+ break;
+ }
break;
case 'L':
success = listLanguages(pattern, show_verbose, show_system);
@@ -1928,11 +1937,13 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
{
char *opt1,
*opt2;
+ bool show_verbose;
opt1 = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
opt2 = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
+ show_verbose = strchr(cmd, '+') ? true : false;
if (strcmp(cmd + 3, "export") == 0)
{
@@ -1962,8 +1973,8 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
}
}
- else if (strcmp(cmd + 3, "list") == 0)
- success = listLargeObjects();
+ else if (strcmp(cmd + 3, "list") == 0 || strcmp(cmd + 3, "list+") == 0)
+ success = listLargeObjects(show_verbose);
else if (strcmp(cmd + 3, "unlink") == 0)
{
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 0d92f3a80b..5edab7f938 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6461,27 +6461,37 @@ listOpFamilyFunctions(const char *access_method_pattern,
* Lists large objects
*/
bool
-listLargeObjects(void)
+listLargeObjects(bool verbose)
{
+ PQExpBufferData buf;
PGresult *res;
- char buf[1024];
printQueryOpt myopt = pset.popt;
- snprintf(buf, sizeof(buf),
- "SELECT oid as \"%s\",\n"
- " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
- " pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
- " FROM pg_catalog.pg_largeobject_metadata "
- " ORDER BY oid",
- gettext_noop("ID"),
- gettext_noop("Owner"),
- gettext_noop("Description"));
-
- res = PSQLexec(buf);
+ initPQExpBuffer(&buf);
+
+ printfPQExpBuffer(&buf,
+ "SELECT oid as \"%s\",\n"
+ " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n ",
+ gettext_noop("ID"),
+ gettext_noop("Owner"));
+
+ if (verbose)
+ {
+ printACLColumn(&buf, "lomacl");
+ appendPQExpBufferStr(&buf, ",\n ");
+ }
+
+ appendPQExpBuffer(&buf,
+ "pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
+ "FROM pg_catalog.pg_largeobject_metadata\n"
+ "ORDER BY oid",
+ gettext_noop("Description"));
+
+ res = PSQLexec(buf.data);
+ termPQExpBuffer(&buf);
if (!res)
return false;
- myopt.topt.tuples_only = false;
myopt.nullPrint = NULL;
myopt.title = _("Large objects");
myopt.translate_header = true;
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index 3a55e1e4ed..b57ba67bba 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -140,6 +140,6 @@ extern bool listOpFamilyFunctions(const char *access_method_pattern,
const char *family_pattern, bool verbose);
/* \dl or \lo_list */
-extern bool listLargeObjects(void);
+extern bool listLargeObjects(bool verbose);
#endif /* DESCRIBE_H */
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 8cadfbb103..5752a36ac8 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -248,7 +248,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _(" \\dFt[+] [PATTERN] list text search templates\n"));
fprintf(output, _(" \\dg[S+] [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[+] list large objects, same as \\lo_list\n"));
fprintf(output, _(" \\dL[S+] [PATTERN] list procedural languages\n"));
fprintf(output, _(" \\dm[S+] [PATTERN] list materialized views\n"));
fprintf(output, _(" \\dn[S+] [PATTERN] list schemas\n"));
@@ -325,7 +325,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _("Large Objects\n"));
fprintf(output, _(" \\lo_export LOBOID FILE\n"
" \\lo_import FILE [COMMENT]\n"
- " \\lo_list\n"
+ " \\lo_list[+]\n"
" \\lo_unlink LOBOID large object operations\n"));
ClosePager(output);
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 6428ebc507..0aa98317a7 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -5290,3 +5290,36 @@ ERROR: relation "notexists" does not exist
LINE 1: SELECT * FROM notexists;
^
STATEMENT: SELECT * FROM notexists;
+ lo_create
+-----------
+ 42
+(1 row)
+
+ Large objects
+ ID | Owner | Description
+----+---------+---------------------------------------------
+ 42 | lo_test | the answer to the ultimate question of life
+(1 row)
+
+ Large objects
+ ID | Owner | Access privileges | Description
+----+---------+--------------------+---------------------------------------------
+ 42 | lo_test | lo_test=rw/lo_test+| the answer to the ultimate question of life
+ | | =r/lo_test |
+(1 row)
+
+ Large objects
+ ID | Owner | Description
+----+---------+---------------------------------------------
+ 42 | lo_test | the answer to the ultimate question of life
+(1 row)
+
+ Large objects
+ ID | Owner | Access privileges | Description
+----+---------+--------------------+---------------------------------------------
+ 42 | lo_test | lo_test=rw/lo_test+| the answer to the ultimate question of life
+ | | =r/lo_test |
+(1 row)
+
+invalid command \dl-
+invalid command \lo_list-
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index d4e4fdbbb7..dea081f937 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1316,3 +1316,19 @@ DROP TABLE oer_test;
\set ECHO errors
SELECT * FROM notexists;
\set ECHO none
+
+-- check printing info about large objects
+-- one large object with OID=42 and owner lo_test is expected in the output
+CREATE ROLE lo_test;
+SELECT lo_create(42);
+ALTER LARGE OBJECT 42 OWNER TO lo_test;
+COMMENT ON LARGE OBJECT 42 IS 'the answer to the ultimate question of life';
+GRANT SELECT ON LARGE OBJECT 42 TO public;
+\dl
+\dl+
+\lo_list
+\lo_list+
+\dl-
+\lo_list-
+\lo_unlink 42
+DROP ROLE lo_test;
Justin, Tom,
Thanks for the review and the help in splitting the patch into two parts.
I wonder if we shouldn't put these new tests in largeobject.sql instead.
(That would mean there are two expected-files to change, which is a bit
tedious, but it's not very hard.)
As suggested, I moved the tests from psql.sql to largeobject.sql.
The tests are added to the beginning of the file because I need one
large object with a known id and a known owner.� This guarantees the
same output of \dl+ as expected.
I made the same changes to two files in the 'expected' directory:
largeobject.out and largeobject_1.out.
Although I must say that I still can't understand why more than one file
with expected result is used for some tests.
Also, I decided to delete following line in the listLargeObjects
function because all the other commands in describe.c do not contain it:
��� myopt.topt.tuples_only = false;
This changed the existing behavior, but is consistent with the other
commands.
Second version (after splitting) is attached.
v2-0001-move-and-rename-do_lo_list.patch - no changes,
v2-0002-print-large-object-acls.patch - tests moved to largeobject.sql
--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company
Attachments:
v2-0001-move-and-rename-do_lo_list.patchtext/x-patch; charset=UTF-8; name=v2-0001-move-and-rename-do_lo_list.patchDownload
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index fb3bab9494..0d64757899 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -811,7 +811,7 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
success = describeRoles(pattern, show_verbose, show_system);
break;
case 'l':
- success = do_lo_list();
+ success = listLargeObjects();
break;
case 'L':
success = listLanguages(pattern, show_verbose, show_system);
@@ -1963,7 +1963,7 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
}
else if (strcmp(cmd + 3, "list") == 0)
- success = do_lo_list();
+ success = listLargeObjects();
else if (strcmp(cmd + 3, "unlink") == 0)
{
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index c28788e84f..0d92f3a80b 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6455,3 +6455,39 @@ listOpFamilyFunctions(const char *access_method_pattern,
PQclear(res);
return true;
}
+
+/*
+ * \dl or \lo_list
+ * Lists large objects
+ */
+bool
+listLargeObjects(void)
+{
+ PGresult *res;
+ char buf[1024];
+ printQueryOpt myopt = pset.popt;
+
+ snprintf(buf, sizeof(buf),
+ "SELECT oid as \"%s\",\n"
+ " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
+ " pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
+ " FROM pg_catalog.pg_largeobject_metadata "
+ " ORDER BY oid",
+ gettext_noop("ID"),
+ gettext_noop("Owner"),
+ gettext_noop("Description"));
+
+ res = PSQLexec(buf);
+ if (!res)
+ return false;
+
+ myopt.topt.tuples_only = false;
+ myopt.nullPrint = NULL;
+ myopt.title = _("Large objects");
+ myopt.translate_header = true;
+
+ printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
+
+ PQclear(res);
+ return true;
+}
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index 71b320f1fc..3a55e1e4ed 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -139,5 +139,7 @@ extern bool listOpFamilyOperators(const char *accessMethod_pattern,
extern bool listOpFamilyFunctions(const char *access_method_pattern,
const char *family_pattern, bool verbose);
+/* \dl or \lo_list */
+extern bool listLargeObjects(void);
#endif /* DESCRIBE_H */
diff --git a/src/bin/psql/large_obj.c b/src/bin/psql/large_obj.c
index 10e47c87ac..243875be83 100644
--- a/src/bin/psql/large_obj.c
+++ b/src/bin/psql/large_obj.c
@@ -262,42 +262,3 @@ do_lo_unlink(const char *loid_arg)
return true;
}
-
-
-
-/*
- * do_lo_list()
- *
- * Show all large objects in database with comments
- */
-bool
-do_lo_list(void)
-{
- PGresult *res;
- char buf[1024];
- printQueryOpt myopt = pset.popt;
-
- snprintf(buf, sizeof(buf),
- "SELECT oid as \"%s\",\n"
- " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
- " pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
- " FROM pg_catalog.pg_largeobject_metadata "
- " ORDER BY oid",
- gettext_noop("ID"),
- gettext_noop("Owner"),
- gettext_noop("Description"));
-
- res = PSQLexec(buf);
- if (!res)
- return false;
-
- myopt.topt.tuples_only = false;
- myopt.nullPrint = NULL;
- myopt.title = _("Large objects");
- myopt.translate_header = true;
-
- printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
-
- PQclear(res);
- return true;
-}
diff --git a/src/bin/psql/large_obj.h b/src/bin/psql/large_obj.h
index 003acbf52c..3172a7704d 100644
--- a/src/bin/psql/large_obj.h
+++ b/src/bin/psql/large_obj.h
@@ -11,6 +11,5 @@
bool do_lo_export(const char *loid_arg, const char *filename_arg);
bool do_lo_import(const char *filename_arg, const char *comment_arg);
bool do_lo_unlink(const char *loid_arg);
-bool do_lo_list(void);
#endif /* LARGE_OBJ_H */
v2-0002-print-large-object-acls.patchtext/x-patch; charset=UTF-8; name=v2-0002-print-large-object-acls.patchDownload
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 64d9030652..22f6c5c7ab 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2146,7 +2146,7 @@ REVOKE ALL ON accounts FROM PUBLIC;
<entry><literal>LARGE OBJECT</literal></entry>
<entry><literal>rw</literal></entry>
<entry>none</entry>
- <entry></entry>
+ <entry><literal>\dl+</literal></entry>
</row>
<row>
<entry><literal>SCHEMA</literal></entry>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae38d3dcc3..1ab200a4ad 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1681,11 +1681,14 @@ testdb=>
<varlistentry>
- <term><literal>\dl</literal></term>
+ <term><literal>\dl[+]</literal></term>
<listitem>
<para>
This is an alias for <command>\lo_list</command>, which shows a
list of large objects.
+ If <literal>+</literal> is appended to the command name,
+ each large object is listed with its associated permissions,
+ if any.
</para>
</listitem>
</varlistentry>
@@ -2610,12 +2613,15 @@ lo_import 152801
</varlistentry>
<varlistentry>
- <term><literal>\lo_list</literal></term>
+ <term><literal>\lo_list[+]</literal></term>
<listitem>
<para>
Shows a list of all <productname>PostgreSQL</productname>
large objects currently stored in the database,
along with any comments provided for them.
+ If <literal>+</literal> is appended to the command name,
+ each large object is listed with its associated permissions,
+ if any.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 0d64757899..ceedcc860c 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -811,7 +811,16 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
success = describeRoles(pattern, show_verbose, show_system);
break;
case 'l':
- success = listLargeObjects();
+ switch (cmd[2])
+ {
+ case '\0':
+ case '+':
+ success = listLargeObjects(show_verbose);
+ break;
+ default:
+ status = PSQL_CMD_UNKNOWN;
+ break;
+ }
break;
case 'L':
success = listLanguages(pattern, show_verbose, show_system);
@@ -1928,11 +1937,13 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
{
char *opt1,
*opt2;
+ bool show_verbose;
opt1 = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
opt2 = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
+ show_verbose = strchr(cmd, '+') ? true : false;
if (strcmp(cmd + 3, "export") == 0)
{
@@ -1962,8 +1973,8 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
}
}
- else if (strcmp(cmd + 3, "list") == 0)
- success = listLargeObjects();
+ else if (strcmp(cmd + 3, "list") == 0 || strcmp(cmd + 3, "list+") == 0)
+ success = listLargeObjects(show_verbose);
else if (strcmp(cmd + 3, "unlink") == 0)
{
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index aa68e9dc6b..cdb33719ff 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6469,27 +6469,37 @@ listOpFamilyFunctions(const char *access_method_pattern,
* Lists large objects
*/
bool
-listLargeObjects(void)
+listLargeObjects(bool verbose)
{
+ PQExpBufferData buf;
PGresult *res;
- char buf[1024];
printQueryOpt myopt = pset.popt;
- snprintf(buf, sizeof(buf),
- "SELECT oid as \"%s\",\n"
- " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
- " pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
- " FROM pg_catalog.pg_largeobject_metadata "
- " ORDER BY oid",
- gettext_noop("ID"),
- gettext_noop("Owner"),
- gettext_noop("Description"));
-
- res = PSQLexec(buf);
+ initPQExpBuffer(&buf);
+
+ printfPQExpBuffer(&buf,
+ "SELECT oid as \"%s\",\n"
+ " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n ",
+ gettext_noop("ID"),
+ gettext_noop("Owner"));
+
+ if (verbose)
+ {
+ printACLColumn(&buf, "lomacl");
+ appendPQExpBufferStr(&buf, ",\n ");
+ }
+
+ appendPQExpBuffer(&buf,
+ "pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
+ "FROM pg_catalog.pg_largeobject_metadata\n"
+ "ORDER BY oid",
+ gettext_noop("Description"));
+
+ res = PSQLexec(buf.data);
+ termPQExpBuffer(&buf);
if (!res)
return false;
- myopt.topt.tuples_only = false;
myopt.nullPrint = NULL;
myopt.title = _("Large objects");
myopt.translate_header = true;
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index 3a55e1e4ed..b57ba67bba 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -140,6 +140,6 @@ extern bool listOpFamilyFunctions(const char *access_method_pattern,
const char *family_pattern, bool verbose);
/* \dl or \lo_list */
-extern bool listLargeObjects(void);
+extern bool listLargeObjects(bool verbose);
#endif /* DESCRIBE_H */
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 8cadfbb103..5752a36ac8 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -248,7 +248,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _(" \\dFt[+] [PATTERN] list text search templates\n"));
fprintf(output, _(" \\dg[S+] [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[+] list large objects, same as \\lo_list\n"));
fprintf(output, _(" \\dL[S+] [PATTERN] list procedural languages\n"));
fprintf(output, _(" \\dm[S+] [PATTERN] list materialized views\n"));
fprintf(output, _(" \\dn[S+] [PATTERN] list schemas\n"));
@@ -325,7 +325,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _("Large Objects\n"));
fprintf(output, _(" \\lo_export LOBOID FILE\n"
" \\lo_import FILE [COMMENT]\n"
- " \\lo_list\n"
+ " \\lo_list[+]\n"
" \\lo_unlink LOBOID large object operations\n"));
ClosePager(output);
diff --git a/src/test/regress/expected/largeobject.out b/src/test/regress/expected/largeobject.out
index f461ca3b9f..e1f469b1bd 100644
--- a/src/test/regress/expected/largeobject.out
+++ b/src/test/regress/expected/largeobject.out
@@ -4,33 +4,77 @@
-- directory paths are passed to us in environment variables
\getenv abs_srcdir PG_ABS_SRCDIR
\getenv abs_builddir PG_ABS_BUILDDIR
--- ensure consistent test output regardless of the default bytea format
-SET bytea_output TO escape;
--- Load a file
-CREATE TABLE lotest_stash_values (loid oid, fd integer);
--- lo_creat(mode integer) returns oid
--- The mode arg to lo_creat is unused, some vestigal holdover from ancient times
--- returns the large object id
-INSERT INTO lotest_stash_values (loid) SELECT lo_creat(42);
+-- Test printing large object acls.
+-- The expected output should contain only one large object
+-- with a known id (42) and a known owner (regress_lo_user).
+SELECT lo_create(42);
+ lo_create
+-----------
+ 42
+(1 row)
+
-- Test ALTER LARGE OBJECT
CREATE ROLE regress_lo_user;
-DO $$
- BEGIN
- EXECUTE 'ALTER LARGE OBJECT ' || (select loid from lotest_stash_values)
- || ' OWNER TO regress_lo_user';
- END
-$$;
+ALTER LARGE OBJECT 42 OWNER TO regress_lo_user;
SELECT
rol.rolname
FROM
- lotest_stash_values s
- JOIN pg_largeobject_metadata lo ON s.loid = lo.oid
- JOIN pg_authid rol ON lo.lomowner = rol.oid;
+ pg_largeobject_metadata lo
+ JOIN pg_authid rol ON lo.lomowner = rol.oid
+WHERE
+ lo.oid = 42;
rolname
-----------------
regress_lo_user
(1 row)
+COMMENT ON LARGE OBJECT 42 IS 'the answer to the ultimate question of life';
+GRANT SELECT ON LARGE OBJECT 42 TO public;
+\dl
+ Large objects
+ ID | Owner | Description
+----+-----------------+---------------------------------------------
+ 42 | regress_lo_user | the answer to the ultimate question of life
+(1 row)
+
+\dl+
+ Large objects
+ ID | Owner | Access privileges | Description
+----+-----------------+------------------------------------+---------------------------------------------
+ 42 | regress_lo_user | regress_lo_user=rw/regress_lo_user+| the answer to the ultimate question of life
+ | | =r/regress_lo_user |
+(1 row)
+
+\lo_list
+ Large objects
+ ID | Owner | Description
+----+-----------------+---------------------------------------------
+ 42 | regress_lo_user | the answer to the ultimate question of life
+(1 row)
+
+\lo_list+
+ Large objects
+ ID | Owner | Access privileges | Description
+----+-----------------+------------------------------------+---------------------------------------------
+ 42 | regress_lo_user | regress_lo_user=rw/regress_lo_user+| the answer to the ultimate question of life
+ | | =r/regress_lo_user |
+(1 row)
+
+-- Invalid commands
+\dl-
+invalid command \dl-
+\lo_list-
+invalid command \lo_list-
+-- Cleanup
+\lo_unlink 42
+-- ensure consistent test output regardless of the default bytea format
+SET bytea_output TO escape;
+-- Load a file
+CREATE TABLE lotest_stash_values (loid oid, fd integer);
+-- lo_creat(mode integer) returns oid
+-- The mode arg to lo_creat is unused, some vestigal holdover from ancient times
+-- returns the large object id
+INSERT INTO lotest_stash_values (loid) SELECT lo_creat(42);
-- NOTE: large objects require transactions
BEGIN;
-- lo_open(lobjId oid, mode integer) returns integer
diff --git a/src/test/regress/expected/largeobject_1.out b/src/test/regress/expected/largeobject_1.out
index a9725c375d..39e27b9609 100644
--- a/src/test/regress/expected/largeobject_1.out
+++ b/src/test/regress/expected/largeobject_1.out
@@ -4,33 +4,77 @@
-- directory paths are passed to us in environment variables
\getenv abs_srcdir PG_ABS_SRCDIR
\getenv abs_builddir PG_ABS_BUILDDIR
--- ensure consistent test output regardless of the default bytea format
-SET bytea_output TO escape;
--- Load a file
-CREATE TABLE lotest_stash_values (loid oid, fd integer);
--- lo_creat(mode integer) returns oid
--- The mode arg to lo_creat is unused, some vestigal holdover from ancient times
--- returns the large object id
-INSERT INTO lotest_stash_values (loid) SELECT lo_creat(42);
+-- Test printing large object acls.
+-- The expected output should contain only one large object
+-- with a known id (42) and a known owner (regress_lo_user).
+SELECT lo_create(42);
+ lo_create
+-----------
+ 42
+(1 row)
+
-- Test ALTER LARGE OBJECT
CREATE ROLE regress_lo_user;
-DO $$
- BEGIN
- EXECUTE 'ALTER LARGE OBJECT ' || (select loid from lotest_stash_values)
- || ' OWNER TO regress_lo_user';
- END
-$$;
+ALTER LARGE OBJECT 42 OWNER TO regress_lo_user;
SELECT
rol.rolname
FROM
- lotest_stash_values s
- JOIN pg_largeobject_metadata lo ON s.loid = lo.oid
- JOIN pg_authid rol ON lo.lomowner = rol.oid;
+ pg_largeobject_metadata lo
+ JOIN pg_authid rol ON lo.lomowner = rol.oid
+WHERE
+ lo.oid = 42;
rolname
-----------------
regress_lo_user
(1 row)
+COMMENT ON LARGE OBJECT 42 IS 'the answer to the ultimate question of life';
+GRANT SELECT ON LARGE OBJECT 42 TO public;
+\dl
+ Large objects
+ ID | Owner | Description
+----+-----------------+---------------------------------------------
+ 42 | regress_lo_user | the answer to the ultimate question of life
+(1 row)
+
+\dl+
+ Large objects
+ ID | Owner | Access privileges | Description
+----+-----------------+------------------------------------+---------------------------------------------
+ 42 | regress_lo_user | regress_lo_user=rw/regress_lo_user+| the answer to the ultimate question of life
+ | | =r/regress_lo_user |
+(1 row)
+
+\lo_list
+ Large objects
+ ID | Owner | Description
+----+-----------------+---------------------------------------------
+ 42 | regress_lo_user | the answer to the ultimate question of life
+(1 row)
+
+\lo_list+
+ Large objects
+ ID | Owner | Access privileges | Description
+----+-----------------+------------------------------------+---------------------------------------------
+ 42 | regress_lo_user | regress_lo_user=rw/regress_lo_user+| the answer to the ultimate question of life
+ | | =r/regress_lo_user |
+(1 row)
+
+-- Invalid commands
+\dl-
+invalid command \dl-
+\lo_list-
+invalid command \lo_list-
+-- Cleanup
+\lo_unlink 42
+-- ensure consistent test output regardless of the default bytea format
+SET bytea_output TO escape;
+-- Load a file
+CREATE TABLE lotest_stash_values (loid oid, fd integer);
+-- lo_creat(mode integer) returns oid
+-- The mode arg to lo_creat is unused, some vestigal holdover from ancient times
+-- returns the large object id
+INSERT INTO lotest_stash_values (loid) SELECT lo_creat(42);
-- NOTE: large objects require transactions
BEGIN;
-- lo_open(lobjId oid, mode integer) returns integer
diff --git a/src/test/regress/sql/largeobject.sql b/src/test/regress/sql/largeobject.sql
index 16da077f3a..a9c2f4cadc 100644
--- a/src/test/regress/sql/largeobject.sql
+++ b/src/test/regress/sql/largeobject.sql
@@ -6,6 +6,34 @@
\getenv abs_srcdir PG_ABS_SRCDIR
\getenv abs_builddir PG_ABS_BUILDDIR
+-- Test printing large object acls.
+-- The expected output should contain only one large object
+-- with a known id (42) and a known owner (regress_lo_user).
+SELECT lo_create(42);
+
+-- Test ALTER LARGE OBJECT
+CREATE ROLE regress_lo_user;
+ALTER LARGE OBJECT 42 OWNER TO regress_lo_user;
+SELECT
+ rol.rolname
+FROM
+ pg_largeobject_metadata lo
+ JOIN pg_authid rol ON lo.lomowner = rol.oid
+WHERE
+ lo.oid = 42;
+
+COMMENT ON LARGE OBJECT 42 IS 'the answer to the ultimate question of life';
+GRANT SELECT ON LARGE OBJECT 42 TO public;
+\dl
+\dl+
+\lo_list
+\lo_list+
+-- Invalid commands
+\dl-
+\lo_list-
+-- Cleanup
+\lo_unlink 42
+
-- ensure consistent test output regardless of the default bytea format
SET bytea_output TO escape;
@@ -16,21 +44,6 @@ CREATE TABLE lotest_stash_values (loid oid, fd integer);
-- returns the large object id
INSERT INTO lotest_stash_values (loid) SELECT lo_creat(42);
--- Test ALTER LARGE OBJECT
-CREATE ROLE regress_lo_user;
-DO $$
- BEGIN
- EXECUTE 'ALTER LARGE OBJECT ' || (select loid from lotest_stash_values)
- || ' OWNER TO regress_lo_user';
- END
-$$;
-SELECT
- rol.rolname
-FROM
- lotest_stash_values s
- JOIN pg_largeobject_metadata lo ON s.loid = lo.oid
- JOIN pg_authid rol ON lo.lomowner = rol.oid;
-
-- NOTE: large objects require transactions
BEGIN;
Pavel Luzanov <p.luzanov@postgrespro.ru> writes:
I wonder if we shouldn't put these new tests in largeobject.sql instead.
(That would mean there are two expected-files to change, which is a bit
tedious, but it's not very hard.)
I made the same changes to two files in the 'expected' directory:
largeobject.out and largeobject_1.out.
Although I must say that I still can't understand why more than one file
with expected result is used for some tests.
Because the output sometimes varies across platforms. IIRC, the
case where largeobject_1.out is needed is for Windows, where the
file that gets inserted into one of the LOs might contain CR/LF
not just LF newlines, so the LO contents look different.
Also, I decided to delete following line in the listLargeObjects
function because all the other commands in describe.c do not contain it:
myopt.topt.tuples_only = false;
Agreed, I'd done that already in my version of the patch.
Second version (after splitting) is attached.
Pushed with some minor editorialization.
regards, tom lane
On 06.01.2022 21:13, Tom Lane wrote:
I made the same changes to two files in the 'expected' directory:
largeobject.out and largeobject_1.out.
Although I must say that I still can't understand why more than one file
with expected result is used for some tests.Because the output sometimes varies across platforms. IIRC, the
case where largeobject_1.out is needed is for Windows, where the
file that gets inserted into one of the LOs might contain CR/LF
not just LF newlines, so the LO contents look different.
So simple. Thanks for the explanation.
Pushed with some minor editorialization.
Thanks!
--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
The Russian Postgres Company