Thread: psql: Add role's membership options to the \du+ command

When you include one role in another, you can specify three options:
ADMIN, INHERIT (added in e3ce2de0) and SET (3d14e171).

For example.

CREATE ROLE alice LOGIN;

GRANT pg_read_all_settings TO alice WITH ADMIN TRUE, INHERIT TRUE, SET TRUE;
GRANT pg_stat_scan_tables TO alice WITH ADMIN FALSE, INHERIT FALSE, SET 
FALSE;
GRANT pg_read_all_stats TO alice WITH ADMIN FALSE, INHERIT TRUE, SET FALSE;

For information about the options, you need to look in the pg_auth_members:

SELECT roleid::regrole, admin_option, inherit_option, set_option
FROM pg_auth_members
WHERE member = 'alice'::regrole;
         roleid        | admin_option | inherit_option | set_option
----------------------+--------------+----------------+------------
  pg_read_all_settings | t            | t              | t
  pg_stat_scan_tables  | f            | f              | f
  pg_read_all_stats    | f            | t              | f
(3 rows)

I think it would be useful to be able to get this information with a 
psql command
like \du (and \dg). With proposed patch the \du command still only lists
the roles of which alice is a member:

\du alice
                                      List of roles
  Role name | Attributes |                          Member of
-----------+------------+--------------------------------------------------------------
  alice     |            | 
{pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables}

But the \du+ command adds information about the selected ADMIN, INHERIT
and SET options:

\du+ alice
                                     List of roles
  Role name | Attributes |                   Member of                   
| Description
-----------+------------+-----------------------------------------------+-------------
  alice     |            | pg_read_all_settings WITH ADMIN, INHERIT, SET+|
            |            | pg_read_all_stats WITH INHERIT               +|
            |            | pg_stat_scan_tables                           |

One more change. The roles in the "Member of" column are sorted for both
\du+ and \du for consistent output.

Any comments are welcome.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Added the patch to the open commitfest:
https://commitfest.postgresql.org/42/4116/

Feel free to reject if it's not interesting.

-- 
Pavel Luzanov

Yeah, I noticed the lack too, then went a bit too far afield with trying to compose a graph of the roles.  I'm still working on that but at this point it probably won't be something I try to get committed to psql.  Something more limited like this does need to be included.

I'm not too keen on the idea of converting the existing array into a newline separated string.  I would try hard to make the modification here purely additional.

Another thing I did with the graph was have both "member" and "memberof" columns in the output.  In short, every grant row in pg_auth_members appears twice, once in each column, so the role being granted membership and the role into which membership is granted both have visibility when you filter on them.  For the role graph I took this idea and extended out to an entire chain of roles (and also broke out user and group separately) but I think doing the direct-grant only here would still be a big improvement.

-- 
Pavel Luzanov

Thanks a lot for the improvement, and it will definitely help provide 
more very useful information.

I noticed the document psql-ref.sgml has been updated for both `du+` and 
`dg+`, but only `du` and `\du+` are covered in regression test. Is that 
because `dg+` is treated exactly the same as `du+` from testing point of 
view?

The reason I am asking this question is that I notice that `pg_monitor` 
also has the detailed information, so not sure if more test cases required.

postgres=# \duS+
List of roles
           Role name          | Attributes                         
|                   Member of                   | Description

-----------------------------+------------------------------------------------------------+-----------------------------------------------+-------------
  alice |                                                            | 
pg_read_all_settings WITH ADMIN, INHERIT, SET |
  pg_checkpoint               | Cannot login 
|                                               |
  pg_database_owner           | Cannot login 
|                                               |
  pg_execute_server_program   | Cannot login 
|                                               |
  pg_maintain                 | Cannot login 
|                                               |
  pg_monitor                  | Cannot 
login                                               | 
pg_read_all_settings WITH INHERIT, SET       +|
|                                                            | 
pg_read_all_stats WITH INHERIT, SET          +|
|                                                            | 
pg_stat_scan_tables WITH INHERIT, SET         |

Best regards,

David

On 2023-01-09 8:09 a.m., Pavel Luzanov wrote:
> When you include one role in another, you can specify three options:
> ADMIN, INHERIT (added in e3ce2de0) and SET (3d14e171).
>
> For example.
>
> CREATE ROLE alice LOGIN;
>
> GRANT pg_read_all_settings TO alice WITH ADMIN TRUE, INHERIT TRUE, SET 
> TRUE;
> GRANT pg_stat_scan_tables TO alice WITH ADMIN FALSE, INHERIT FALSE, 
> SET FALSE;
> GRANT pg_read_all_stats TO alice WITH ADMIN FALSE, INHERIT TRUE, SET 
> FALSE;
>
> For information about the options, you need to look in the 
> pg_auth_members:
>
> SELECT roleid::regrole, admin_option, inherit_option, set_option
> FROM pg_auth_members
> WHERE member = 'alice'::regrole;
>         roleid        | admin_option | inherit_option | set_option
> ----------------------+--------------+----------------+------------
>  pg_read_all_settings | t            | t              | t
>  pg_stat_scan_tables  | f            | f              | f
>  pg_read_all_stats    | f            | t              | f
> (3 rows)
>
> I think it would be useful to be able to get this information with a 
> psql command
> like \du (and \dg). With proposed patch the \du command still only lists
> the roles of which alice is a member:
>
> \du alice
>                                      List of roles
>  Role name | Attributes |                          Member of
> -----------+------------+-------------------------------------------------------------- 
>
>  alice     |            | 
> {pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables}
>
> But the \du+ command adds information about the selected ADMIN, INHERIT
> and SET options:
>
> \du+ alice
>                                     List of roles
>  Role name | Attributes |                   Member 
> of                   | Description
> -----------+------------+-----------------------------------------------+------------- 
>
>  alice     |            | pg_read_all_settings WITH ADMIN, INHERIT, SET+|
>            |            | pg_read_all_stats WITH INHERIT               +|
>            |            | pg_stat_scan_tables                           |
>
> One more change. The roles in the "Member of" column are sorted for both
> \du+ and \du for consistent output.
>
> Any comments are welcome.
>

On Fri, Feb 10, 2023 at 2:08 PM David Zhang <david.zhang@highgo.ca> wrote:

I noticed the document psql-ref.sgml has been updated for both `du+` and
`dg+`, but only `du` and `\du+` are covered in regression test. Is that
because `dg+` is treated exactly the same as `du+` from testing point of
view?

Yes.

The reason I am asking this question is that I notice that `pg_monitor`
also has the detailed information, so not sure if more test cases required.

Of course it does.  Why does that bother you?  And what does that have to do with the previous paragraph?

There is a default built-in role `pg_monitor` and the behavior changed after the patch. If `\dg+` and `\du+` is treated as the same, and `make check` all pass, then I assume there is no test case to verify the output of `duS+`. My point is should we consider add a test case?

Before patch the output for `pg_monitor`,

postgres=# \duS+
                                                                             List of roles
          Role name          |                         Attributes                         |                          Member of                           | Description
-----------------------------+------------------------------------------------------------+--------------------------------------------------------------+-------------
 alice                       |                                                            | {pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables} |
 pg_checkpoint               | Cannot login                                               | {}                                                           |
 pg_database_owner           | Cannot login                                               | {}                                                           |
 pg_execute_server_program   | Cannot login                                               | {}                                                           |
 pg_maintain                 | Cannot login                                               | {}                                                           |
 pg_monitor                  | Cannot login                                               | {pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables} |
 pg_read_all_data            | Cannot login                                               | {}                                                           |
 pg_read_all_settings        | Cannot login                                               | {}                                                           |
 pg_read_all_stats           | Cannot login                                               | {}                                                           |
 pg_read_server_files        | Cannot login                                               | {}                                                           |
 pg_signal_backend           | Cannot login                                               | {}                                                           |
 pg_stat_scan_tables         | Cannot login                                               | {}                                                           |
 pg_use_reserved_connections | Cannot login                                               | {}                                                           |
 pg_write_all_data           | Cannot login                                               | {}                                                           |
 pg_write_server_files       | Cannot login                                               | {}                                                           |
 ubuntu                      | Superuser, Create role, Create DB, Replication, Bypass RLS | {}                                                           |

