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

From: Pavel Luzanov <[email-masked]>
Date: Thu, 16 Feb 2023 21:03:29 +0300

On 16.02.2023 00:37, David G. Johnston wrote:
> 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

---

From: Pavel Luzanov <[email-masked]>
Date: Fri, 17 Feb 2023 14:02:01 +0300

Hello,
> 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

---

From: "David G. Johnston" <[email-masked]>
Date: Fri, 17 Feb 2023 09:53:35 -0700

On Fri, Feb 17, 2023 at 4:02 AM Pavel Luzanov <[email-masked]>
wrote:

>  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

---

From: Pavel Luzanov <[email-masked]>
Date: Wed, 22 Feb 2023 00:14:34 +0300

On 17.02.2023 19:53, David G. Johnston wrote:
> On Fri, Feb 17, 2023 at 4:02 AM Pavel Luzanov 
> <[email-masked]> wrote:
>
>                                        List of roles
>      admin     | Create
>     role                                                | {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.

Ok, I'll try in the next few days. But what presentation format to use?

1. bob(admin_option=t inherit_option=t set_option=f) -- it seems very long
2. bob(ai) -- short, but will it be clear?
3. something else?

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

This can help in issuing GRANT command, but not REVOKE. Revoking a 
role's membership is now very similar to revoking privileges. Only the 
role that granted membership can revoke that membership. So for REVOKE 
you need to know who granted membership, but this information will not 
be available after grouping.

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

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.

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

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 21 Feb 2023 14:34:27 -0700

On Tue, Feb 21, 2023 at 2:14 PM Pavel Luzanov <[email-masked]>
wrote:

> to produce their result.
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)
I don't think first-letter mnemonics will be an issue - you need to learn
the syntax anyway.  And it is already what we do for object grants besides.

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

So I converted the "/" into "from" and stuck the permissions on the end
instead of in the middle (makes reading the "from" fragment cleaner).

To be clear, this is going away from grouping but trades verbosity and
deviation from what is done today for better information.  If we are going
to break this I suppose we might as well break it thoroughly.

>
> I didn't even know this function existed. But I see that it was changed in
> 3d14e171 with updated documentation:
>
I think that should probably have ADMIN as one of the options as well.
Also curious what it reports for an empty membership.

David J.

---

From: Pavel Luzanov <[email-masked]>
Date: Mon, 27 Feb 2023 23:14:42 +0300

On 22.02.2023 00:34, David G. Johnston wrote:
> 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).
>

I think this is a good compromise.

> undesirable: bob=asi/grantor

Agree. Membership options are not the ACL (although they have 
similarities). Therefore, showing them as a ACL-like column will be 
confusing.

So, please find attached the second version of the patch. It implements 
suggested display format and small refactoring of existing code for \du 
command.
As a non-native writer, I have doubts about the documentation part.

-- 
Pavel Luzanov
Postgres Professional:https://postgrespro.com
From 70489a605687a325287bad109e2741dd7d08cea3 Mon Sep 17 00:00:00 2001
From: Pavel Luzanov <[email-masked]>
Date: Mon, 27 Feb 2023 22:35:29 +0300
Subject: [PATCH v2] psql: \du shows membership options

---
 doc/src/sgml/ref/psql-ref.sgml     | 12 ++++++++
 src/bin/psql/describe.c            | 45 +++++++++++++++++++-----------
 src/test/regress/expected/psql.out | 45 ++++++++++++++++++++++++++++++
 src/test/regress/sql/psql.sql      | 25 +++++++++++++++++
 4 files changed, 111 insertions(+), 16 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc6528dc11..c94a2287f0 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1724,6 +1724,12 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
         <literal>S</literal> modifier to include system roles.
         If <replaceable class="parameter">pattern</replaceable> is specified,
         only those roles whose names match the pattern are listed.
+        For each membership in the role, the membership options and
+        the role that granted the membership are displayed.
+        Оne-letter abbreviations are used for membership options:
+        <literal>a</literal> &mdash; admin option, <literal>i</literal> &mdash; inherit option,
+        <literal>s</literal> &mdash; set option and <literal>empty</literal> if no one is set.
+        See <link linkend="sql-grant"><command>GRANT</command></link> command for their meaning.
         If the form <literal>\dg+</literal> is used, additional information
         is shown about each role; currently this adds the comment for each
         role.
@@ -1966,6 +1972,12 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
         <literal>S</literal> modifier to include system roles.
         If <replaceable class="parameter">pattern</replaceable> is specified,
         only those roles whose names match the pattern are listed.
+        For each membership in the role, the membership options and
+        the role that granted the membership are displayed.
+        Оne-letter abbreviations are used for membership options:
+        <literal>a</literal> &mdash; admin option, <literal>i</literal> &mdash; inherit option,
+        <literal>s</literal> &mdash; set option and <literal>empty</literal> if no one is set.
+        See <link linkend="sql-grant"><command>GRANT</command></link> command for their meaning.
         If the form <literal>\du+</literal> is used, additional information
         is shown about each role; currently this adds the comment for each
         role.
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index c8a0bb7b3a..27a8680ddf 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3623,22 +3623,36 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 					  "SELECT r.rolname, r.rolsuper, r.rolinherit,\n"
 					  "  r.rolcreaterole, r.rolcreatedb, r.rolcanlogin,\n"
 					  "  r.rolconnlimit, r.rolvaliduntil,\n"
