Обсуждение: Usability fail with psql's \dp command

I noticed today that \dp does not distinguish empty acl fields
(meaning nobody has any privileges) from null acl fields
(which mean default privileges, typically not empty).
For instance

regression=# \c joe joe
You are now connected to database "joe" as user "joe".
joe=> create table jt (f1 int);
CREATE TABLE
joe=> \dp
                            Access privileges
 Schema | Name | Type  | Access privileges | Column privileges | Policies 
--------+------+-------+-------------------+-------------------+----------
 public | jt   | table |                   |                   | 
(1 row)

joe=> insert into jt values(1);
INSERT 0 1
joe=> revoke all on table jt from joe;
REVOKE
joe=> \dp
                            Access privileges
 Schema | Name | Type  | Access privileges | Column privileges | Policies 
--------+------+-------+-------------------+-------------------+----------
 public | jt   | table |                   |                   | 
(1 row)

joe=> insert into jt values(1);
ERROR:  permission denied for table jt

So those are definitely different privilege states, but they look
the same.

One idea is to replace a null ACL value with the actual effective
permissions, which we could get from the acldefault() function.
However, acldefault() only exists since 9.2, and in any case
I'm afraid that might be perceived as mostly clutter.

What do people think of printing "Default" if the ACL is null?

Alternatively, since the state with an empty ACL is certainly
the unusual case, maybe we should mark that specially, perhaps
by printing "None" or "No privileges".