After patch the output for `pg_monitor`,

postgres=# \duS+
                                                                     List of roles
          Role name          |                         Attributes                         |                   Member of                   | Description
-----------------------------+------------------------------------------------------------+-----------------------------------------------+-------------
 alice                       |                                                            | pg_read_all_settings WITH ADMIN, INHERIT, SET+|
                             |                                                            | pg_read_all_stats WITH INHERIT               +|
                             |                                                            | pg_stat_scan_tables                           |
 pg_checkpoint               | Cannot login                                               |                                               |
 pg_database_owner           | Cannot login                                               |                                               |
 pg_execute_server_program   | Cannot login                                               |                                               |
 pg_maintain                 | Cannot login                                               |                                               |
 pg_monitor                  | Cannot login                                               | pg_read_all_settings WITH INHERIT, SET       +|
                             |                                                            | pg_read_all_stats WITH INHERIT, SET          +|
                             |                                                            | pg_stat_scan_tables WITH INHERIT, SET         |
 pg_read_all_data            | Cannot login                                               |                                               |
 pg_read_all_settings        | Cannot login                                               |                                               |
 pg_read_all_stats           | Cannot login                                               |                                               |
 pg_read_server_files        | Cannot login                                               |                                               |
 pg_signal_backend           | Cannot login                                               |                                               |
 pg_stat_scan_tables         | Cannot login                                               |                                               |
 pg_use_reserved_connections | Cannot login                                               |                                               |
 pg_write_all_data           | Cannot login                                               |                                               |
 pg_write_server_files       | Cannot login                                               |                                               |
 ubuntu                      | Superuser, Create role, Create DB, Replication, Bypass RLS |                                               |

Best regards,

David

David J.

On 2023-02-15 1:37 p.m., David G. Johnston wrote:

On Wed, Feb 15, 2023 at 2:31 PM David Zhang <david.zhang@highgo.ca> wrote:

There is a default built-in role `pg_monitor` and the behavior changed after the patch. If `\dg+` and `\du+` is treated as the same, and `make check` all pass, then I assume there is no test case to verify the output of `duS+`. My point is should we consider add a test case?

 

I mean, either you accept the change in how this meta-command presents its information or you don't.  I don't see how a test case is particularly beneficial.  Or, at least the pg_monitor role is not special in this regard.  Alice changed too and you don't seem to be including it in your complaint.

I mean, either you accept the change in how this meta-command presents its information or you don't.

Yes, that's the main issue of this patch.

On the one hand, it would be nice to see the membership options with the psql command.

On the other hand, I don't have an exact understanding of how best to do it. That's why I proposed a variant for discussion. It is quite possible that if there is no consensus, it would be better to leave it as is, and get information by queries to the system catalog.

-----
Pavel Luzanov

On the one hand, it would be nice to see the membership options with the psql command.

After playing with cf5eb37c and e5b8a4c0 I think something must be made with \du command.postgres@demo(16.0)=# CREATE ROLE admin LOGIN CREATEROLE;
CREATE ROLE
postgres@demo(16.0)=# \c - admin
You are now connected to database "demo" as user "admin".
admin@demo(16.0)=> SET createrole_self_grant = 'SET, INHERIT';
SET
admin@demo(16.0)=> CREATE ROLE bob LOGIN;
CREATE ROLE
admin@demo(16.0)=> \du

                                   List of roles
 Role name |                         Attributes                         | Member of
-----------+------------------------------------------------------------+-----------
 admin     | Create role                                                | {bob,bob}
 bob       |                                                            | {}
 postgres  | Superuser, Create role, Create DB, Replication, Bypass RLS | {}

We see two bob roles in the 'Member of' column. Strange? But this is correct.

admin@demo(16.0)=> select roleid::regrole, member::regrole, * from pg_auth_members where roleid = 'bob'::regrole;
 roleid | member |  oid  | roleid | member | grantor | admin_option | inherit_option | set_option
--------+--------+-------+--------+--------+---------+--------------+----------------+------------
 bob    | admin  | 16713 |  16712 |  16711 |      10 | t            | f              | f
 bob    | admin  | 16714 |  16712 |  16711 |   16711 | f            | t              | t
(2 rows)

First 'grant bob to admin' command issued immediately after creating role bob by superuser(grantor=10). Second command issues by admin role and set membership options SET and INHERIT.If we don't ready to display membership options with \du+ may be at least we must group records in 'Member of' column for \du command?

-----
Pavel Luzanov

On Fri, Feb 17, 2023 at 4:02 AM Pavel Luzanov <p.luzanov@postgrespro.ru> wrote:

                                   List of roles
 Role name |                         Attributes                         | Member of
-----------+------------------------------------------------------------+-----------
 admin     | Create role                                                | {bob,bob}
 bob       |                                                            | {}
 postgres  | Superuser, Create role, Create DB, Replication, Bypass RLS | {}

First 'grant bob to admin' command issued immediately after creating role bob by superuser(grantor=10). Second command issues by admin role and set membership options SET and INHERIT.If we don't ready to display membership options with \du+ may be at least we must group records in 'Member of' column for \du command?

I agree that these views should GROUP BY roleid and use bool_or(*_option) to produce their result. 

Their purpose is to communicate the current effective state to the user, not facilitate full inspection of the configuration, possibly to aid in issuing GRANT and REVOKE commands.

One thing I found, and I plan to bring this up independently once I've collected my thoughts, is that pg_has_role() uses the terminology "USAGE" and "MEMBER" for "INHERIT" and "SET" respectively.

It's annoying that "member" has been overloaded here.  And the choice of USAGE just seems arbitrary (though I haven't researched it) given the related syntax.

https://www.postgresql.org/docs/15/functions-info.html

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

This is the format I've gone for (more-or-less) in my RoleGraph view (I'll be sharing it publicly in the near future).

bob from grantor (a, s, i) \n

adam from postgres (a, s, i) \n

emily from postgres (empty)

Based upon prior comments going for something like the following is undesirable:  bob=asi/grantor

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Next version (v3) addresses complains from cfbot. Changed only tests.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

I didn't even know this function existed. But I see that it was changed in 3d14e171 with updated documentation:

https://www.postgresql.org/docs/devel/functions-info.html#FUNCTIONS-INFO-ACCESS
Maybe that's enough.

I think that should probably have ADMIN as one of the options as well.  Also curious what it reports for an empty membership.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

I'd be fine with "pg_can_admin_role" being a newly created function that provides this true/false answer but it seems indisputable that today there is no core-provided means to answer the question "can one role get ADMIN rights on another role".  Modifying \du to show this seems out-of-scope but the pg_has_role function already provides that question for INHERIT and SET so it is at least plausible to extend it to include ADMIN, even if the phrase "has role" seems a bit of a misnomer.  I do cover this aspect with the Role Graph pseudo-extension but given the presence and ease-of-use of a boolean-returning function this seems like a natural addition.  We've also survived quite long without it - this isn't a new concept in v16, just a bit refined.