-					  "  ARRAY(SELECT b.rolname\n"
-					  "        FROM pg_catalog.pg_auth_members m\n"
-					  "        JOIN pg_catalog.pg_roles b ON (m.roleid = b.oid)\n"
-					  "        WHERE m.member = r.oid) as memberof");
+					  "  r.rolreplication, r.rolbypassrls,\n");
+
+	if (pset.sversion >= 160000)
+		appendPQExpBufferStr(&buf,
+							 "  (SELECT pg_catalog.string_agg(\n"
+							 "    pg_catalog.format('%I from %I (%s)',\n"
+							 "      b.rolname, m.grantor::pg_catalog.regrole::pg_catalog.text,\n"
+							 "      pg_catalog.regexp_replace(\n"
+							 "        pg_catalog.concat_ws(', ',\n"
+							 "          CASE WHEN m.admin_option THEN 'a' END,\n"
+							 "          CASE WHEN m.inherit_option THEN 'i' END,\n"
+							 "          CASE WHEN m.set_option THEN 's' END),\n"
+							 "      '^$', 'empty')\n"
+							 "    ), E'\\n'\n"
+							 "    ORDER BY b.rolname, m.grantor::pg_catalog.regrole::pg_catalog.text)\n"
+							 "  FROM pg_catalog.pg_auth_members m\n"
+							 "       JOIN pg_catalog.pg_roles b ON (m.roleid = b.oid)\n"
+							 "  WHERE m.member = r.oid) as memberof");
+	else
+		appendPQExpBufferStr(&buf,
+							 "  ARRAY(SELECT b.rolname\n"
+							 "        FROM pg_catalog.pg_auth_members m\n"
+							 "        JOIN pg_catalog.pg_roles b ON (m.roleid = b.oid)\n"
+							 "        WHERE m.member = r.oid) as memberof");
 
 	if (verbose)
 	{
 		appendPQExpBufferStr(&buf, "\n, pg_catalog.shobj_description(r.oid, 'pg_authid') AS description");
 		ncols++;
 	}
-	appendPQExpBufferStr(&buf, "\n, r.rolreplication");
-
-	if (pset.sversion >= 90500)
-	{
-		appendPQExpBufferStr(&buf, "\n, r.rolbypassrls");
-	}
 
 	appendPQExpBufferStr(&buf, "\nFROM pg_catalog.pg_roles r\n");
 
@@ -3692,12 +3706,11 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 		if (strcmp(PQgetvalue(res, i, 5), "t") != 0)
 			add_role_attribute(&buf, _("Cannot login"));
 
-		if (strcmp(PQgetvalue(res, i, (verbose ? 10 : 9)), "t") == 0)
+		if (strcmp(PQgetvalue(res, i, 8), "t") == 0)
 			add_role_attribute(&buf, _("Replication"));
 
-		if (pset.sversion >= 90500)
-			if (strcmp(PQgetvalue(res, i, (verbose ? 11 : 10)), "t") == 0)
-				add_role_attribute(&buf, _("Bypass RLS"));
+		if (strcmp(PQgetvalue(res, i, 9), "t") == 0)
+			add_role_attribute(&buf, _("Bypass RLS"));
 
 		conns = atoi(PQgetvalue(res, i, 6));
 		if (conns >= 0)
@@ -3726,10 +3739,10 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 
 		printTableAddCell(&cont, attr[i], false, false);
 
-		printTableAddCell(&cont, PQgetvalue(res, i, 8), false, false);
+		printTableAddCell(&cont, PQgetvalue(res, i, 10), false, false);
 
 		if (verbose)
-			printTableAddCell(&cont, PQgetvalue(res, i, 9), false, false);
+			printTableAddCell(&cont, PQgetvalue(res, i, 11), false, false);
 	}
 	termPQExpBuffer(&buf);
 
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 8fc62cebd2..a9e47f1ebf 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -6578,3 +6578,48 @@ cross-database references are not implemented: "no.such.database"."no.such.schem
 cross-database references are not implemented: "no.such.database"."no.such.schema"."no.such.data.type"
 \dX "no.such.database"."no.such.schema"."no.such.extended.statistics"
 cross-database references are not implemented: "no.such.database"."no.such.schema"."no.such.extended.statistics"
