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

[Jonathan S. Katz]

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



OpenPGP_signature
Description: OpenPGP digital signature

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

[Pavel Luzanov]

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

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

[David G. Johnston]

On Wed, Jul 19, 2023 at 9:47 AM Tom Lane  wrote:

> I wrote:
> > Alvaro Herrera  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.
>
>
Thank You!

David J.

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

[Tom Lane]

I wrote:
> Alvaro Herrera  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

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

[Tom Lane]

Alvaro Herrera  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

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

[Alvaro Herrera]

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/)

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

[Pavel Luzanov]

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
From d25743f87462ef89f9c098f0b907ac1fff473a99 Mon Sep 17 00:00:00 2001
From: Pavel Luzanov 
Date: Fri, 14 Jul 2023 00:08:03 +0300
Subject: [PATCH v10] psql: Add \drg command to show role memberships

New command shows assigned privileges for each membership
ADMIN, INHERIT (added by e3ce2de0) or SET (3d14e171) and
who is grantor. This is important to know
how privileges can be used and who can revoke membership.
Without this command you need to query pg_auth_members directly.

This patch also removes the "Member of" column from the \du & \dg
commands, as it does not provide enough information about membership.
---
 doc/src/sgml/ref/psql-ref.sgml | 21 
 src/bin/psql/command.c |  2 +
 src/bin/psql/describe.c| 87 +-
 src/bin/psql/describe.h|  3 ++
 src/bin/psql/help.c|  1 +
 src/bin/psql/tab-complete.c|  6 ++-
 src/test/regress/expected/psql.out | 46 ++--
 src/test/regress/sql/psql.sql  | 26 +
 8 files changed, 174 insertions(+), 18 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 35aec6d3ce..e09fd06105 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1883,6 +1883,27 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
 
   
 
+
+  
+\drg[S] [ pattern ]
+
+
+Lists detailed information about each role membership, including
+assigned options (ADMIN,
+INHERIT or SET) and grantor.
+See the GRANT
+command for their meaning.
+
+
+By default, only user-created roles are shown; supply the
+S modifier to include system roles.
+If pattern is specified,
+only those roles whose names match the pattern are listed.
+
+
+  
+
+
   
 \drds [ role-pattern [ database-pattern ] ]
 
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 511debbe81..88a653a709 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -918,6 +918,8 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
 
 	free(pattern2);
 }
+else if (strncmp(cmd, "drg", 3) == 0)
+	success = describeRoleGrants(pattern, show_system);
 else
 	status = PSQL_CMD_UNKNOWN;
 break;
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 9325a46b8f..722ba95420 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3617,7 +3617,7 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 	PGresult   *res;
 	printTableContent cont;
 	printTableOpt myopt = pset.popt.topt;
-	int			ncols = 3;
+	int			ncols = 2;
 	int			nrows = 0;
 	int			i;
 	int			conns;
