Re: psql: \dl+ to list large objects privileges

‐‐‐‐‐‐‐ 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