+-- check \du
+CREATE ROLE regress_du0;
+CREATE ROLE regress_du1 LOGIN;
+CREATE ROLE regress_du2 LOGIN;
+COMMENT ON ROLE regress_du0 IS 'Description for regress_du0';
+COMMENT ON ROLE regress_du1 IS 'Description for regress_du1';
+COMMENT ON ROLE regress_du2 IS 'Description for regress_du2';
+GRANT regress_du0 TO regress_du1 WITH ADMIN TRUE, INHERIT FALSE, SET FALSE;
+GRANT regress_du0 TO regress_du2 WITH ADMIN TRUE, INHERIT FALSE, SET TRUE;
+GRANT pg_monitor TO regress_du1;
+GRANT regress_du0 TO regress_du1 WITH ADMIN FALSE, INHERIT TRUE, SET FALSE GRANTED BY regress_du1;
+GRANT regress_du0 TO regress_du2 WITH ADMIN FALSE, INHERIT FALSE, SET FALSE GRANTED BY regress_du1;
+GRANT regress_du0 TO regress_du2 WITH ADMIN FALSE, INHERIT FALSE, SET TRUE GRANTED BY regress_du2;
+GRANT regress_du0 TO regress_du1 WITH ADMIN TRUE, INHERIT TRUE, SET TRUE GRANTED BY regress_du2;
+\du regress*
+                            List of roles
+  Role name  |  Attributes  |               Member of                
+-------------+--------------+----------------------------------------
+ regress_du0 | Cannot login | 
+ regress_du1 |              | pg_monitor from pal (i, s)            +
+             |              | regress_du0 from pal (a)              +
+             |              | regress_du0 from regress_du1 (i)      +
+             |              | regress_du0 from regress_du2 (a, i, s)
+ regress_du2 |              | regress_du0 from pal (a, s)           +
+             |              | regress_du0 from regress_du1 (empty)  +
+             |              | regress_du0 from regress_du2 (s)
+
+\du+ regress*
+                                           List of roles
+  Role name  |  Attributes  |               Member of                |         Description         
+-------------+--------------+----------------------------------------+-----------------------------
+ regress_du0 | Cannot login |                                        | Description for regress_du0
+ regress_du1 |              | pg_monitor from pal (i, s)            +| Description for regress_du1
+             |              | regress_du0 from pal (a)              +| 
+             |              | regress_du0 from regress_du1 (i)      +| 
+             |              | regress_du0 from regress_du2 (a, i, s) | 
+ regress_du2 |              | regress_du0 from pal (a, s)           +| Description for regress_du2
+             |              | regress_du0 from regress_du1 (empty)  +| 
+             |              | regress_du0 from regress_du2 (s)       | 
+
+REVOKE regress_du0 FROM regress_du1 CASCADE;
+REVOKE regress_du0 FROM regress_du2 CASCADE;
+DROP ROLE regress_du2;
+DROP ROLE regress_du1;
+DROP ROLE regress_du0;
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 2da9665a19..dca1ede0ab 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1791,3 +1791,28 @@ DROP FUNCTION psql_error;
 \dP "no.such.database"."no.such.schema"."no.such.partitioned.relation"
 \dT "no.such.database"."no.such.schema"."no.such.data.type"
 \dX "no.such.database"."no.such.schema"."no.such.extended.statistics"
+
+-- check \du
+CREATE ROLE regress_du0;
+CREATE ROLE regress_du1 LOGIN;
+CREATE ROLE regress_du2 LOGIN;
+COMMENT ON ROLE regress_du0 IS 'Description for regress_du0';
+COMMENT ON ROLE regress_du1 IS 'Description for regress_du1';
+COMMENT ON ROLE regress_du2 IS 'Description for regress_du2';
+
+GRANT regress_du0 TO regress_du1 WITH ADMIN TRUE, INHERIT FALSE, SET FALSE;
+GRANT regress_du0 TO regress_du2 WITH ADMIN TRUE, INHERIT FALSE, SET TRUE;
+GRANT pg_monitor TO regress_du1;
+GRANT regress_du0 TO regress_du1 WITH ADMIN FALSE, INHERIT TRUE, SET FALSE GRANTED BY regress_du1;
+GRANT regress_du0 TO regress_du2 WITH ADMIN FALSE, INHERIT FALSE, SET FALSE GRANTED BY regress_du1;
+GRANT regress_du0 TO regress_du2 WITH ADMIN FALSE, INHERIT FALSE, SET TRUE GRANTED BY regress_du2;
+GRANT regress_du0 TO regress_du1 WITH ADMIN TRUE, INHERIT TRUE, SET TRUE GRANTED BY regress_du2;
+
+\du regress*
+\du+ regress*
+
+REVOKE regress_du0 FROM regress_du1 CASCADE;
+REVOKE regress_du0 FROM regress_du2 CASCADE;
+DROP ROLE regress_du2;
+DROP ROLE regress_du1;
+DROP ROLE regress_du0;
-- 
2.34.1

---

From: Pavel Luzanov <[email-masked]>
Date: Wed, 1 Mar 2023 13:55:13 +0300

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

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com
From 6db62993d4b7afbcbce3e63ce3fbe3946ec50cff Mon Sep 17 00:00:00 2001
From: Pavel Luzanov <[email-masked]>
Date: Wed, 1 Mar 2023 13:29:10 +0300
Subject: [PATCH v3] psql: \du shows memberships options