@@ -3631,11 +3631,7 @@ 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\n");
 
 	if (verbose)
 	{
@@ -3675,8 +3671,6 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 
 	printTableAddHeader(&cont, gettext_noop("Role name"), true, align);
 	printTableAddHeader(&cont, gettext_noop("Attributes"), true, align);
-	/* ignores implicit memberships from superuser & pg_database_owner */
-	printTableAddHeader(&cont, gettext_noop("Member of"), true, align);
 
 	if (verbose)
 		printTableAddHeader(&cont, gettext_noop("Description"), true, align);
@@ -3701,11 +3695,11 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 		if (strcmp(PQgetvalue(res, i, 5), "t") != 0)
 			add_role_attribute(&buf,

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

[Tom Lane]

Pavel Luzanov  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

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

[Pavel Luzanov]

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

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

[David G. Johnston]

On Thu, Jul 13, 2023 at 8:01 AM Tom Lane  wrote:

> > 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?
>
>
That replacement is for version 15 and earlier where pam.set_option doesn't
exist at all and the presence of a row here means that set has been granted.

David J.

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

[Tom Lane]

Pavel Luzanov  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

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

[Pavel Luzanov]

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

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

[Pavel Luzanov]

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
From ca303798c14b75cbd0131f850d93c68508ac62e9 Mon Sep 17 00:00:00 2001
From: Pavel Luzanov 
Date: Wed, 12 Jul 2023 13:00:07 +0300
Subject: [PATCH v9] psql: add \drg command to show role memberships and grants
 information.

New command shows assigned privileges for each membership
(ADMIN, INHERIT or SET) and who is grantor. This is important to know
which privileges can be used and how to revoke membership.
Without this command you need to query pg_auth_members directly.

This patch also removes the "Member of" column from the \du & \dg
commands, as it does not provide enough information about membership.
---
 doc/src/sgml/ref/psql-ref.sgml | 21 
 src/bin/psql/command.c |  2 +
 src/bin/psql/describe.c| 87 +-
 src/bin/psql/describe.h|  3 ++
 src/bin/psql/help.c|  1 +
 src/bin/psql/tab-complete.c|  6 ++-
 src/test/regress/expected/psql.out | 46 ++--
 src/test/regress/sql/psql.sql  | 26 +
 8 files changed, 174 insertions(+), 18 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 35aec6d3ce..e09fd06105 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1883,6 +1883,27 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
 
   
 
+
+  
+\drg[S] [ pattern ]
+
+
+Lists detailed information about each role membership, including
+assigned options (ADMIN,
+INHERIT or SET) and grantor.
+See the GRANT
+command for their meaning.
+
+
+By default, only user-created roles are shown; supply the
+S modifier to include system roles.
+If pattern is specified,
+only those roles whose names match the pattern are listed.
+
+
+  
+
+
   
 \drds [ role-pattern [ database-pattern ] ]
 
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 511debbe81..88a653a709 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -918,6 +918,8 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
 
 	free(pattern2);
 }
+else if (strncmp(cmd, "drg", 3) == 0)
+	success = describeRoleGrants(pattern, show_system);
 else
 	status = PSQL_CMD_UNKNOWN;
 break;
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 9325a46b8f..434043594a 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3617,7 +3617,7 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 	PGresult   *res;
 	printTableContent cont;
 	printTableOpt myopt = pset.popt.topt;
-	int			ncols = 3;
+	int			ncols = 2;
 	int			nrows = 0;
 	int			i;
 	int			conns;
@@ -3631,11 +3631,7 @@ 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\n");
 
 	if (verbose)
 	{
@@ -3675,8 +3671,6 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 
 	printTableAddHeader(&cont, gettext_noop("Role name"), true, align);
 	printTableAddHeader(&cont, gettext_noop("Attributes"), true, align);
-	/* ignores implicit memberships from superuser & pg_database_owner */
-	printTableAddHeader(&cont, gettext_noop("Member of"), true, align);
 
 	if (verbose)
 		printTableAddHeader(&cont, gettext_noop("Description"), true, align);
@@ -3701,11 +3695,11 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 		if (strcmp(PQgetvalue(res, i, 5), "t") != 0)
 			add_role_attribute(&buf, _("Cannot login"));
 
-		if (st

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

[Pavel Luzanov]

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.


I understood it that way that the dropping "Member of" column will be 
done as part of another work for v17. [1]

But I'm ready to do it now.


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.


The reason is that \du has translatable all attributes of the role, 
including NOINHERIT.

I decided to make a new command the same way.
But I'm ready to leave them untranslatable, if no objections.


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.


It was David's suggestion:

On 24.06.2023 18:57, David G. Johnston wrote:
On Sat, Jun 24, 2023 at 8:11 AM Pavel Luzanov 
 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.


Personally, I tend to think that left join is not needed. No memberships 
- nothing shown.


So, I accepted all three suggestions. I will wait for other opinions and
plan to implement discussed changes within a few days.

1. https://www.postgresql.org/message-id/4133242.1687481416%40sss.pgh.pa.us

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

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

[Tom Lane]

Pavel Luzanov  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

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

[Pavel Luzanov]

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

--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
From a117f13fd497bf6ff8a504bcda6cb10d34dd22a7 Mon Sep 17 00:00:00 2001
From: Pavel Luzanov 
Date: Mon, 26 Jun 2023 22:10:31 +0300
Subject: [PATCH v8] psql: add \drg command to show role memberships and grants
 information.

New command shows assigned privileges for each membership (ADMIN, INHERIT
or SET) and who is grantor. This is important to know which privileges can
be used and how to revoke membership. Without this command you need to query
pg_auth_members directly.

Since v16 it is possible to include one role to another several times with
different grantors. Therefore, the 'Member of' column of the \du
and \dg commands now shows a sorted array of distinct roles to hide duplicates.
---
 doc/src/sgml/ref/psql-ref.sgml | 27 +-
 src/bin/psql/command.c |  2 +
 src/bin/psql/describe.c| 80 +-
 src/bin/psql/describe.h|  3 ++
 src/bin/psql/help.c|  1 +
 src/bin/psql/tab-complete.c|  6 ++-
 src/test/regress/expected/psql.out | 41 +++
 src/test/regress/sql/psql.sql  | 26 ++
 8 files changed, 180 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 35aec6d3ce..2d2e394a84 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1722,7 +1722,8 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
 Lists database roles.
 (Since the concepts of users and groups have been
 unified into roles, this command is now equivalent to
-\du.)
+\du.) For each role a sorted list of distinct roles
+of which it is a member is shown in array format.
 By default, only user-created roles are shown; supply the
 S modifier to include system roles.
 If pattern is specified,
@@ -1883,6 +1884,27 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
 
   
 
+
+  
+\drg[S] [ pattern ]
+
+
+Lists detailed information about each role membership, including
+assigned options (ADMIN,
+INHERIT or SET) and grantor.
+See the GRANT
+command for their meaning.
+
+
+By default, only user-created roles are shown; supply the
+S modifier to include system roles.
+If pattern is specified,
+only those roles whose names match the pattern are listed.
+
+
+  
+
+
   
 \drds [ role-pattern [ database-pattern ] ]
 
@@ -1957,7 +1979,8 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
 Lists database roles.
 (Since the concepts of users and groups have been
 unified into roles, this command is now equivalent to
-\dg.)
+\dg.) For each role a sorted list of distinct roles
+of which it is a member is shown in array format.
 By default, only user-created roles are shown; supply the
 S modifier to include system roles.
 If pattern is specified,
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 511debbe81..88a653a709 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -918,6 +918,8 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
 
 	free(pattern2);
 }
