Thread: 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
Attachment
> 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] 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
Attachment
‐‐‐‐‐‐‐ 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
>
> [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 \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 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]. 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
Attachment
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
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, passed Hi, 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
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 */
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
Attachment
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