(I've not looked at the code to see how hard such changes would be.)

            regards, tom lane

> On Jul 28, 2018, at 11:41, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Alternatively, since the state with an empty ACL is certainly
> the unusual case, maybe we should mark that specially, perhaps
> by printing "None" or "No privileges".

+1 for "No privileges".   I was just bitted by that this week.

--
-- Christophe Pettus
   xof@thebuild.com

On Sat, Jul 28, 2018 at 02:41:24PM -0400, Tom Lane wrote:
> I noticed today that \dp does not distinguish empty acl fields
> (meaning nobody has any privileges) from null acl fields
> (which mean default privileges, typically not empty).
> For instance
> 
> regression=# \c joe joe
> You are now connected to database "joe" as user "joe".
> joe=> create table jt (f1 int);
> CREATE TABLE
> joe=> \dp
>                             Access privileges
>  Schema | Name | Type  | Access privileges | Column privileges | Policies 
> --------+------+-------+-------------------+-------------------+----------
>  public | jt   | table |                   |                   | 
> (1 row)
> 
> joe=> insert into jt values(1);
> INSERT 0 1
> joe=> revoke all on table jt from joe;
> REVOKE
> joe=> \dp
>                             Access privileges
>  Schema | Name | Type  | Access privileges | Column privileges | Policies 
> --------+------+-------+-------------------+-------------------+----------
>  public | jt   | table |                   |                   | 
> (1 row)
> 
> joe=> insert into jt values(1);
> ERROR:  permission denied for table jt
> 
> So those are definitely different privilege states, but they look
> the same.

Please find attached a patch to fix this.  Would this be a
back-patchable bug?

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

>> So those are definitely different privilege states, but they look
>> the same.
>
> Please find attached a patch to fix this.  Would this be a
> back-patchable bug?

My 0.02€: this creates an exception for anyone trying to parse the output.
I would have preferred empty logically meaning no rights, and the default 
being spelled out explicitely.

-- 
Fabien.

On Sat, Jul 28, 2018 at 08:11:17PM -0400, Fabien COELHO wrote:
> 
> >>So those are definitely different privilege states, but they look
> >>the same.
> >
> >Please find attached a patch to fix this.  Would this be a
> >back-patchable bug?
> 
> My 0.02€: this creates an exception for anyone trying to parse the output.
> I would have preferred empty logically meaning no rights, and the default
> being spelled out explicitely.

If we were designing this on a clean sheet of paper, I'd agree that
what you propose is a better UI, but it's a significant change from
what we have now.  People parsing much more common output--this is
pretty corner-case--will have to make changes.  Do you think it's
"better enough" to break compatibility in the way you propose, forcing
it into 12?

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Fabien COELHO <coelho@cri.ensmp.fr> writes:
> My 0.02¤: this creates an exception for anyone trying to parse the output.
> I would have preferred empty logically meaning no rights, and the default 
> being spelled out explicitely.

Uh, who'd be trying to parse the output of \dp?

The reason we provide psql's -E option is so that people can look at the
underlying queries and adapt them to their own purposes.  In many cases,
that would include stripping out provisions that make the output more
human-friendly and less machine-friendly.  (Localization of output words
is one obvious example of things that psql does that are quite
machine-parsing-unfriendly.)  This seems to me to be another such case.

We could certainly consider the explicit-default approach (and it's one
of the options I suggested), but to my mind we should evaluate the
options entirely on what humans find readable, with exactly zero weight
to machine readability.

            regards, tom lane

>> My 0.02¤: this creates an exception for anyone trying to parse the output.
>> I would have preferred empty logically meaning no rights, and the default
>> being spelled out explicitely.
>
> Uh, who'd be trying to parse the output of \dp?

Ok. Maybe humans?

Note that 'No privileges' could be somehow interpreted as "default 
privileges" (no "special/given" privileges) or as "no permissions at all", 
so there is still some ambiguity, at least for me.

> We could certainly consider the explicit-default approach (and it's one
> of the options I suggested), but to my mind we should evaluate the
> options entirely on what humans find readable, with exactly zero weight
> to machine readability.

Ok. So I agree with your suggestion, on the ground of avoiding a special 
output syntax in one particular case if possible.

Attached is a quick and dirty attempt at regenerating default privileges 
from dp query, with an added join on roles & and test on kind.

I'm not 100% sure of the list of privileges for all types, and I do not 
like much having them in a query like that because in the unlikely event 
that a new one is added, the query output suddenly becomes false.

From a programming point of view there is another pain with that approach 
as "describe.c" uses "printACLColumn", but this version would need the 
kind and the owner role as well, and it seems that all 11 instances of 
printACLColumn should be adapted as well.

As for David point of breaking anything from a user perspective, as the 
current output is currently ambiguous thus unreliable/unusable, I think it 
is more a bug fix than anything else.

-- 
Fabien.

Hello.

At Sun, 29 Jul 2018 21:34:29 -0400 (EDT), Fabien COELHO <coelho@cri.ensmp.fr> wrote in
<alpine.DEB.2.21.1807291818460.14827@lancre>
> 
> >> My 0.02¤: this creates an exception for anyone trying to parse the
> >> output.
> >> I would have preferred empty logically meaning no rights, and the
> >> default
> >> being spelled out explicitely.
> >
> > Uh, who'd be trying to parse the output of \dp?
> 
> Ok. Maybe humans?
> 
> Note that 'No privileges' could be somehow interpreted as "default
> privileges" (no "special/given" privileges) or as "no permissions at
> all", so there is still some ambiguity, at least for me.

FWIW "No privileges" seems to me as "The user cannot access it at
all" with no ambiguity.

Currently the behavior is documented here. (This needs to be
edited.)

https://www.postgresql.org/docs/10/static/sql-grant.html

| If the “Access privileges” column is empty for a given object,
| it means the object has default privileges (that is, its
| privileges column is null). Default privileges always include all
| privileges for the owner, and can include some privileges for
| PUBLIC depending on the object type, as explained above. The
| first GRANT or REVOKE on an object will instantiate the default
| privileges (producing, for example, {miriam=arwdDxt/miriam}) and
| then modify them per the specified request.

So it changes the existing documented behavior.

What is most significant to me here is it's confusing that the
empty representation means rather opposite things for
pg_class.relacl and the correspondent in \dp's output.

  relacl | Access privileges
 --------+------------------
  (null) | joe=arwdDxt/joe
  {}     | (null)

> > We could certainly consider the explicit-default approach (and it's
> > one
> > of the options I suggested), but to my mind we should evaluate the
> > options entirely on what humans find readable, with exactly zero
> > weight
> > to machine readability.
> 
> Ok. So I agree with your suggestion, on the ground of avoiding a
> special output syntax in one particular case if possible.

\dp is a convenient shortcut for users so the output should be
intuitive or easy-to-grasp. If we wanted to use the output as a
input of other programs, we are to use bare tables.. maybe, or
should handle the special indications.  But, if we were to change
the documented behavior, I'd propose the following.

  relacl | Access privileges
 --------+------------------
  (null) | '(default)'
  {}     | '(no privilege)'

The parentheses ('()') can be '<>' as pg_stat_activity uses for a
content with a special meaning. (<insufficient privilege>).  Also
I found that \dC shows '(binary coercible)' when the cast is a
relabel. So I'm not confident on whether to use but I'd like to
choose '()' for them. Both are not "parsable" for... a human?

> Attached is a quick and dirty attempt at regenerating default
> privileges from dp query, with an added join on roles & and test on
> kind.
> 
> I'm not 100% sure of the list of privileges for all types, and I do
> not like much having them in a query like that because in the unlikely
> event that a new one is added, the query output suddenly becomes
> false.
> 
> From a programming point of view there is another pain with that
> approach as "describe.c" uses "printACLColumn", but this version would
> need the kind and the owner role as well, and it seems that all 11
> instances of printACLColumn should be adapted as well.
> 
> As for David point of breaking anything from a user perspective, as
> the current output is currently ambiguous thus unreliable/unusable, I
> think it is more a bug fix than anything else.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello Kyotaro-san,

>> Note that 'No privileges' could be somehow interpreted as "default
>> privileges" (no "special/given" privileges) or as "no permissions at
>> all", so there is still some ambiguity, at least for me.
>
> FWIW "No privileges" seems to me as "The user cannot access it at
> all" with no ambiguity.

Ok. For me '' and 'No privileges' still looks like they mean the same, 
whereas the point of the patch is to solve the ambiguity.

> Currently the behavior is documented here. (This needs to be
> edited.)
>
> https://www.postgresql.org/docs/10/static/sql-grant.html

Sure, but user friendlyness would suggest that the output should not be 
misleading from the start.

> So it changes the existing documented behavior.

Sure. The behavior is misleading, and documentation is of little help in 
such a case.

> \dp is a convenient shortcut for users so the output should be
> intuitive or easy-to-grasp.

Yes!

> [...]
>
>  relacl | Access privileges
> --------+------------------
>  (null) | '(default)'
>  {}     | '(no privilege)'

This suggestion is better as it avoids the "empty/no" ambiguity. It breaks 
the documented behavior, but I'm fine with that anyway.

The human I am now has to know what "default" permissions are depending on 
the kind of object, where it could have said it to me directly. Moreover, 
the line is not self-contained because the default permission depends on 
the owner, but "\dp" does not tell who the owner is, which is another 
annoyance.

A benefit of your approach is that its coding is easy because it does not 
have to fetch the owner tuple and reconstruct the default perms depending 
on the kind of object.

A cleaner approach would be to have a NOT NULL column and have the default 
always explicit, instead of having a lazy instantiation of the field 
managed on GRANT/REVOKE but not on initialization. However this is 
probably too big a change for the problem at hand, and it would not solve 
the ambiguity issue for previous versions.

-- 
Fabien.

On Sat, Jul 28, 2018 at 4:36 PM, David Fetter <david@fetter.org> wrote:
> Please find attached a patch to fix this.  Would this be a
> back-patchable bug?

In my view, this is not a bug fix, but an improvement, and therefore
should not be back-patched.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

I noticed today that \dp does not distinguish empty acl fields
(meaning nobody has any privileges) from null acl fields
(which mean default privileges, typically not empty).

One idea is to replace a null ACL value with the actual effective
permissions, which we could get from the acldefault() function.

-----
Pavel Luzanov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

> One idea is to replace a null ACL value with the actual effective
> permissions, which we could get from the acldefault() function.
> However, acldefault() only exists since 9.2, and in any case
> I'm afraid that might be perceived as mostly clutter.

Here is an poc implementation of this which does not rely on 
"acldefault()". Cannot say I'm thrilled, but it is not too bad either. I 
worked around \ddp with a recursion.

Writing this piece code makes me realize once again the abysmal coverage 
of psql non-regression tests, where basically no \d* functions are tested. 
I think at least this part could be salvage from the patch.

Another benefit of expliciting the defaults is that the documentation can 
be made more straightforward: shown permissions means what you can see, 
which simplifies the description.

Another implementation approach could be to use acldefault() from 9.2, and 
just display (no privileges) and (default privileges) before, as suggested 
on the thread. This would probably help simplify the code.

I find that having "default privileges" written as not very helpful, 
because you have to remember them. Ok, it is all perms for the owner, but 
then there are some objects which also have some permissions to public, 
and do not memorize these exception. Also, some \d* (eg \dT \dD) do not 
display the owner.

I do not perceive as "clutter" the fact that a column advertising "Access 
privileges" provides the access privileges... Mostly it is shown only 
under "+";  For \dp, you ask for it, you get it.

-- 
Fabien.

Hello Pavel,

>> I noticed today that \dp does not distinguish empty acl fields
>> (meaning nobody has any privileges) from null acl fields
>> (which mean default privileges, typically not empty).
>
> This confusing behavior exists not only for \dp command.
> Consider schemas and \dn+ command:

Indeed, all \d* which display perms have the empty/default confusion:

   \dp \ddp \des \dew \l \dn \db \df \dT \dD \dL

I fixed them all to display the default acl in the patch I just sent.

I also noticed that although large objects have permissions, they are not 
printed by any backslash commands.

-- 
Fabien.

Indeed, all \d* which display perms have the empty/default confusion:

  \dp \ddp \des \dew \l \dn \db \df \dT \dD \dL

I fixed them all to display the default acl in the patch I just sent.

I also noticed that although large objects have permissions, they are not printed by any backslash commands.

-----
Pavel Luzanov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company