+else if (strncmp(cmd, "drg", 3) == 0)
+	success = describeRoleGrants(pattern, show_system);
 else
 	status = PSQL_CMD_UNKNOWN;
 break;
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 9325a46b8f..e4101fe36b 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3632,10 +3632,11 @@ 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"
+	  "  ARRAY(SELECT DISTINCT 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");
+	  "WHERE m.member = r.oid\n"
+	  "ORDER BY 1) as memberof");
 
 	if (verbose)
 	{
@@ -3753,6 +3754,81 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 	return true;
 }
 
+/*
+ * \drg
+ * Describes role grants.
+ */
+bool
+describeRoleGrants(const char *pattern, bool showSystem)
+{
+	PQExpBufferData buf;
+	PGresult   *res;
+	printQueryOpt myopt = pset.popt;
+
+	initPQExpBuffer(&buf);
+	printfPQExpBuffer(&buf,
+	  "SELECT m.rolname AS \"%s\", r.rolname AS \"%s\",\n"
+	  "  pg_catalog.concat_ws(', ',\n",
+	  gettext_noop("Role 

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

[Pavel Luzanov]

On 24.06.2023 18:57, David G. Johnston wrote:
On Sat, Jun 24, 2023 at 8:11 AM Pavel Luzanov 
 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.


It's my mistake and inattention. I was thinking about '\du' and '\dg', 
and wrote about '\du' and '\dr'.

I agree that \dr and \drg the best names.
So, now concentrating on implementing \drg.


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


Indeed! I will do so.




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


Ok

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

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

[David G. Johnston]

On Sat, Jun 24, 2023 at 8:11 AM Pavel Luzanov 
wrote:

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

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.

David J.

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

[Pavel Luzanov]

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

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

[David G. Johnston]

On Fri, Jun 23, 2023 at 5:12 PM Tom Lane  wrote:

> "David G. Johnston"  writes:
> > On Thu, Jun 22, 2023 at 5:08 PM Tom Lane  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?
>
>
I haven't given enough thought to that.  My first reaction is that using
blank for old servers would be desirable and then, if allowed in v16+
server, "empty" for those.

That said, the entire grantor premise that motivated this doesn't exist on
those servers so maybe \drg just shouldn't work against pre-v16 servers -
and we keep the existing \du query as-is for those as well while removing
the "member of" column when \du is executed against a v16+ server.

David J.

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

[Tom Lane]

"David G. Johnston"  writes:
> On Thu, Jun 22, 2023 at 5:08 PM Tom Lane  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

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

[Jonathan S. Katz]

On 6/23/23 12:16 PM, Tom Lane wrote:

"David G. Johnston"  writes:

On Thu, Jun 22, 2023 at 5:08 PM Tom Lane  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


OpenPGP_signature
Description: OpenPGP digital signature

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

[Jonathan S. Katz]

On 6/23/23 11:52 AM, David G. Johnston wrote:
On Thu, Jun 22, 2023 at 5:08 PM Tom Lane > wrote:


"Jonathan S. Katz" mailto:jk...@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


OpenPGP_signature
Description: OpenPGP digital signature

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

[Tom Lane]

"David G. Johnston"  writes:
> On Thu, Jun 22, 2023 at 5:08 PM Tom Lane  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

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

[David G. Johnston]

On Thu, Jun 22, 2023 at 5:08 PM Tom Lane  wrote:

> "Jonathan S. Katz"  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.
>

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.

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


> * Parenthetically, the "Attributes" column of \du is a complete
> disaster
>
>
I hadn't thought about this in detail but did get the same impression.

David J.

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

[Tom Lane]

"Jonathan S. Katz"  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

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

[Jonathan S. Katz]

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




OpenPGP_signature
Description: OpenPGP digital signature

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

[David G. Johnston]

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)


On Thu, May 18, 2023 at 6:07 AM Pavel Luzanov 
wrote:

> 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:
>
> I think we should consider the tabular form with translatable headers to
be the acceptable choice here.  I don't see enough value in the sentence
form to make it worth trying to overcome the i18n objection there.

> 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:But is it 
> worth inventing a new psql command for this?
>
>
IMO, no.  I'd much rather read "admin, inherit, set" than deal with
true/false in columns.  I think the newlines are better compared to
repetition of the rolname as well.

I'm also strongly in favor of explicitly writing out the word "empty"
instead of leaving the column blank for the case that no options are marked
true.  But it isn't a show-stopper for me.

David J.

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

[Pavel Luzanov]

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

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

[Jonathan S. Katz]

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


[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



OpenPGP_signature
Description: OpenPGP digital signature

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

[Pavel Luzanov]

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


t.sql
Description: application/sql

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

[David G. Johnston]

On Wed, May 3, 2023 at 9:30 AM Jonathan S. Katz 
wrote:

> [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?
>
>
Yes.  I dislike having the equivalent of null embedded within the output
here.  I would rather label it for what it is.  As a membership without any
attributes has no real purpose I don't see how the choice matters and at
least empty both stands out visually and can be grepped.

The SQL language uses the words "by" and "from" in its syntax; I'm against
avoiding them in our presentation here without a clearly superior
alternative that doesn't require a majority of people to have to translate
the symbol " / " back into the word " by " in order to read the output.

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.

David J.

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

[Jonathan S. Katz]

On 5/3/23 12:25 PM, Tom Lane wrote:

"David G. Johnston"  writes:

On Wed, May 3, 2023 at 9:00 AM Jonathan S. Katz 
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


OpenPGP_signature
Description: OpenPGP digital signature

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

[Tom Lane]

"David G. Johnston"  writes:
> On Wed, May 3, 2023 at 9:00 AM Jonathan S. Katz 
> 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

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

[David G. Johnston]

On Wed, May 3, 2023 at 9:00 AM Jonathan S. Katz 
wrote:

> 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


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.  Or, at worse, we should decide now that we don't
want to do so and spare people the effort of trying to get this committed
later.

David J.

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

[Jonathan S. Katz]

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


OpenPGP_signature
Description: OpenPGP digital signature

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

[Pavel Luzanov]

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

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

[Kyotaro Horiguchi]

Sorry for joining in late..

At Thu, 13 Apr 2023 15:44:20 +0300, Pavel Luzanov  
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

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

[Pavel Luzanov]

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


$ LANGUAGE=fr psql -c "ALTER ROLE postgres CONNECTION LIMIT 3 VALID 
UNTIL 'infinity'" -c '\du'

ALTER ROLE
  Liste des rôles
 Nom du rôle | Attributs    | Membre de
-+-+---
 postgres    | Superutilisateur, Créer un rôle, Créer une base, 
Réplication, Contournement RLS+| {}

 | 3 connexions +|
 | Mot de passe valide jusqu'à 
infinity    |



So I decided to keep the format suggested by David, but without 
abbreviations and only for extended mode.


$ psql -c '\duS+'
 List of roles
  Role name  |  Attributes 
| Member of | Description

-+---+---+-
 pg_checkpoint   | Cannot login 
|   |
 pg_create_subscription  | 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 from postgres (inherit, set)+|
 |   | 
pg_read_all_stats from postgres (inherit, set)   +|
 |   | 
pg_stat_scan_tables from postgres (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 
|   |
 postgres    | Superuser 
+|   |
 | Create role 
+|   |
 | Create DB 
+|   |
 | Replication 
+|   |
 | Bypass RLS 
+|   |
 | 3 connections 
+|   |
 | Password valid until infinity 
|   |



Please look at new version. I understand that this is a compromise choice.
I am ready to change it if a better solution is offered.

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

On 05.04.2023 17:24, David G. Johnston wrote:

On Wed, Apr 5, 2023 at 6:58 AM Tom Lane  wrote:

Pavel Luzanov  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
From 1f9433696e41a8f37cfd4c0514e136fedd50939e Mon Sep 17 00:00:00 2001
From: Pavel Luzanov 
Date: Thu, 13 Apr 2023 15:09:57 +0300
Subject: [PATCH v7]

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

[David G. Johnston]

On Wed, Apr 5, 2023 at 6:58 AM Tom Lane  wrote:

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

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

[Tom Lane]

Pavel Luzanov  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

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

[Pavel Luzanov]

On 04.04.2023 22:02, David G. Johnston wrote:

On Tue, Apr 4, 2023 at 10:37 AM Tom Lane  wrote:

Robert Haas  writes:
> On Tue, Apr 4, 2023 at 1:12 PM Tom Lane  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


Perhaps more closely to syntax?

regress_du_role0 with admin, inherit, set granted by regress_du_admin

instead of

regress_du_role0 granted by regress_du_admin with admin, inherit, set



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

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


+1


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.


From yesterday's discussion, I think two things are important:
- it is advisable to avoid translation,
- there is no sense in the abbreviation (a,i,s), if there are full names 
in the 'attributes' column.


So I agree with such changes and plan to implement them.

And one more question. (I think it's better to have it explicitly 
rejected than to keep silent.)


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

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

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

[Pavel Luzanov]

On 04.04.2023 23:00, Robert Haas wrote:

On Tue, Apr 4, 2023 at 3:02 PM David G. Johnston
 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)?


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


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

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

[Robert Haas]

On Tue, Apr 4, 2023 at 3:02 PM David G. Johnston
 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
>
> ~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

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

[David G. Johnston]

On Tue, Apr 4, 2023 at 10:37 AM Tom Lane  wrote:

> Robert Haas  writes:
> > On Tue, Apr 4, 2023 at 1:12 PM Tom Lane  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

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

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

[Tom Lane]

Robert Haas  writes:
> On Tue, Apr 4, 2023 at 1:12 PM Tom Lane  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

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

[Robert Haas]

On Tue, Apr 4, 2023 at 1:12 PM Tom Lane  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

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

[Tom Lane]

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

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

[Robert Haas]

On Tue, Apr 4, 2023 at 12:13 PM Tom Lane  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. 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

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

[David G. Johnston]

On Tue, Apr 4, 2023 at 9:13 AM Tom Lane  wrote:

> "David G. Johnston"  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.
>

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

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

[Tom Lane]

"David G. Johnston"  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

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

[David G. Johnston]

On Wed, Mar 22, 2023 at 11:11 AM Pavel Luzanov 
wrote:

> In the previous version, I didn't notice (unlike cfbot) the compiler
> 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.

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

[Pavel Luzanov]

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 
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(-)

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
 S modifier to include system roles.
 If pattern is specified,
 only those roles whose names match the pattern are listed.
-If the form \dg+ is used, additional information
-is shown about each role; currently this adds the comment for each
-role.
+
+
+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.
+See the GRANT command for their meaning.
+
+
+If the form \dg+ is used the comment attached to the role is shown.
 
 
   
@@ -1969,9 +1978,18 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
 S modifier to include system roles.
 If pattern is specified,
 only those roles whose names match the pattern are listed.
-If the form \du+ is used, additional information
-is shown about each role; currently this adds the comment for each
-role.
+
+
+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.
+See the GRANT command for their meaning.
+
+
+If the form \du+ is used the comment attached to the role is shown.
 
 
   
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 99e28f607e..7f2b7c9363 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++;
-	}
-	appendPQExpBufferStr(&buf, "\n, r.rolreplication");
+	if (pset.sversion >= 16)
+		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
+		appendPQExpBuffe

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

[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
From b8f35733126a843edd47a1f89da0d9f8babeec93 Mon Sep 17 00:00:00 2001
From: Pavel Luzanov 
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(-)

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
 S modifier to include system roles.
 If pattern is specified,
 only those roles whose names match the pattern are listed.
-If the form \dg+ is used, additional information
-is shown about each role; currently this adds the comment for each
-role.
+
+
+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.
+See the GRANT command for their meaning.
+
+
+If the form \dg+ is used the comment attached to the role is shown.
 
 
   
@@ -1969,9 +1978,18 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
 S modifier to include system roles.
 If pattern is specified,
 only those roles whose names match the pattern are listed.
-If the form \du+ is used, additional information
-is shown about each role; currently this adds the comment for each
-role.
+
+
+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.
+See the GRANT command for their meaning.
+
+
+If the form \du+ is used the comment attached to the role is shown.
 
 
   
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++;
-	}
-	appendPQExpBufferStr(&buf, "\n, r.rolreplication");
+	if (pset.sversion >= 16)
+		appendPQExpBufferStr(&buf,
+			 "  (SELEC

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

[Pavel Luzanov]

On 10.03.2023 15:06, Pavel Luzanov wrote:
I missed the comment at the beginning of the file about version 9.2. I 
will return the version check for rolbypassrls.




+        
+        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.
+        See the linkend="sql-grant">GRANT command for their 
meaning.

+        
+        
+        If the form \dg+ is used the comment 
attached to the role is shown.

         


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
From b67cdf2299b6053ed247254c8be4edde472efdae Mon Sep 17 00:00:00 2001
From: Pavel Luzanov 
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
 S modifier to include system roles.
 If pattern is specified,
 only those roles whose names match the pattern are listed.
-If the form \dg+ is used, additional information
-is shown about each role; currently this adds the comment for each
-role.
+
+
+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.
+See the GRANT command for their meaning.
+
+
+If the form \dg+ is used the comment attached to the role is shown.
 
 
   
@@ -1969,9 +1978,18 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
 S modifier to include system roles.
 If pattern is specified,
 only those roles whose names match the pattern are listed.
-If the form \du+ is used, additional information
-is shown about each role; currently this adds the comment for each
-role.
+
+
+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.
+See the GRANT command for their meaning.
+
+
+If the form \du+ is used the comment attached to the role is shown.
 
 
   
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++;
-	}
-	appendPQExpBufferStr(&buf, "\n, r.rolreplication");
+	if (pset.sversion >= 16)
+		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(', ',

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

[Pavel Luzanov]

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:


https://github.com/polobo/RoleGraphForPostgreSQL


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.


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

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

[Pavel Luzanov]

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.



+        
+        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.
+        See the linkend="sql-grant">GRANT command for their 
meaning.

+        
+        
+        If the form \dg+ is used the comment 
attached to the role is shown.

         


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.


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

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

[David G. Johnston]

On Tue, Mar 7, 2023 at 2:02 PM David G. Johnston 
wrote:

>
> 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
 S modifier to include system roles.
 If pattern 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:
-a — admin option, i
— inherit option,
-s — set option and
empty if no one is set.
-See GRANT
command for their meaning.
-If the form \dg+ is used, additional information
-is shown about each role; currently this adds the comment for each
-role.
+
+
+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.
+See the GRANT
command for their meaning.
+
+
+If the form \dg+ is used the comment attached
to the role is shown.
 
 
   

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.

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

[David G. Johnston]

On Mon, Mar 6, 2023 at 12:43 AM Pavel Luzanov 
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.
>
And it is not very clear whether (except for backward compatibility) a
> 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.

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

[Pavel Luzanov]

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.

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

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

[David G. Johnston]

On Fri, Mar 3, 2023 at 4:01 AM Pavel Luzanov 
wrote:

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

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

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.


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

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


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

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 is one of the goals of the discussion patch.
>

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.

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

[Pavel Luzanov]

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:

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.


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.

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

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

[Pavel Luzanov]

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 
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(-)

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
 S modifier to include system roles.
 If pattern 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:
+a — admin option, i — inherit option,
+s — set option and empty if no one is set.
+See GRANT command for their meaning.
 If the form \dg+ 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
 S modifier to include system roles.
 If pattern 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:
+a — admin option, i — inherit option,
+s — set option and empty if no one is set.
+See GRANT command for their meaning.
 If the form \du+ 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 >= 16)
+		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"

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

[Pavel Luzanov]

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


bob from grantor (a, s, i) \n
adam from postgres (a, s, i) \n
emily from postgres (empty)


I think this is a good compromise.

Based upon prior comments going for something like the following is 
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 
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
 S modifier to include system roles.
 If pattern 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:
+a — admin option, i — inherit option,
+s — set option and empty if no one is set.
+See GRANT command for their meaning.
 If the form \dg+ 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
 S modifier to include system roles.
 If pattern 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:
+a — admin option, i — inherit option,
+s — set option and empty if no one is set.
+See GRANT command for their meaning.
 If the form \du+ 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 >= 16)
+		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)
-	{
-		appendPQExpBuffe

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

[David G. Johnston]

On Tue, Feb 21, 2023 at 2:14 PM Pavel Luzanov 
wrote:

> On 17.02.2023 19:53, David G. Johnston wrote:
>
> On Fri, Feb 17, 2023 at 4:02 AM Pavel Luzanov 
> 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.
>
>
> Ok, I'll try in the next few days. But what presentation format to use?
>
>
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:
>
> 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.

David J.

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

[Pavel Luzanov]

On 17.02.2023 19:53, David G. Johnston wrote:
On Fri, Feb 17, 2023 at 4:02 AM Pavel Luzanov 
 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.


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.


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



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

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

[David G. Johnston]

On Fri, Feb 17, 2023 at 4:02 AM Pavel Luzanov 
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

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

[Pavel Luzanov]

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

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

[Pavel Luzanov]

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

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

[David Zhang]

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


On Wed, Feb 15, 2023 at 2:31 PM David Zhang  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.

Good improvement, +1.

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

[David G. Johnston]

On Wed, Feb 15, 2023 at 2:31 PM David Zhang  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.

David J.

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

[David Zhang]

On 2023-02-10 2:27 p.m., David G. Johnston wrote:

On Fri, Feb 10, 2023 at 2:08 PM David Zhang  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   +|
| 

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

[David G. Johnston]

On Fri, Feb 10, 2023 at 2:08 PM David Zhang  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?

David J.

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

[David Zhang]

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.

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

[Pavel Luzanov]

On 24.01.2023 20:16, David G. Johnston wrote:
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.


Glad to hear that you're working on it.

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.


I agree with all of your arguments. A couple of months I tried to find 
an acceptable variant in the background.

But apparently tried not very hard ))