---
 doc/src/sgml/ref/psql-ref.sgml     | 12 ++++++++
 src/bin/psql/describe.c            | 45 ++++++++++++++++++----------
 src/test/regress/expected/psql.out | 48 ++++++++++++++++++++++++++++++
 src/test/regress/sql/psql.sql      | 29 ++++++++++++++++++
 4 files changed, 118 insertions(+), 16 deletions(-)

 	if (verbose)
 	{

 

 

 		if (verbose)
 
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 8fc62cebd2..584d644261 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -6578,3 +6578,51 @@ cross-database references are not implemented: "no.such.database"."no.such.schem
 cross-database references are not implemented: "no.such.database"."no.such.schema"."no.such.data.type"
 \dX "no.such.database"."no.such.schema"."no.such.extended.statistics"
 cross-database references are not implemented: "no.such.database"."no.such.schema"."no.such.extended.statistics"
+-- check \du
+CREATE ROLE regress_du_role0;
+CREATE ROLE regress_du_role1;
+CREATE ROLE regress_du_role2;
+CREATE ROLE regress_du_admin;
+COMMENT ON ROLE regress_du_role0 IS 'Description for regress_du_role0';
+COMMENT ON ROLE regress_du_role1 IS 'Description for regress_du_role1';
+COMMENT ON ROLE regress_du_role2 IS 'Description for regress_du_role2';
+GRANT regress_du_role0 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role1 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role2 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN TRUE,  INHERIT TRUE,  SET TRUE  GRANTED BY regress_du_admin;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN TRUE,  INHERIT FALSE, SET FALSE GRANTED BY regress_du_admin;
+GRANT regress_du_role1 TO regress_du_role2 WITH ADMIN TRUE , INHERIT FALSE, SET TRUE  GRANTED BY regress_du_admin;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN FALSE, INHERIT TRUE,  SET FALSE GRANTED BY regress_du_role1;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN FALSE, INHERIT TRUE , SET TRUE  GRANTED BY regress_du_role1;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN FALSE, INHERIT FALSE, SET TRUE  GRANTED BY regress_du_role2;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN FALSE, INHERIT FALSE, SET FALSE GRANTED BY regress_du_role2;
+\du regress_du_role*
+                                   List of roles
+    Role name     |  Attributes  |                    Member of                     
+------------------+--------------+--------------------------------------------------
+ regress_du_role0 | Cannot login | 
+ regress_du_role1 | Cannot login | regress_du_role0 from regress_du_admin (a, i, s)+
+                  |              | regress_du_role0 from regress_du_role1 (i)      +
+                  |              | regress_du_role0 from regress_du_role2 (s)
+ regress_du_role2 | Cannot login | regress_du_role0 from regress_du_admin (a)      +
+                  |              | regress_du_role0 from regress_du_role1 (i, s)   +
+                  |              | regress_du_role0 from regress_du_role2 (empty)  +
+                  |              | regress_du_role1 from regress_du_admin (a, s)
+
+\du+ regress_du_role*
+                                                     List of roles
+    Role name     |  Attributes  |                    Member of                     |           Description            
+------------------+--------------+--------------------------------------------------+----------------------------------
+ regress_du_role0 | Cannot login |                                                  | Description for regress_du_role0
+ regress_du_role1 | Cannot login | regress_du_role0 from regress_du_admin (a, i, s)+| Description for regress_du_role1
+                  |              | regress_du_role0 from regress_du_role1 (i)      +| 
+                  |              | regress_du_role0 from regress_du_role2 (s)       | 
+ regress_du_role2 | Cannot login | regress_du_role0 from regress_du_admin (a)      +| Description for regress_du_role2
+                  |              | regress_du_role0 from regress_du_role1 (i, s)   +| 
+                  |              | regress_du_role0 from regress_du_role2 (empty)  +| 
+                  |              | regress_du_role1 from regress_du_admin (a, s)    | 
+
+DROP ROLE regress_du_role0;
+DROP ROLE regress_du_role1;
+DROP ROLE regress_du_role2;
+DROP ROLE regress_du_admin;
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 2da9665a19..e54a94c96a 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1791,3 +1791,32 @@ DROP FUNCTION psql_error;
 \dP "no.such.database"."no.such.schema"."no.such.partitioned.relation"
 \dT "no.such.database"."no.such.schema"."no.such.data.type"
 \dX "no.such.database"."no.such.schema"."no.such.extended.statistics"
+
+-- check \du
+CREATE ROLE regress_du_role0;
+CREATE ROLE regress_du_role1;
+CREATE ROLE regress_du_role2;
+CREATE ROLE regress_du_admin;
+COMMENT ON ROLE regress_du_role0 IS 'Description for regress_du_role0';
+COMMENT ON ROLE regress_du_role1 IS 'Description for regress_du_role1';
+COMMENT ON ROLE regress_du_role2 IS 'Description for regress_du_role2';
+
+GRANT regress_du_role0 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role1 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role2 TO regress_du_admin WITH ADMIN TRUE;
+
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN TRUE,  INHERIT TRUE,  SET TRUE  GRANTED BY regress_du_admin;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN TRUE,  INHERIT FALSE, SET FALSE GRANTED BY regress_du_admin;
+GRANT regress_du_role1 TO regress_du_role2 WITH ADMIN TRUE , INHERIT FALSE, SET TRUE  GRANTED BY regress_du_admin;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN FALSE, INHERIT TRUE,  SET FALSE GRANTED BY regress_du_role1;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN FALSE, INHERIT TRUE , SET TRUE  GRANTED BY regress_du_role1;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN FALSE, INHERIT FALSE, SET TRUE  GRANTED BY regress_du_role2;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN FALSE, INHERIT FALSE, SET FALSE GRANTED BY regress_du_role2;
+
+\du regress_du_role*
+\du+ regress_du_role*
+
+DROP ROLE regress_du_role0;
+DROP ROLE regress_du_role1;
+DROP ROLE regress_du_role2;
+DROP ROLE regress_du_admin;
-- 
2.34.1

---

From: Pavel Luzanov <[email-masked]>
Date: Fri, 3 Mar 2023 14:01:59 +0300

Hello,

On 22.02.2023 00:34, David G. Johnston wrote:
> I didn't even know this function existed. But I see that it was 
> changed in 3d14e171 with updated documentation:

I've been experimenting for a few days and I want to admit that this is 
a very difficult and not obvious topic.
I'll try to summarize what I think.

1.
About ADMIN value for pg_has_role.
Implementation of ADMIN value will be different from USAGE and SET.
To be True, USAGE value requires the full chain of memberships to have 
INHERIT option.
Similar with SET: the full chain of memberships must have SET option.
But for ADMIN, only last member in the chain must have ADMIN option and 
all previous members
must have INHERIT (to administer directly) or SET option (to switch to 
role, last in the chain).
Therefore, it is not obvious to me that the function needs the ADMIN value.

2.
pg_has_role function description starts with: Does user have privilege 
for role?
     - This is not exact: function works not only with users, but with 
NOLOGIN roles too.
     - Term "privilege": this term used for ACL columns, such usage may 
be confusing,
       especially after adding INHERIT and SET in addition to ADMIN option.

3.
It is possible to grant membership with all three options turned off:
     grant a to b with admin false, inherit false, set false;
But such membership is completely useless (if i didn't miss something).
May be such grants must be prohibited. At least this may be documented 
in the GRANT command.

4.
Since v16 it is possible to grant membership from one role to another 
several times with different grantors.
And only grantor can revoke membership.
     - This is not documented anywhere.
     - Current behavior of \du command with duplicated roles in "Member 
of" column strongly confusing.
       This is one of the goals of the discussion patch.

I think to write about this to pgsql-docs additionally to this topic.

---

From: "David G. Johnston" <[email-masked]>
Date: Fri, 3 Mar 2023 09:21:11 -0700

On Fri, Mar 3, 2023 at 4:01 AM Pavel Luzanov <[email-masked]>
wrote:

> I've been experimenting for a few days and I want to admit that this is a
> very difficult and not obvious topic.

Or you can SET to some role that then has an unbroken INHERIT chain to the
administered role.

ADMIN basically implies SET/USAGE but it doesn't work the other way around.

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.

>
> 2.
> pg_has_role function description starts with: Does user have privilege for
> role?
>     - Term "privilege": this term used for ACL columns, such usage may be
> confusing,

Yes, it missed the whole "there are only roles now" memo.  I don't have an
issue with using privilege here though - you have to use the GRANT command
which "defines access privileges".  Otherwise "membership option" or maybe
just "option" would need to be explored.

> May be such grants must be prohibited. At least this may be documented in
> the GRANT command.
>

I have no issue with prohibiting the "empty membership" if someone wants to
code that up.

>     - This is not documented anywhere.

Yeah, a pass over the GRANTED BY actual operation versus documentation
seems warranted.

>     - Current behavior of \du command with duplicated roles in "Member of"
> column strongly confusing.

This indeed needs to be fixed, one way (include grantor) or the other
(du-duplicate), with the current proposal of including grantor getting my
vote.

> I think to write about this to pgsql-docs additionally to this topic.

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.

David J.

---

From: Pavel Luzanov <[email-masked]>
Date: Mon, 6 Mar 2023 10:43:22 +0300

On 03.03.2023 19:21, David G. Johnston wrote:
> 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 must admit that I am slowly coming to the same conclusions that you 
have already outlined in previous messages.

Indeed, adding ADMIN to pg_has_role looks logical. The function will 
show whether one role can manage another directly or indirectly (via SET 
ROLE).
Adding ADMIN will lead to the question of naming other values. It is 
more reasonable to have INHERIT instead of USAGE.
And it is not very clear whether (except for backward compatibility) a 
separate MEMBER value is needed at all.

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

I agree.

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 7 Mar 2023 14:02:54 -0700

On Mon, Mar 6, 2023 at 12:43 AM Pavel Luzanov <[email-masked]>
wrote:

> Indeed, adding ADMIN to pg_has_role looks logical. The function will show
> whether one role can manage another directly or indirectly (via SET ROLE).
>

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

The administration column basically determines all this via a recursive
CTE.  I'm pondering how to incorporate some of this core material into it
now for both cross-validation purposes and ease-of-use.

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?

Adding ADMIN will lead to the question of naming other values. It is more
> reasonable to have INHERIT instead of USAGE.
>
> separate MEMBER value is needed at all.

There is an appeal to breaking things thoroughly here and removing both
MEMBER and USAGE terms while adding ADMIN, SET, INHERIT.

However, I am against that.  Most everyday usage of this would only likely
care about SET and INHERIT even going forward - administration of roles is
distinct from how those roles generally behave at runtime.  Breaking the
later because we improved upon the former doesn't seem defensible.  Thus,
while adding ADMIN makes sense, keeping MEMBER (a.k.a., SET) and USAGE
(a.k.a., INHERIT) is my suggested way forward.

I'll be looking over your v3 patch sometime this week, if not today.

David J.

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 7 Mar 2023 19:31:42 -0700

> I'll be looking over your v3 patch sometime this week, if not today.
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.  I find saying "additional
information is shown...currently this adds the comment".  Repeating that
"+" means (show more) everywhere seems excessive, just state what those
"more" things are.  I consider \dFp and \dl to be good examples in this
regard.

I also think that "Wall of text" doesn't serve us well.  See \dP for
permission to use paragraphs.

I didn't modify \du to match; keeping those in sync (as opposed to having
\du just say "see \dg") seems acceptable.

You had the direction of membership wrong in your copy: "For each
membership in the role" describes the reverse of "Member of" which is what
the column is.  The actual format template is constructed properly.

--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1727,15 +1727,18 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first
value' 'second value' \g
         <literal>S</literal> modifier to include system roles.
         If <replaceable class="parameter">pattern</replaceable> is
specified,
         only those roles whose names match the pattern are listed.
-        For each membership in the role, the membership options and
-        the role that granted the membership are displayed.
-        Оne-letter abbreviations are used for membership options:
-        <literal>a</literal> &mdash; admin option, <literal>i</literal>
&mdash; inherit option,
-        <literal>s</literal> &mdash; set option and
<literal>empty</literal> if no one is set.
-        See <link linkend="sql-grant"><command>GRANT</command></link>
command for their meaning.
-        If the form <literal>\dg+</literal> is used, additional information
-        is shown about each role; currently this adds the comment for each
-        role.
+        </para>
+        <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>
         </listitem>
       </varlistentry>

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

I'll need to update the Role Graph View to add the spaces and swap the
order of the "s" and "i" symbols.

David J.

---

From: Pavel Luzanov <[email-masked]>
Date: Fri, 10 Mar 2023 15:06:04 +0300

On 08.03.2023 05:31, David G. Johnston wrote:
> 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.

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

> My suggestion for the docs is below.

> +        <literal>a</literal> for admin option, <literal>i</literal> 
> for inherit option,
> +        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.

Thank you for review. I will definitely work on the new version, but 
unfortunately and with a high probability it will happen after March 20.

---

From: Pavel Luzanov <[email-masked]>
Date: Fri, 10 Mar 2023 15:18:26 +0300

On 08.03.2023 00:02, David G. Johnston wrote:
>
> FWIW I've finally gotten to publishing my beta version of the Role 
> Graph for PostgreSQL pseudo-extension I'd been talking about:
>

Great. So far I've looked very briefly, but it's interesting.

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

I don't plan to do that. Right now I don't have enough time and 
experience. This requires an experienced developer.

---

From: Pavel Luzanov <[email-masked]>
Date: Mon, 20 Mar 2023 11:49:55 +0300

>> and also to make regress_du_admin a CREATEROLE role with LOGIN.

Please review the attached version 4 with the changes discussed.

-----
Pavel Luzanov
From b67cdf2299b6053ed247254c8be4edde472efdae Mon Sep 17 00:00:00 2001
From: Pavel Luzanov <[email-masked]>
Date: Mon, 20 Mar 2023 11:28:31 +0300
Subject: [PATCH v4] psql: show membership options in the \du command

---
 doc/src/sgml/ref/psql-ref.sgml     | 30 ++++++++++++---
 src/bin/psql/describe.c            | 61 ++++++++++++++++++++----------
 src/test/regress/expected/psql.out | 55 +++++++++++++++++++++++++++
 src/test/regress/sql/psql.sql      | 30 +++++++++++++++
 4 files changed, 150 insertions(+), 26 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 7b8ae9fac3..03e7da93de 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1727,9 +1727,18 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
         <literal>S</literal> modifier to include system roles.
         If <replaceable class="parameter">pattern</replaceable> is specified,
         only those roles whose names match the pattern are listed.
-        If the form <literal>\dg+</literal> is used, additional information
-        is shown about each role; currently this adds the comment for each
-        role.
+        </para>
+        <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>
         </listitem>
       </varlistentry>
@@ -1969,9 +1978,18 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
         <literal>S</literal> modifier to include system roles.
         If <replaceable class="parameter">pattern</replaceable> is specified,
         only those roles whose names match the pattern are listed.
-        If the form <literal>\du+</literal> is used, additional information
-        is shown about each role; currently this adds the comment for each
-        role.
+        </para>
+        <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>\du+</literal> is used the comment attached to the role is shown.
         </para>
         </listitem>
       </varlistentry>
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 99e28f607e..9f7b7326e9 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3631,24 +3631,42 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 	printfPQExpBuffer(&buf,
 					  "SELECT r.rolname, r.rolsuper, r.rolinherit,\n"
 					  "  r.rolcreaterole, r.rolcreatedb, r.rolcanlogin,\n"
-					  "  r.rolconnlimit, r.rolvaliduntil,\n"
-					  "  ARRAY(SELECT b.rolname\n"
-					  "        FROM pg_catalog.pg_auth_members m\n"
-					  "        JOIN pg_catalog.pg_roles b ON (m.roleid = b.oid)\n"
-					  "        WHERE m.member = r.oid) as memberof");
+					  "  r.rolconnlimit, r.rolvaliduntil, r.rolreplication,\n");
 
