Re: 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

> On 31 Aug 2021, at 16:14, Pavel Luzanov <p(dot)luzanov(at)postgrespro(dot)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

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Sunday, September 5th, 2021 at 21:47, Pavel Luzanov <p(dot)luzanov(at)postgrespro(dot)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(at)pm(dot)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(at)pm(dot)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

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(at)telsasoft(dot)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

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

Pavel Luzanov <p(dot)luzanov(at)postgrespro(dot)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