I wouldn't bother starting yet another thread in this area right now, this one can absorb some related changes as well as the subject line item.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Moving the goal posts for this meta-command to >= 9.5 seems like it should be done as a separate patch and thread.  The documentation presently states we are targeting 9.2 and newer.

My suggestion for the docs is below.

+        <para>
+        Shown within each row, in newline-separated format, are the memberships granted to
+        the role.  The presentation includes both the name of the grantor
+        as well as the membership permissions (in an abbreviated format:
+        <literal>a</literal> for admin option, <literal>i</literal> for inherit option,
+        <literal>s</literal> for set option.) The word <literal>empty</literal> is printed in
+        the case that none of those permissions are granted.
+        See the <link linkend="sql-grant"><command>GRANT</command></link> command for their meaning.
+        </para>
+        <para>
+        If the form <literal>\dg+</literal> is used the comment attached to the role is shown.
         </para>

I would suggest tweaking the test output to include regress_du_admin and also to make regress_du_admin a CREATEROLE role with LOGIN.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

FWIW I've finally gotten to publishing my beta version of the Role Graph for PostgreSQL pseudo-extension I'd been talking about:

https://github.com/polobo/RoleGraphForPostgreSQL

I handle EMPTY explicitly in the Role Graph but as I noted somewhere in my comments, it really shouldn't be possible to leave the database in that state.  Do we need to bug Robert on this directly or do you plan to have a go at it?

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

I missed the comment at the beginning of the file about version 9.2. I will return the version check for rolbypassrls.

+        <para>
+        Shown within each row, in newline-separated format, are the memberships granted to
+        the role.  The presentation includes both the name of the grantor
+        as well as the membership permissions (in an abbreviated format:
+        <literal>a</literal> for admin option, <literal>i</literal> for inherit option,
+        <literal>s</literal> for set option.) The word <literal>empty</literal> is printed in
+        the case that none of those permissions are granted.
+        See the <link linkend="sql-grant"><command>GRANT</command></link> command for their meaning.
+        </para>
+        <para>
+        If the form <literal>\dg+</literal> is used the comment attached to the role is shown.
         </para>

Thanks. I will replace the description with this one.

I would suggest tweaking the test output to include regress_du_admin and also to make regress_du_admin a CREATEROLE role with LOGIN.

Ok.

Please review the attached version 4 with the changes discussed.

-----
Pavel Luzanov

I would suggest tweaking the test output to include regress_du_admin ...

I found (with a help of cfbot) difficulty with this. The problem is the bootstrap superuser name (oid=10). 
This name depends on the OS username. In my case it's pal, but in most cases it's postgres or something else. 
And the output of \du regress_du_admin can't be predicted:

\du regress_du_admin                            List of roles    Role name     | Attributes  |              Member of              
------------------+-------------+------------------------------------- regress_du_admin | Create role | regress_du_role0 from pal (a, i, s)+                  |             | regress_du_role1 from pal (a, i, s)+                  |             | regress_du_role2 from pal (a, i, s)


So, I decided not to include regress_du_admin in the test output.

Please, see version 5 attached. Only tests changed.

-----
Pavel Luzanov

In the previous version, I didn't notice (unlike cfbot) the compiler 
warning. Fixed in version 6.

-----
Pavel Luzanov

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> I've marked this Ready for Committer.

Hmm ... not sure I like the proposed output.  The 'a', 'i', 's'
annotations are short but they don't have much else to recommend them.
On the other hand, there's nearby precedent for single-letter
abbreviations in ACL displays.  Nobody particularly likes those,
though.  Also, if we're modeling this on ACLs then the display
could be further shortened to "(ais)" or the like.

Also, the patch is ignoring i18n issues.  I suppose if we stick with
said single-letter abbreviations we'd not translate them, but the
construction "rolename from rolename" ought to be translatable IMO.
Perhaps it'd be enough to allow replacement of "from", but I wonder
if the phrase order would need to be different in some languages?

            regards, tom lane

On Tue, Apr 4, 2023 at 12:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Hmm ... not sure I like the proposed output.  The 'a', 'i', 's'
> annotations are short but they don't have much else to recommend them.

Yeah, I don't like that, either.

I'm not sure what the right thing to do is here. It's a problem to
have new information in the catalogs that you can't view via
\d<whatever>. But displaying that information as a string of
characters that will be gibberish to anyone but an expert doesn't
necessarily seem like it really solves the problem. However, if we
spell out the words, then it gets bulky. But maybe bulky is better
than incomprehensible.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> I'm not sure what the right thing to do is here. It's a problem to
> have new information in the catalogs that you can't view via
> \d<whatever>. But displaying that information as a string of
> characters that will be gibberish to anyone but an expert doesn't
> necessarily seem like it really solves the problem. However, if we
> spell out the words, then it gets bulky. But maybe bulky is better
> than incomprehensible.

The existing precedent in \du definitely leans towards "bulky":

regression=# \du
                                   List of roles
 Role name |                         Attributes                         | Member of
-----------+------------------------------------------------------------+-----------
 alice     | Cannot login                                               | {bob}
 bob       | Cannot login                                               | {}
 postgres  | Superuser, Create role, Create DB, Replication, Bypass RLS | {}

It seems pretty inconsistent to me to treat the role attributes this
way and then economize in the adjacent column.

Another advantage to spelling out the SQL keywords is that it removes
the question of whether we should translate them.

I wonder if, while we're here, we should apply the idea of
joining-with-newlines-not-commas to the attributes column too.
That's another source of inconsistency in the proposed display.

            regards, tom lane

On Tue, Apr 4, 2023 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wonder if, while we're here, we should apply the idea of
> joining-with-newlines-not-commas to the attributes column too.
> That's another source of inconsistency in the proposed display.

That would make the column narrower, which might be good, because it
seems to me that listing the memberships could require quite a lot of
space, both vertical and horizontal.

There can be any number of memberships, and each of those memberships
has a grantor and three flag bits (INHERIT, SET, ADMIN). If some user
with a long username has been granted membership with all three of
those flags by a grantor who also has a long username, and if we show
all that information, we're going to use up a lot of horizontal space.
And if there are many such grants, also a lot of vertical space.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 4, 2023 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I wonder if, while we're here, we should apply the idea of
>> joining-with-newlines-not-commas to the attributes column too.

> That would make the column narrower, which might be good, because it
> seems to me that listing the memberships could require quite a lot of
> space, both vertical and horizontal.

Right, that's what I was thinking.

> There can be any number of memberships, and each of those memberships
> has a grantor and three flag bits (INHERIT, SET, ADMIN). If some user
> with a long username has been granted membership with all three of
> those flags by a grantor who also has a long username, and if we show
> all that information, we're going to use up a lot of horizontal space.
> And if there are many such grants, also a lot of vertical space.

Yup --- and if you were incautious enough to not exclude the bootstrap
superuser, then you'll also have a very wide Attributes column.  We
can buy back some of that by joining the attributes with newlines.
At some point people are going to have to resort to \x mode for this
display, but we should do what we can to put that off as long as we're
not sacrificing readability.

            regards, tom lane

On Tue, Apr 4, 2023 at 3:02 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> So, by way of example:
>
> regress_du_role1 | cannot login | regress_du_role0 granted by regress_du_admin with admin, inherit, set | Description
forregress_du_role1 
>
> ~140 character width with description

That seems wider than necessary. Why not have the third column be
something like regress_du_role0 by regress_du_admin (admin, inherit,
set)?

--
Robert Haas
EDB: http://www.enterprisedb.com

On 04.04.2023 23:00, Robert Haas wrote:
> On Tue, Apr 4, 2023 at 3:02 PM David G. Johnston
> <david.g.johnston@gmail.com> wrote:
>> So, by way of example:
>>
>> regress_du_role1 | cannot login | regress_du_role0 granted by regress_du_admin with admin, inherit, set |
Descriptionfor regress_du_role1
 