-	if (verbose)
-	{
-		appendPQExpBufferStr(&buf, "\n, pg_catalog.shobj_description(r.oid, 'pg_authid') AS description");
-		ncols++;
-	}
 
 	if (pset.sversion >= 90500)
 	{
 		appendPQExpBufferStr(&buf, "\n, r.rolbypassrls");
 	}
 
+	if (verbose)
+	{
+		appendPQExpBufferStr(&buf, "\n, pg_catalog.shobj_description(r.oid, 'pg_authid') AS description");
+		ncols++;
+	}
+
 	appendPQExpBufferStr(&buf, "\nFROM pg_catalog.pg_roles r\n");
 
 	if (!showSystem && !pattern)
@@ -3701,13 +3719,6 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 		if (strcmp(PQgetvalue(res, i, 5), "t") != 0)
 			add_role_attribute(&buf, _("Cannot login"));
 
-		if (strcmp(PQgetvalue(res, i, (verbose ? 10 : 9)), "t") == 0)
-			add_role_attribute(&buf, _("Replication"));
-
-		if (pset.sversion >= 90500)
-			if (strcmp(PQgetvalue(res, i, (verbose ? 11 : 10)), "t") == 0)
-				add_role_attribute(&buf, _("Bypass RLS"));
-
 		conns = atoi(PQgetvalue(res, i, 6));
 		if (conns >= 0)
 		{
@@ -3735,10 +3746,20 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)