In the end, the variant proposed in the patch seemed to me worthy to 
show and

start a discussion. But I'm not sure that this is the best choice.

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.


It will be interesting to see the result.

--
Pavel Luzanov

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

[David G. Johnston]

On Mon, Jan 9, 2023 at 9:09 AM 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.
>
>
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.

One thing I did was name the situation where none of the grants are true -
EMPTY.  So: pg_stat_scan_tables WITH EMPTY.

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.  If users really want to build up queries on their own
they should be using the system catalog.  So concise human readability
should be the goal here.  Keeping those two things in mind I would add a
new text[] column to the views with the following possible values in each
cell the meaning of which should be self-evident or this probably isn't a
good approach...

ais
ai
as
a
is
i
s
empty

That said, I do find the newline based output to be quite useful in the
graph query I'm writing and so wouldn't be disappointed if we changed over
to that.  I'd probably stick with abbreviations though.  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.

postgres=# \dgS+ pg_read_all_settings
 List of roles
  Role name   |  Attributes  | Member of | Members | Description
--+--+---+-
 pg_read_all_settings | Cannot login | {}| { pg_monitor }   |

David J.

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

[Pavel Luzanov]

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

Feel free to reject if it's not interesting.

--
Pavel Luzanov

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

[Pavel Luzanov]

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
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 8a5285da9a..ef3e87fa32 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1724,9 +1724,8 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
 S modifier to include system roles.
 If pattern is specified,
 only those roles whose names match the pattern are listed.
-If the form \dg+ is used, additional information
-is shown about each role; currently this adds the comment for each
-role.
+If the form \dg+ is used, each role is listed
+with its associated description and options for each role's membership.
 
 
   
@@ -1964,9 +1963,8 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
 S modifier to include system roles.
 If pattern is specified,
 only those roles whose names match the pattern are listed.
-If the form \du+ is used, additional information
-is shown about each role; currently this adds the comment for each
-role.
+If the form \du+ is used, each role is listed
+with its associated description and options for each role's membership.
 
 
   
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index df166365e8..c7b10f96f3 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3624,11 +3624,29 @@ 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,\n");
+
+	if (verbose)
+	{
+		appendPQExpBufferStr(&buf,
+			 "  (SELECT string_agg (quote_ident(b.rolname) || regexp_replace(\n"
+			 " CASE WHEN m.admin_option THEN ', ADMIN' ELSE '' END ||\n"
+			 " CASE WHEN m.inherit_option THEN ', INHERIT' ELSE '' END ||\n"
+			 " CASE WHEN m.set_option THEN ', SET' ELSE '' END, \n"
+			 " '^,', ' WITH', 1), E'\\n' ORDER BY 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");
+	}
+	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\n"
+			 "