>>
>>
>> That seems wider than necessary. Why not have the third column be
>> something like regress_du_role0 by regress_du_admin (admin, inherit,
>> set)?

'granted by' can be left without translation, but just 'by' required 
translation, as I think.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

On Tue, Apr 4, 2023 at 10:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 4, 2023 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I wonder if, while we're here, we should apply the idea of
>> joining-with-newlines-not-commas to the attributes column too.

> That would make the column narrower, which might be good, because it
> seems to me that listing the memberships could require quite a lot of
> space, both vertical and horizontal.

Right, that's what I was thinking.

So, by way of example:

regress_du_role1 | cannot login | regress_du_role0 granted by regress_du_admin with admin, inherit, set | Description for regress_du_role1

No translations, all words are either identical to syntax or identifiers.

I'm on board with newlines in the attributes field.

The specific member of column changes are:

from -> granted by

( ) -> "with"

ais -> admin, inherit, set

I'm good with any or all of those selections, either as-is or in the more verbose form.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Pavel Luzanov <p.luzanov@postgrespro.ru> writes:
> What if this long output will be available only for \du+, and for \du 
> just show distinct (without duplicates)
> roles in the current array format? For those, who don't care about these 
> new membership options, nothing will change.
> Those, who need details will use the + modifier.
> ?

I kind of like that.  Would we change to newlines in the Attributes
field in both \du and \du+?  (I'm +1 for that, but maybe others aren't.)

            regards, tom lane

On Wed, Apr 5, 2023 at 6:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Pavel Luzanov <p.luzanov@postgrespro.ru> writes:
> What if this long output will be available only for \du+, and for \du
> just show distinct (without duplicates)
> roles in the current array format? For those, who don't care about these
> new membership options, nothing will change.
> Those, who need details will use the + modifier.
> ?

I kind of like that.  Would we change to newlines in the Attributes
field in both \du and \du+?  (I'm +1 for that, but maybe others aren't.)

If we don't change the \du "Member of" column display (aside from removing duplicates) I'm disinclined to change the Attributes column.

I too am partial to only exposing this detail on the extended (+) display.

David J.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Sorry for joining in late..

At Thu, 13 Apr 2023 15:44:20 +0300, Pavel Luzanov <p.luzanov@postgrespro.ru> wrote in 
> After playing with the \du command, I found that we can't avoid
> translation.
> All attributes are translatable. Also, two of nine attributes shows in
> new line separated format (connection limit and password valid until).

Going a bit off-topic here, but I'd like the "infinity" to be
translatable...

As David-G appears to express concern in upthread, I don't think a
direct Japanese translation from "rolename from rolename" works well,
as the "from" needs accompanying verb. I, as a Japanese speaker, I
prefer a more non-sentence-like notation, similar to David's
suggestion but with slight differences:

"pg_read_all_stats (grantor: postgres, inherit, set)"

This is easily translated into Japanese.

"pg_read_all_stats (付与者: postgres、継承、設定)"

Come to think of this, I recalled a past discussion in which we
concluded that translating punctuation marks appearing between a
variable number of items within list expressions should be avoided...

Thus, I'd like to propose to use an ACL-like notation, which doesn't
need translation.

..|                               Mamber of                               |
..| pg_read_server_files=ais/horiguti,pg_execute_server_program=/postgres | 

If we'd like, but not likely, we might want to provide a parallel
function to aclexplode for this notation.

=# select memberofexplode('pg_read_server_files=ais/horiguti,pg_execute_server_program=/postgres');
   privilege               | grantor  | admin | inherit | set
---------------------------+----------+-------+---------+-------
pg_read_server_files       | horiguti | true  | true    | true
pg_execute_server_programs | postgres | false | false   | false

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 14.04.2023 10:28, Kyotaro Horiguchi wrote:
> As David-G appears to express concern in upthread, I don't think a
> direct Japanese translation from "rolename from rolename" works well,
> as the "from" needs accompanying verb. I, as a Japanese speaker, I
> prefer a more non-sentence-like notation, similar to David's
> suggestion but with slight differences:
>
> "pg_read_all_stats (grantor: postgres, inherit, set)"

In this form, it confuses me that 'postgres' and 'inherit, set' look 
like a common list.

> Come to think of this, I recalled a past discussion in which we
> concluded that translating punctuation marks appearing between a
> variable number of items within list expressions should be avoided...
>
> Thus, I'd like to propose to use an ACL-like notation, which doesn't
> need translation.
>
> ..|                               Mamber of                               |
> ..| pg_read_server_files=ais/horiguti,pg_execute_server_program=/postgres |

It's very tempting to do so. But I don't like this approach. Showing 
membership options as an ACL-like column will be confusing.
Even in your example, my first reaction is that 
pg_execute_server_program is available to public.
(As for the general patterns, we can also consider combining three 
options into one column, like pg_class.reloptions.)

So, yet another way to discuss:

pg_read_all_stats(inherit,set/horiguti)
pg_execute_server_program(empty/postgres)


One more point. Grants without any option probably should be prohibited 
as useless. But this is for a new thread.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

On 4/13/23 8:44 AM, Pavel Luzanov wrote:

> P.S. If no objections I plan to add this patch to Open Items for v16
> https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items

[RMT hat]

I don't see why this is an open item as this feature was not committed 
for v16. Open items typically revolve around committed features.

Unless someone makes a convincing argument otherwise, I'll remove this 
from the Open Items list[1] tomorrow.

Thanks,

Jonathan

[1] https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Wed, May 3, 2023 at 9:00 AM Jonathan S. Katz <jkatz@postgresql.org>
> wrote:
>> I don't see why this is an open item as this feature was not committed
>> for v16. Open items typically revolve around committed features.

> The argument is that updating the psql \d views to show the newly added
> options is something that the patch to add those options should have done
> before being committed.

Yeah, if there is not any convenient way to see that info in psql
then that seems like a missing part of the feature.

            regards, tom lane

On 5/3/23 12:25 PM, Tom Lane wrote:
> "David G. Johnston" <david.g.johnston@gmail.com> writes:
>> On Wed, May 3, 2023 at 9:00 AM Jonathan S. Katz <jkatz@postgresql.org>
>> wrote:
>>> I don't see why this is an open item as this feature was not committed
>>> for v16. Open items typically revolve around committed features.
> 
>> The argument is that updating the psql \d views to show the newly added
>> options is something that the patch to add those options should have done
>> before being committed.
> 
> Yeah, if there is not any convenient way to see that info in psql
> then that seems like a missing part of the feature.

[RMT hat]

OK -- I was rereading the thread again to see if I could glean that 
insight. There was a comment buried in the thread with David's opinion 
on that front, but no one had +1'd that.

However, if this is for feature completeness, I'll withdraw the closing 
of the open item, but would strongly suggest we complete it in time for 
Beta 1.

[Personal hat]

Looking at Pavel's latest patch, I do find the output easy to 
understand, though do we need to explicitly list "empty" if there are no 
membership permissions?

Thanks,

Jonathan

But if it is really a blocker then maybe we should produce 3 separate newline separated columns, one for the member of role, one for the list of attributes, and one with the grantor.  The column headers can be translated more easily as single nouns.  The readability quite probably would end up being equivalent (maybe even better) in tabular form instead of sentence form.

Just to visualize this approach.
Below are the output for the tabular form and the sentence form from last patch version
(sql script attached):

Tabular form
     rolname      |     memberof     |       options       |     grantor      