-		printTableAddCell(&cont, PQgetvalue(res, i, 8), false, false);
+		if (strcmp(PQgetvalue(res, i, 8), "t") == 0)
+			add_role_attribute(&buf, _("Replication"));
+
+		printTableAddCell(&cont, PQgetvalue(res, i, 9), false, false);
+
+		if (pset.sversion >= 90500)
+			if (strcmp(PQgetvalue(res, i, 10), "t") == 0)
+				add_role_attribute(&buf, _("Bypass RLS"));
 
 		if (verbose)
-			printTableAddCell(&cont, PQgetvalue(res, i, 9), false, false);
+			if (pset.sversion >= 90500)
+				printTableAddCell(&cont, PQgetvalue(res, i, 11), false, false);
+			else
+				printTableAddCell(&cont, PQgetvalue(res, i, 10), false, false);
 	}
 	termPQExpBuffer(&buf);
 
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index c00e28361c..b041922410 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -6607,3 +6607,58 @@ cross-database references are not implemented: "no.such.database"."no.such.schem
 cross-database references are not implemented: "no.such.database"."no.such.schema"."no.such.data.type"
 \dX "no.such.database"."no.such.schema"."no.such.extended.statistics"
 cross-database references are not implemented: "no.such.database"."no.such.schema"."no.such.extended.statistics"