------------------+------------------+---------------------+------------------
 postgres         |                  |                     | 
 regress_du_admin | regress_du_role0+| admin, inherit, set+| postgres        +
                  | regress_du_role1+| admin, inherit, set+| postgres        +
                  | regress_du_role2 | admin, inherit, set | postgres
 regress_du_role0 |                  |                     | 
 regress_du_role1 | regress_du_role0+| admin, inherit, set+| regress_du_admin+
                  | regress_du_role0+| inherit            +| regress_du_role1+
                  | regress_du_role0 | set                 | regress_du_role2
 regress_du_role2 | regress_du_role0+| admin              +| regress_du_admin+
                  | regress_du_role0+| inherit, set       +| regress_du_role1+
                  | regress_du_role0+| empty              +| regress_du_role2+
                  | regress_du_role1 | admin, set          | regress_du_admin
(5 rows)

Sentence form from patch v7
     rolname      |                           memberof                           
------------------+--------------------------------------------------------------
 postgres         | 
 regress_du_admin | regress_du_role0 from postgres (admin, inherit, set)        +
                  | regress_du_role1 from postgres (admin, inherit, set)        +
                  | regress_du_role2 from postgres (admin, inherit, set)
 regress_du_role0 | 
 regress_du_role1 | regress_du_role0 from regress_du_admin (admin, inherit, set)+
                  | regress_du_role0 from regress_du_role1 (inherit)            +
                  | regress_du_role0 from regress_du_role2 (set)
 regress_du_role2 | regress_du_role0 from regress_du_admin (admin)              +
                  | regress_du_role0 from regress_du_role1 (inherit, set)       +
                  | regress_du_role0 from regress_du_role2 (empty)              +
                  | regress_du_role1 from regress_du_admin (admin, set)
(5 rows)
 

The tabular form solves the latest patch translation problems mentioned by Kyotaro.
But it requires mapping elements between 3 array-like columns.

To move forward, needs more opinions?

 
-----
Pavel Luzanov

On 5/7/23 3:14 PM, Pavel Luzanov wrote:
> On 05.05.2023 19:51, David G. Johnston wrote:
>> But if it is really a blocker then maybe we should produce 3 separate 
>> newline separated columns, one for the member of role, one for the 
>> list of attributes, and one with the grantor.  The column headers can 
>> be translated more easily as single nouns.  The readability quite 
>> probably would end up being equivalent (maybe even better) in tabular 
>> form instead of sentence form.
> 
> Just to visualize this approach. Below are the output for the tabular 
> form and the sentence form from last patch version (sql script attached):
> 
> Tabular form     rolname      |     memberof     |       options       
> |     grantor 
> ------------------+------------------+---------------------+------------------ postgres         |                 
|                    |  regress_du_admin | regress_du_role0+| admin, inherit, set+| postgres        +                 
|regress_du_role1+| admin, inherit, set+| postgres        +                  | regress_du_role2 | admin, inherit, set |
postgres regress_du_role0|                  |                     |  regress_du_role1 | regress_du_role0+| admin,
inherit,set+| regress_du_admin+                  | regress_du_role0+| inherit            +|
regress_du_role1+                 | regress_du_role0 | set                 | regress_du_role2 regress_du_role2 |
regress_du_role0+|admin              +| regress_du_admin+                  | regress_du_role0+| inherit, set       +|
regress_du_role1+                 | regress_du_role0+| empty              +| regress_du_role2+                  |
regress_du_role1| admin, set          | regress_du_admin(5 rows)Sentence form from patch v7     rolname     
|                          memberof
------------------+-------------------------------------------------------------- postgres        |  regress_du_admin |
regress_du_role0from postgres (admin, inherit, set)        +                  | regress_du_role1 from postgres (admin,
inherit,set)        +                  | regress_du_role2 from postgres (admin, inherit, set) regress_du_role0 |
 regress_du_role1| regress_du_role0 from regress_du_admin (admin, inherit, set)+                  | regress_du_role0
fromregress_du_role1 (inherit)            +                  | regress_du_role0 from regress_du_role2
(set) regress_du_role2| regress_du_role0 from regress_du_admin (admin)              +                  |
regress_du_role0from regress_du_role1 (inherit, set)       +                  | regress_du_role0 from regress_du_role2
(empty)             +                  | regress_du_role1 from regress_du_admin (admin, set)(5 rows)  
 
> 
> The tabular form solves the latest patch translation problems mentioned by Kyotaro.
> But it requires mapping elements between 3 array-like columns.
> 
> To move forward, needs more opinions?

[RMT Hat]

Nudging this along, as it's an open item. It'd be good to get this 
resolved before Beta 1, but that may be tough at this point.

[Personal hat]

I'm probably not the target user for this feature, so I'm not sure how 
much you should weigh my opinion (e.g. I still don't agree with 
explicitly showing "empty", but as mentioned, I'm not the target user).

That said, from a readability standpoint, it was easier for me to follow 
the tabular form vs. the sentence form.

Thanks,

Jonathan

On 18.05.2023 05:42, Jonathan S. Katz wrote:

That said, from a readability standpoint, it was easier for me to follow 
the tabular form vs. the sentence form.

May be possible to reach a agreement on the sentence form. Similar descriptions used
for referential constraints in the \d command:

create table t1 (id int primary key);
create table t2 (id int references t1(id));
\d t2
                 Table "public.t2"
 Column |  Type   | Collation | Nullable | Default 
--------+---------+-----------+----------+---------
 id     | integer |           |          | 
Foreign-key constraints:
    "t2_id_fkey" FOREIGN KEY (id) REFERENCES t1(id)


As for tabular form it looks more natural to have a separate psql command
for pg_auth_members system catalog. Something based on this query:

SELECT r.rolname role, m.rolname member,
       admin_option admin, inherit_option inherit, set_option set,
       g.rolname grantor
FROM pg_catalog.pg_auth_members pam
     JOIN pg_catalog.pg_roles r ON (pam.roleid = r.oid)
     JOIN pg_catalog.pg_roles m ON (pam.member = m.oid)
     JOIN pg_catalog.pg_roles g ON (pam.grantor = g.oid)
WHERE r.rolname !~ '^pg_'
ORDER BY role, member, grantor;
       role       |      member      | admin | inherit | set |     grantor      
------------------+------------------+-------+---------+-----+------------------
 regress_du_role0 | regress_du_admin | t     | t       | t   | postgres
 regress_du_role0 | regress_du_role1 | t     | t       | t   | regress_du_admin
 regress_du_role0 | regress_du_role1 | f     | t       | f   | regress_du_role1
 regress_du_role0 | regress_du_role1 | f     | f       | t   | regress_du_role2
 regress_du_role0 | regress_du_role2 | t     | f       | f   | regress_du_admin
 regress_du_role0 | regress_du_role2 | f     | t       | t   | regress_du_role1
 regress_du_role0 | regress_du_role2 | f     | f       | f   | regress_du_role2
 regress_du_role1 | regress_du_admin | t     | t       | t   | postgres
 regress_du_role1 | regress_du_role2 | t     | f       | t   | regress_du_admin
 regress_du_role2 | regress_du_admin | t     | t       | t   | postgres
(10 rows)

But is it worth inventing a new psql command for this?

-----
Pavel Luzanov

On 6/15/23 2:47 PM, David G. Johnston wrote:
> Robert - can you please comment on what you are willing to commit in 
> order to close out your open item here.  My take is that the design for 
> this, the tabular form a couple of emails ago (copied here), is 
> ready-to-commit, just needing the actual (trivial) code changes to be 
> made to accomplish it.
> 
> Tabular form
> 
>       rolname      |     memberof     |       options       |     
> grantor 
> ------------------+------------------+---------------------+------------------ postgres         |                 
|                    |  regress_du_admin | regress_du_role0+| admin, inherit, set+| postgres        +                 
|regress_du_role1+| admin, inherit, set+| postgres        +                  | regress_du_role2 | admin, inherit, set |
postgres regress_du_role0|                  |                     |  regress_du_role1 | regress_du_role0+| admin,
inherit,set+| regress_du_admin+                  | regress_du_role0+| inherit            +|
regress_du_role1+                 | regress_du_role0 | set                 | regress_du_role2 regress_du_role2 |
regress_du_role0+|admin              +| regress_du_admin+                  | regress_du_role0+| inherit, set       +|
regress_du_role1+                 | regress_du_role0+| empty              +| regress_du_role2+                  |
regress_du_role1| admin, set          | regress_du_admin(5 rows)
 
> 

[RMT hat]

Can we resolve this before Beta 2?[1] The RMT originally advised to try 
to resolve before Beta 1[2], and this seems to be lingering.

Thanks,

Jonathan

[1] 
https://www.postgresql.org/message-id/460ae02a-3123-16a3-f2d7-ccd79778819b%40postgresql.org
[2] 
https://www.postgresql.org/message-id/d61db38b-29d9-81cc-55b3-8a5c704bb969%40postgresql.org

"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> On 6/15/23 2:47 PM, David G. Johnston wrote:
>> Robert - can you please comment on what you are willing to commit in 
>> order to close out your open item here.  My take is that the design for 
>> this, the tabular form a couple of emails ago (copied here), is 
>> ready-to-commit, just needing the actual (trivial) code changes to be 
>> made to accomplish it.

> Can we resolve this before Beta 2?[1] The RMT originally advised to try 
> to resolve before Beta 1[2], and this seems to be lingering.

At this point I kinda doubt that we can get this done before beta2
either, but I'll put in my two cents anyway:

* I agree that the "tabular" format looks nicer and has fewer i18n
issues than the other proposals.

* Personally I could do without the "empty" business, but that seems
unnecessary in the tabular format; an empty column will serve fine.

* I also agree with Pavel's comment that we'd be better off taking
this out of \du altogether and inventing a separate \d command.
Maybe "\drg" for "display role grants"?

* Parenthetically, the "Attributes" column of \du is a complete
disaster, lacking not only conceptual but even notational consistency.
(Who decided that some items belonged on their own line and others
not?)  I suppose it's way too late to redesign that for v16.  But
I think we'd have more of a free hand to clean that up if we weren't
trying to shoehorn role grants into the same display.

            regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Thu, Jun 22, 2023 at 5:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * I agree that the "tabular" format looks nicer and has fewer i18n
>> issues than the other proposals.

> As you are on board with a separate command please clarify whether you mean
> the tabular format but still with newlines, one row per grantee, or the
> table with one row per grantor-grantee pair.

I'd lean towards a straight table with a row per grantee/grantor.
I tend to think that faking table layout with some newlines is
a poor idea.  I'm not dead set on that approach though.

            regards, tom lane

On 6/23/23 11:52 AM, David G. Johnston wrote:
> On Thu, Jun 22, 2023 at 5:08 PM Tom Lane <tgl@sss.pgh.pa.us 
> <mailto:tgl@sss.pgh.pa.us>> wrote:
> 
>     "Jonathan S. Katz" <jkatz@postgresql.org
>     <mailto:jkatz@postgresql.org>> writes:
>      > On 6/15/23 2:47 PM, David G. Johnston wrote:
>      >> Robert - can you please comment on what you are willing to
>     commit in
>      >> order to close out your open item here.  My take is that the
>     design for
>      >> this, the tabular form a couple of emails ago (copied here), is
>      >> ready-to-commit, just needing the actual (trivial) code changes
>     to be
>      >> made to accomplish it.
> 
>      > Can we resolve this before Beta 2?[1] The RMT originally advised
>     to try
>      > to resolve before Beta 1[2], and this seems to be lingering.
> 
>     At this point I kinda doubt that we can get this done before beta2
>     either, but I'll put in my two cents anyway:

[RMT Hat]

Well, the probability of completing this before the beta 2 freeze is 
effectively zero now. This is a bit disappointing as there was ample 
time since the first RMT nudge on the issue. But let's move forward and 
resolve it before Beta 3.

>     * I agree that the "tabular" format looks nicer and has fewer i18n
>     issues than the other proposals.
> 
> As you are on board with a separate command please clarify whether you 
> mean the tabular format but still with newlines, one row per grantee, or 
> the table with one row per grantor-grantee pair.
> 
> I still like using newlines here even in the separate meta-command.

(I'll save for the downthread comment).

> 
>     * Personally I could do without the "empty" business, but that seems
>     unnecessary in the tabular format; an empty column will serve fine.
> 
> 
> I disagree, but not strongly.
> 
> I kinda expected you to be on the side of "why are we discussing a 
> situation that should just be prohibited" though.

[Personal hat]

I'm still not a fan of "empty" but perhaps the formatting around the 
"separate command" will help drive a conclusion on this.

> 
>     * I also agree with Pavel's comment that we'd be better off taking
>     this out of \du altogether and inventing a separate \d command.
>     Maybe "\drg" for "display role grants"?
> 
> Just to be clear, the open item fix proposal is to remove the presently 
> broken (due to it showing duplicates without any context) "member of" 
> array in \du and make a simple table report output in \drg instead.
> 
> I'm good with \drg as a new meta-command.

[Personal hat]

+1 for a new command. The proposal above seems reasonable.

Thanks,

Jonathan

On 6/23/23 12:16 PM, Tom Lane wrote:
> "David G. Johnston" <david.g.johnston@gmail.com> writes:
>> On Thu, Jun 22, 2023 at 5:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> * I agree that the "tabular" format looks nicer and has fewer i18n
>>> issues than the other proposals.
> 
>> As you are on board with a separate command please clarify whether you mean
>> the tabular format but still with newlines, one row per grantee, or the
>> table with one row per grantor-grantee pair.
> 
> I'd lean towards a straight table with a row per grantee/grantor.
> I tend to think that faking table layout with some newlines is
> a poor idea.  I'm not dead set on that approach though.

[Personal hat]

Generally, I find the tabular view w/o newlines is easier to read, and 
makes it simpler to join to other data (though that may not be 
applicable here).

Again, I'm not the target user of this feature (until I need to use it), 
so my opinion comes with a few grains of salt.

Thanks,

Jonathan

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Thu, Jun 22, 2023 at 5:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * Personally I could do without the "empty" business, but that seems
>> unnecessary in the tabular format; an empty column will serve fine.

> I disagree, but not strongly.

> I kinda expected you to be on the side of "why are we discussing a
> situation that should just be prohibited" though.

I haven't formed an opinion yet on whether it should be prohibited.
But even if we do that going forward, won't psql need to deal with
such cases when examining old servers?

            regards, tom lane

Thank you for all valuable comments. I can now continue working on the 
patch.
Here's what I plan to do in the next version.

Changes for \du & \dg commands
* showing distinct roles in the "Member of" column
* explicit order for list of roles
* no changes for extended mode (\du+)

New meta-command \drg
* showing info from pg_auth_members based on a query:

SELECT r.rolname role, m.rolname member,
        pg_catalog.concat_ws(', ',
            CASE WHEN pam.admin_option THEN 'ADMIN' END,
            CASE WHEN pam.inherit_option THEN 'INHERIT' END,
            CASE WHEN pam.set_option THEN 'SET' END
        ) AS options,
        g.rolname grantor
FROM pg_catalog.pg_auth_members pam
      JOIN pg_catalog.pg_roles r ON (pam.roleid = r.oid)
      JOIN pg_catalog.pg_roles m ON (pam.member = m.oid)
      JOIN pg_catalog.pg_roles g ON (pam.grantor = g.oid)
WHERE r.rolname !~ '^pg_'
ORDER BY role, member, grantor;
        role       |      member      |       options |     grantor
------------------+------------------+---------------------+------------------
  regress_du_role0 | regress_du_admin | ADMIN, INHERIT, SET | postgres
  regress_du_role0 | regress_du_role1 | ADMIN, INHERIT, SET | 
regress_du_admin
  regress_du_role0 | regress_du_role1 | INHERIT | regress_du_role1
  regress_du_role0 | regress_du_role1 | SET | regress_du_role2
  regress_du_role0 | regress_du_role2 | ADMIN | regress_du_admin
  regress_du_role0 | regress_du_role2 | INHERIT, SET | regress_du_role1
  regress_du_role0 | regress_du_role2 | | regress_du_role2
  regress_du_role1 | regress_du_admin | ADMIN, INHERIT, SET | postgres
  regress_du_role1 | regress_du_role2 | ADMIN, SET | regress_du_admin
  regress_du_role2 | regress_du_admin | ADMIN, INHERIT, SET | postgres
(10 rows)

Notes
* The name of the new command. It's a good name, if not for the history.
There are two commands showing the same information about roles: \du and 
\dr.
The addition of \drg may be misinterpreted: if there is \drg, then there 
is also \dug.
Maybe it's time to think about deprecating of the \du command and leave 
only \dg in the next versions?

* 'empty'. I suggest thinking about forbidding the situation with empty 
options.
If we prohibit them, the issue will be resolved automatically.

* The new meta-command will also make sense for versions <16.
The ADMIN OPTION is available in all supported versions.

* The new meta-command will not show all roles. It will only show the 
roles included in other roles.
To show all roles you need to add an outer join between pg_roles and 
pg_auth_members.
But all columns except "role" will be left blank. Is it worth doing this?

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

On Sat, Jun 24, 2023 at 8:11 AM Pavel Luzanov <p.luzanov@postgrespro.ru> wrote:

There are two commands showing the same information about roles: \du and
\dr.

I would add \dr as the new official command to complement adding \drg and deprecate both \du and \dg.  Though actual removal and de-documenting doesn't seem like a good idea.  But if we ever did assign something non-role to \dr it would be very confusing.

* The new meta-command will also make sense for versions <16.
The ADMIN OPTION is available in all supported versions.

Doesn't every role pre-16 gain SET permission?  We can also deduce whether the grant provides INHERIT based upon the attribute of the role in question.

* The new meta-command will not show all roles. It will only show the
roles included in other roles.
To show all roles you need to add an outer join between pg_roles and
pg_auth_members.
But all columns except "role" will be left blank. Is it worth doing this?

I'm inclined to want this.  I would be good when specifying a role to filter upon that all rows that do exist matching that filter end up in the output regardless if they are standalone or not.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Please find attached new patch version.
It implements \drg command and hides duplicates in \du & \dg commands.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Pavel Luzanov <p.luzanov@postgrespro.ru> writes:
> Please find attached new patch version.
> It implements \drg command and hides duplicates in \du & \dg commands.

I took a quick look through this, and have some minor suggestions:

1. I was thinking in terms of dropping the "Member of" column entirely
in \du and \dg.  It doesn't tell you enough, and the output of those
commands is often too wide already.

2. You have describeRoleGrants() set up to localize "ADMIN", "INHERIT",
and "SET".  Since those are SQL keywords, our usual practice is to not
localize them; this'd simplify the code.

3. Not sure about use of LEFT JOIN in the query.  That will mean we
get a row out even for roles that have no grants, which seems like
clutter.  The LEFT JOINs to r and g are fine, but I suggest changing
the first join to a plain join.

Beyond those nits, I think this is a good approach and we should
adopt it (including in v16).

            regards, tom lane

1. I was thinking in terms of dropping the "Member of" column entirely
in \du and \dg.  It doesn't tell you enough, and the output of those
commands is often too wide already.

2. You have describeRoleGrants() set up to localize "ADMIN", "INHERIT",
and "SET".  Since those are SQL keywords, our usual practice is to not
localize them; this'd simplify the code.

3. Not sure about use of LEFT JOIN in the query.  That will mean we
get a row out even for roles that have no grants, which seems like
clutter.  The LEFT JOINs to r and g are fine, but I suggest changing
the first join to a plain join.

On Sat, Jun 24, 2023 at 8:11 AM Pavel Luzanov <p.luzanov@postgrespro.ru> wrote:

* The new meta-command will not show all roles. It will only show the
roles included in other roles.
To show all roles you need to add an outer join between pg_roles and
pg_auth_members.
But all columns except "role" will be left blank. Is it worth doing this?

I'm inclined to want this.  I would be good when specifying a role to filter upon that all rows that do exist matching that filter end up in the output regardless if they are standalone or not.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

On 09.07.2023 13:56, Pavel Luzanov wrote:
> On 08.07.2023 20:07, Tom Lane wrote:
>> 1. I was thinking in terms of dropping the "Member of" column entirely
>> in \du and \dg.  It doesn't tell you enough, and the output of those
>> commands is often too wide already.
>
>> 2. You have describeRoleGrants() set up to localize "ADMIN", "INHERIT",
>> and "SET".  Since those are SQL keywords, our usual practice is to not
>> localize them; this'd simplify the code.
>
>
>> 3. Not sure about use of LEFT JOIN in the query.  That will mean we
>> get a row out even for roles that have no grants, which seems like
>> clutter.  The LEFT JOINs to r and g are fine, but I suggest changing
>> the first join to a plain join.
>
> So, I accepted all three suggestions. I will wait for other opinions and
> plan to implement discussed changes within a few days.

Please review the updated version with suggested changes.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

On 08.07.2023 20:07, Tom Lane wrote
> 3. Not sure about use of LEFT JOIN in the query.  That will mean we
> get a row out even for roles that have no grants, which seems like
> clutter.  The LEFT JOINs to r and g are fine, but I suggest changing
> the first join to a plain join.

Hm.
Can you explain why LEFT JOIN to r and g are fine after removing LEFT 
JOIN to pam?
Why not to change all three left joins to plain join?

The query for v16+ now looks like:

SELECT m.rolname AS "Role name", r.rolname AS "Member of",
   pg_catalog.concat_ws(', ',
     CASE WHEN pam.admin_option THEN 'ADMIN' END,
     CASE WHEN pam.inherit_option THEN 'INHERIT' END,
     CASE WHEN pam.set_option THEN 'SET' END
   ) AS "Options",
   g.rolname AS "Grantor"
FROM pg_catalog.pg_roles m
      JOIN pg_catalog.pg_auth_members pam ON (pam.member = m.oid)
      LEFT JOIN pg_catalog.pg_roles r ON (pam.roleid = r.oid)
      LEFT JOIN pg_catalog.pg_roles g ON (pam.grantor = g.oid)
WHERE m.rolname !~ '^pg_'
ORDER BY 1, 2, 4;


And for versions <16 I forget to simplify expression for 'Options' 
column after removing LEFT JOIN on pam:

SELECT m.rolname AS "Role name", r.rolname AS "Member of",
   pg_catalog.concat_ws(', ',
     CASE WHEN pam.admin_option THEN 'ADMIN' END,
     CASE WHEN pam.roleid IS NOT NULL AND m.rolinherit THEN 'INHERIT' END,
     CASE WHEN pam.roleid IS NOT NULL THEN 'SET' END
   ) AS "Options",
   g.rolname AS "Grantor"
FROM pg_catalog.pg_roles m
      JOIN pg_catalog.pg_auth_members pam ON (pam.member = m.oid)
      LEFT JOIN pg_catalog.pg_roles r ON (pam.roleid = r.oid)
      LEFT JOIN pg_catalog.pg_roles g ON (pam.grantor = g.oid)
WHERE m.rolname !~ '^pg_'
ORDER BY 1, 2, 4;

I plan to replace it to:

   pg_catalog.concat_ws(', ',
     CASE WHEN pam.admin_option THEN 'ADMIN' END,
     CASE WHEN m.rolinherit THEN 'INHERIT' END,
     'SET'
   ) AS "Options",

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Pavel Luzanov <p.luzanov@postgrespro.ru> writes:
> On 08.07.2023 20:07, Tom Lane wrote
>> 3. Not sure about use of LEFT JOIN in the query.  That will mean we
>> get a row out even for roles that have no grants, which seems like
>> clutter.  The LEFT JOINs to r and g are fine, but I suggest changing
>> the first join to a plain join.

> Can you explain why LEFT JOIN to r and g are fine after removing LEFT 
> JOIN to pam?

The idea with that, IMO, is to do something at least minimally sane
if there's a bogus role OID in pg_auth_members.  With plain joins,
the output row would disappear and you'd have no clue that anything
is wrong.  With left joins, you get a row with a null column and
there's reason to investigate why.

Since such a case should not happen in normal use, I don't think it
counts for discussions about compactness of output.  However, this
is also an argument for using a plain not left join between pg_roles
and pg_auth_members: if we do it as per the earlier patch, then
nulls in the output are common and wouldn't draw your attention.
(Indeed, I think broken and not-broken pg_auth_members contents
would be indistinguishable.)

> I plan to replace it to:

>    pg_catalog.concat_ws(', ',
>      CASE WHEN pam.admin_option THEN 'ADMIN' END,
>      CASE WHEN m.rolinherit THEN 'INHERIT' END,
>      'SET'
>    ) AS "Options",

That does not seem right.  Is it impossible for pam.set_option
to be false?  Even if it is, should this code assume that?

            regards, tom lane

On 13.07.2023 18:01, Tom Lane wrote:
> The idea with that, IMO, is to do something at least minimally sane
> if there's a bogus role OID in pg_auth_members.  With plain joins,
> the output row would disappear and you'd have no clue that anything
> is wrong.  With left joins, you get a row with a null column and
> there's reason to investigate why.
>
> Since such a case should not happen in normal use, I don't think it
> counts for discussions about compactness of output.  However, this
> is also an argument for using a plain not left join between pg_roles
> and pg_auth_members: if we do it as per the earlier patch, then
> nulls in the output are common and wouldn't draw your attention.
> (Indeed, I think broken and not-broken pg_auth_members contents
> would be indistinguishable.)

It reminds me about defensive programming practices.
That's great, thanks for explanation.

> That does not seem right.  Is it impossible for pam.set_option
> to be false?  Even if it is, should this code assume that?

For versions before 16, including one role to another automatically
gives possibility to issue SET ROLE.

IMO, the only question is whether it is correct to show IMPLICIT and
SET options in versions where they are not actually present
in pg_auth_members, but can be determined.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Pavel Luzanov <p.luzanov@postgrespro.ru> writes:
> On 13.07.2023 18:01, Tom Lane wrote:
>> That does not seem right.  Is it impossible for pam.set_option
>> to be false?  Even if it is, should this code assume that?

> For versions before 16, including one role to another automatically
> gives possibility to issue SET ROLE.

Right, -ENOCAFFEINE.

> IMO, the only question is whether it is correct to show IMPLICIT and
> SET options in versions where they are not actually present
> in pg_auth_members, but can be determined.

Hmm, that's definitely a judgment call.  You could argue that it's
useful and consistent, but also that it's confusing to somebody
who's not familiar with the new terminology.  On balance I'd lean
to showing them, but I won't fight hard for that viewpoint.

            regards, tom lane

On 13.07.2023 11:26, Pavel Luzanov wrote:
> And for versions <16 I forget to simplify expression for 'Options' 
> column after removing LEFT JOIN on pam:
>
> SELECT m.rolname AS "Role name", r.rolname AS "Member of",
>   pg_catalog.concat_ws(', ',
>     CASE WHEN pam.admin_option THEN 'ADMIN' END,
>     CASE WHEN pam.roleid IS NOT NULL AND m.rolinherit THEN 'INHERIT' END,
>     CASE WHEN pam.roleid IS NOT NULL THEN 'SET' END
>   ) AS "Options",
>   g.rolname AS "Grantor"
> FROM pg_catalog.pg_roles m
>      JOIN pg_catalog.pg_auth_members pam ON (pam.member = m.oid)
>      LEFT JOIN pg_catalog.pg_roles r ON (pam.roleid = r.oid)
>      LEFT JOIN pg_catalog.pg_roles g ON (pam.grantor = g.oid)
> WHERE m.rolname !~ '^pg_'
> ORDER BY 1, 2, 4;
>
> I plan to replace it to:
>
>   pg_catalog.concat_ws(', ',
>     CASE WHEN pam.admin_option THEN 'ADMIN' END,
>     CASE WHEN m.rolinherit THEN 'INHERIT' END,
>     'SET'
>   ) AS "Options",
>

The new version contains only this change.

-- 
-----
Pavel Luzanov

I tried this out.  It looks good to me, and I like it.  Not translating
the labels seems correct to me.

+1 for backpatching to 16, given that it's a psql-only change that
pertains to a backend change that was done in the 16 timeframe.

Regarding the controversy of showing SET for previous versions, I think
it's clearer if it's shown, because ultimately what the user really
wants to know is if the role can be SET to; they don't want to have to
learn from memory in which version they can SET because the column is
empty and in which version they have to look for the label.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Small aircraft do not crash frequently ... usually only once!"
                                  (ponder, http://thedailywtf.com/)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I tried this out.  It looks good to me, and I like it.  Not translating
> the labels seems correct to me.
> +1 for backpatching to 16, given that it's a psql-only change that
> pertains to a backend change that was done in the 16 timeframe.

Agreed.  In the interests of moving things along, I'll take point
on getting this committed.

> Regarding the controversy of showing SET for previous versions, I think
> it's clearer if it's shown, because ultimately what the user really
> wants to know is if the role can be SET to; they don't want to have to
> learn from memory in which version they can SET because the column is
> empty and in which version they have to look for the label.

Seems reasonable.  I'll go with that interpretation unless there's
pretty quick pushback.

            regards, tom lane

I wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> +1 for backpatching to 16, given that it's a psql-only change that
>> pertains to a backend change that was done in the 16 timeframe.

> Agreed.  In the interests of moving things along, I'll take point
> on getting this committed.

And done, with some minor editorialization.  I'll go mark the
open item as closed.

            regards, tom lane

On 19.07.2023 19:47, Tom Lane wrote:
> And done, with some minor editorialization.

Thanks to everyone who participated in the work.
Special thanks to David for moving forward this patch for a long time, 
and to Tom for taking commit responsibilities.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

On 7/19/23 1:44 PM, Pavel Luzanov wrote:
> On 19.07.2023 19:47, Tom Lane wrote:
>> And done, with some minor editorialization.
> 
> Thanks to everyone who participated in the work.
> Special thanks to David for moving forward this patch for a long time, 
> and to Tom for taking commit responsibilities.

[RMT]

+1; thanks to everyone for seeing this through!

Jonathan