+-- check \du
+CREATE ROLE regress_du_role0;
+CREATE ROLE regress_du_role1;
+CREATE ROLE regress_du_role2;
+CREATE ROLE regress_du_admin LOGIN CREATEROLE;
+COMMENT ON ROLE regress_du_role0 IS 'Description for regress_du_role0';
+COMMENT ON ROLE regress_du_role1 IS 'Description for regress_du_role1';
+COMMENT ON ROLE regress_du_role2 IS 'Description for regress_du_role2';
+COMMENT ON ROLE regress_du_admin IS 'Description for regress_du_admin';
+GRANT regress_du_role0 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role1 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role2 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN TRUE,  INHERIT TRUE,  SET TRUE  GRANTED BY regress_du_admin;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN TRUE,  INHERIT FALSE, SET FALSE GRANTED BY regress_du_admin;
+GRANT regress_du_role1 TO regress_du_role2 WITH ADMIN TRUE , INHERIT FALSE, SET TRUE  GRANTED BY regress_du_admin;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN FALSE, INHERIT TRUE,  SET FALSE GRANTED BY regress_du_role1;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN FALSE, INHERIT TRUE , SET TRUE  GRANTED BY regress_du_role1;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN FALSE, INHERIT FALSE, SET TRUE  GRANTED BY regress_du_role2;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN FALSE, INHERIT FALSE, SET FALSE GRANTED BY regress_du_role2;
+\du regress_du_admin|regress_du_role*
+                                   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)
+ regress_du_role0 | Cannot login | 
+ regress_du_role1 | Cannot login | regress_du_role0 from regress_du_admin (a, i, s)+
+                  |              | regress_du_role0 from regress_du_role1 (i)      +
+                  |              | regress_du_role0 from regress_du_role2 (s)
+ regress_du_role2 | Cannot login | regress_du_role0 from regress_du_admin (a)      +
+                  |              | regress_du_role0 from regress_du_role1 (i, s)   +
+                  |              | regress_du_role0 from regress_du_role2 (empty)  +
+                  |              | regress_du_role1 from regress_du_admin (a, s)
+
+\du+ regress_du_admin|regress_du_role*
+                                                     List of roles
+    Role name     |  Attributes  |                    Member of                     |           Description            
+------------------+--------------+--------------------------------------------------+----------------------------------
+ regress_du_admin | Create role  | regress_du_role0 from pal (a, i, s)             +| Description for regress_du_admin
+                  |              | regress_du_role1 from pal (a, i, s)             +| 
+                  |              | regress_du_role2 from pal (a, i, s)              | 
+ regress_du_role0 | Cannot login |                                                  | Description for regress_du_role0
+ regress_du_role1 | Cannot login | regress_du_role0 from regress_du_admin (a, i, s)+| Description for regress_du_role1
+                  |              | regress_du_role0 from regress_du_role1 (i)      +| 
+                  |              | regress_du_role0 from regress_du_role2 (s)       | 
+ regress_du_role2 | Cannot login | regress_du_role0 from regress_du_admin (a)      +| Description for regress_du_role2
+                  |              | regress_du_role0 from regress_du_role1 (i, s)   +| 
+                  |              | regress_du_role0 from regress_du_role2 (empty)  +| 
+                  |              | regress_du_role1 from regress_du_admin (a, s)    | 
+
+DROP ROLE regress_du_role0;
+DROP ROLE regress_du_role1;
+DROP ROLE regress_du_role2;
+DROP ROLE regress_du_admin;
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 961783d6ea..9dc1da432f 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1815,3 +1815,33 @@ DROP FUNCTION psql_error;
 \dP "no.such.database"."no.such.schema"."no.such.partitioned.relation"
 \dT "no.such.database"."no.such.schema"."no.such.data.type"
 \dX "no.such.database"."no.such.schema"."no.such.extended.statistics"
+
+-- check \du
+CREATE ROLE regress_du_role0;
+CREATE ROLE regress_du_role1;
+CREATE ROLE regress_du_role2;
+CREATE ROLE regress_du_admin LOGIN CREATEROLE;
+COMMENT ON ROLE regress_du_role0 IS 'Description for regress_du_role0';
+COMMENT ON ROLE regress_du_role1 IS 'Description for regress_du_role1';
+COMMENT ON ROLE regress_du_role2 IS 'Description for regress_du_role2';
+COMMENT ON ROLE regress_du_admin IS 'Description for regress_du_admin';
+
+GRANT regress_du_role0 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role1 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role2 TO regress_du_admin WITH ADMIN TRUE;
+
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN TRUE,  INHERIT TRUE,  SET TRUE  GRANTED BY regress_du_admin;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN TRUE,  INHERIT FALSE, SET FALSE GRANTED BY regress_du_admin;
+GRANT regress_du_role1 TO regress_du_role2 WITH ADMIN TRUE , INHERIT FALSE, SET TRUE  GRANTED BY regress_du_admin;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN FALSE, INHERIT TRUE,  SET FALSE GRANTED BY regress_du_role1;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN FALSE, INHERIT TRUE , SET TRUE  GRANTED BY regress_du_role1;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN FALSE, INHERIT FALSE, SET TRUE  GRANTED BY regress_du_role2;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN FALSE, INHERIT FALSE, SET FALSE GRANTED BY regress_du_role2;
+
+\du regress_du_admin|regress_du_role*
+\du+ regress_du_admin|regress_du_role*
+
+DROP ROLE regress_du_role0;
+DROP ROLE regress_du_role1;
+DROP ROLE regress_du_role2;
+DROP ROLE regress_du_admin;
-- 
2.34.1

---

From: Pavel Luzanov <[email-masked]>
Date: Tue, 21 Mar 2023 06:37:22 +0300

>>> 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
From b8f35733126a843edd47a1f89da0d9f8babeec93 Mon Sep 17 00:00:00 2001
From: Pavel Luzanov <[email-masked]>
Date: Tue, 21 Mar 2023 05:58:31 +0300
Subject: [PATCH v5] psql: show membership options in the \du command

Format for the "Member of" column completely redesigned.
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:
a for admin option, i for inherit option,
s for set option.) The word 'empty' is printed in
the case that none of those permissions are granted.
---
 doc/src/sgml/ref/psql-ref.sgml     | 30 ++++++++++++---
 src/bin/psql/describe.c            | 61 ++++++++++++++++++++----------
 src/test/regress/expected/psql.out | 49 ++++++++++++++++++++++++
 src/test/regress/sql/psql.sql      | 30 +++++++++++++++
 4 files changed, 144 insertions(+), 26 deletions(-)

-	if (verbose)
-	{

+	if (verbose)
+	{

 

 		if (verbose)
 
index c00e28361c..d5f31b2082 100644
@@ -6607,3 +6607,52 @@ cross-database references are not implemented: "no.such.database"."no.such.schem
index 961783d6ea..1e8d46882b 100644

---

From: Pavel Luzanov <[email-masked]>
Date: Wed, 22 Mar 2023 21:11:16 +0300

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

-----
Pavel Luzanov
From 1b8b5743df23637b70e8d4ad0df0e1f892c595f3 Mon Sep 17 00:00:00 2001
From: Pavel Luzanov <[email-masked]>
Date: Wed, 22 Mar 2023 20:54:41 +0300
Subject: [PATCH v6] psql: show membership options in the \du command

Format for the "Member of" column completely redesigned.
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:
a for admin option, i for inherit option, s for set option.)
The word 'empty' is printed in the case that none of those permissions are granted.
---
 doc/src/sgml/ref/psql-ref.sgml     | 30 +++++++++++---
 src/bin/psql/describe.c            | 63 ++++++++++++++++++++----------
 src/test/regress/expected/psql.out | 49 +++++++++++++++++++++++
 src/test/regress/sql/psql.sql      | 30 ++++++++++++++
 4 files changed, 146 insertions(+), 26 deletions(-)

index 99e28f607e..7f2b7c9363 100644
 
-	if (verbose)
-	{

+	if (verbose)
+	{

@@ -3735,10 +3746,22 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)

 
 		if (verbose)

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 3 Apr 2023 15:06:02 -0700

> warning. Fixed in version 6.
I've marked this Ready for Committer.

My opinion is that this is a necessary modification due to the
already-committed changes to the membership grant implementation and so
only needs to be accepted prior to v16 going live, not feature freeze.

I've added Robert to this thread as the committer of said changes.

David J.

---

From: Tom Lane <[email-masked]>
Date: Tue, 04 Apr 2023 12:13:10 -0400

> 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

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 4 Apr 2023 09:38:04 -0700

> could be further shortened to "(ais)" or the like.

I am on board with removing the comma and space between the specifiers.  My
particular issue with the condensed form is readability, especially the
lowercase "i".  We aren't so desperate for horizontal space here that
compaction seems particularly desirable.

>
> Also, the patch is ignoring i18n issues.

Fair point.

>   I suppose if we stick with
> said single-letter abbreviations we'd not translate them,

Correct.  I don't see this being a huge issue - the abbreviations are the
first letter of the various option "keywords" specified in the syntax.

> but the
Leveraging position and some optional symbols for readability, and sticking
with the premise that abbreviations down to the first letter of the
relevant syntax keyword is OK:

rolename [g: grantor_role] (ais)

I don't have any ideas regarding i18n concerns besides avoiding them by not
using words...but I'd much prefer "from" and just hope the equivalent in
other languages is just as understandable.

I'd rather have the above than go and fully try to emulate ACL presentation
just to avoid i18n issues.

David J.

---

From: Robert Haas <[email-masked]>
Date: Tue, 4 Apr 2023 12:39:43 -0400

> 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

---

From: Tom Lane <[email-masked]>
Date: Tue, 04 Apr 2023 13:12:33 -0400

> spell out the words, then it gets bulky. But maybe bulky is better

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

---

From: Robert Haas <[email-masked]>
Date: Tue, 4 Apr 2023 13:29:10 -0400

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

---

From: Tom Lane <[email-masked]>
Date: Tue, 04 Apr 2023 13:37:46 -0400

> space, both vertical and horizontal.

Right, that's what I was thinking.

> 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

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 4 Apr 2023 12:02:25 -0700

> 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

~140 character width with description

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.

David J.

---

From: Robert Haas <[email-masked]>
Date: Tue, 4 Apr 2023 16:00:43 -0400

On Tue, Apr 4, 2023 at 3:02 PM David G. Johnston
<[email-masked]> wrote:
> 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
>

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

---

From: Pavel Luzanov <[email-masked]>
Date: Tue, 4 Apr 2023 23:42:04 +0300

>> something like regress_du_role0 by regress_du_admin (admin, inherit,

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