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

From: Pavel Luzanov <[email-masked]>
Date: Wed, 5 Apr 2023 10:42:19 +0300

On 04.04.2023 22:02, David G. Johnston wrote:
> On Tue, Apr 4, 2023 at 10:37 AM Tom Lane <[email-masked]> wrote:
>
>     Robert Haas <[email-masked]> writes:
>     > On Tue, Apr 4, 2023 at 1:12 PM Tom Lane <[email-masked]> 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

---

From: Tom Lane <[email-masked]>
Date: Wed, 05 Apr 2023 09:58:31 -0400

> 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

---

From: "David G. Johnston" <[email-masked]>
Date: Wed, 5 Apr 2023 07:24:16 -0700

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

---

From: Pavel Luzanov <[email-masked]>
Date: Thu, 13 Apr 2023 15:44:20 +0300

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 <[email-masked]>
wrote:

Pavel Luzanov <[email-masked]>

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 <[email-masked]>
Date: Thu, 13 Apr 2023 15:09:57 +0300
Subject: [PATCH v7] psql: show membership options in the \du command

---
 doc/src/sgml/ref/psql-ref.sgml     | 40 +++++++++++--
 src/bin/psql/describe.c            | 92 ++++++++++++++++++++++--------
 src/test/regress/expected/psql.out | 49 ++++++++++++++++
 src/test/regress/sql/psql.sql      | 30 ++++++++++
 4 files changed, 182 insertions(+), 29 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc422373d6..f174bddcb0 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1727,9 +1727,23 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
         <literal>S</literal> modifier to include system roles.
         If <replaceable class="parameter">pattern</replaceable> is specified,
         only those roles whose names match the pattern are listed.
-        If the form <literal>\dg+</literal> is used, additional information
-        is shown about each role; currently this adds the comment for each
-        role.
+        </para>
+        <para>
+        If the form <literal>\dg</literal> is used, a list of distinct roles
+        of which it is a member is shown for each role in array format.
+        </para>
+        <para>
+        If the form <literal>\dg+</literal> is used, the memberships granted
+        to the role shown within each row, in newline-separated format.
+        The presentation includes both the name of the grantor as well as
+        the membership permissions: <literal>admin</literal>,
+        <literal>inherit</literal>, <literal>set</literal>.
+        The word <literal>empty</literal> is printed in the case that none of those
+        permissions are granted.
+        See the <link linkend="sql-grant"><command>GRANT</command></link> command for their meaning.
+        </para>
+        <para>
+        Also, if the form <literal>\dg+</literal> is used, the comment attached to the role is shown.
         </para>
         </listitem>
       </varlistentry>
@@ -1969,9 +1983,23 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
         <literal>S</literal> modifier to include system roles.
         If <replaceable class="parameter">pattern</replaceable> is specified,
         only those roles whose names match the pattern are listed.
-        If the form <literal>\du+</literal> is used, additional information
-        is shown about each role; currently this adds the comment for each
-        role.
+        </para>
+        <para>
+        If the form <literal>\du</literal> is used, a list of distinct roles
+        of which it is a member is shown for each role in array format.
+        </para>
+        <para>
+        If the form <literal>\du+</literal> is used, the memberships granted
+        to the role shown within each row, in newline-separated format.
+        The presentation includes both the name of the grantor as well as
+        the membership permissions: <literal>admin</literal>,
+        <literal>inherit</literal>, <literal>set</literal>.
+        The word <literal>empty</literal> is printed in the case that none of those
+        permissions are granted.
+        See the <link linkend="sql-grant"><command>GRANT</command></link> command for their meaning.
+        </para>
+        <para>
+        Also, if the form <literal>\du+</literal> is used, the comment attached to the role is shown.
         </para>
         </listitem>
       </varlistentry>
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 83a37ee601..afdffb6932 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -36,7 +36,9 @@ static bool describeOneTableDetails(const char *schemaname,
 									bool verbose);
 static void add_tablespace_footer(printTableContent *const cont, char relkind,
 								  Oid tablespace, const bool newline);
-static void add_role_attribute(PQExpBuffer buf, const char *const str);
+static void add_role_attribute(PQExpBuffer buf,
+							   const char *const str,
+							   bool verbose);
 static bool listTSParsersVerbose(const char *pattern);
 static bool describeOneTSParser(const char *oid, const char *nspname,
 								const char *prsname);
@@ -3631,24 +3633,56 @@ 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)
+	if (pset.sversion >= 160000)
 	{
-		appendPQExpBufferStr(&buf, "\n, pg_catalog.shobj_description(r.oid, 'pg_authid') AS description");
-		ncols++;
+		if (verbose)
+			appendPQExpBuffer(&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 '%s' END,\n"
+							  "          CASE WHEN m.inherit_option THEN '%s' END,\n"
+							  "          CASE WHEN m.set_option THEN '%s' END),\n"
+							  "      '^$', '%s')\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",
+							  gettext_noop("admin"),
+							  gettext_noop("inherit"),
+							  gettext_noop("set"),
+							  gettext_noop("empty"));
+		else
+			appendPQExpBufferStr(&buf,
+								 "  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\n"
+								 "        ORDER BY 1) AS memberof");
 	}
-	appendPQExpBufferStr(&buf, "\n, r.rolreplication");
+	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 (pset.sversion >= 90500)
 	{
 		appendPQExpBufferStr(&buf, "\n, r.rolbypassrls");
 	}
 
+	if (verbose)
+	{
+		appendPQExpBufferStr(&buf, "\n, pg_catalog.shobj_description(r.oid, 'pg_authid') AS description");
+		ncols++;
+	}
+
 	appendPQExpBufferStr(&buf, "\nFROM pg_catalog.pg_roles r\n");
 
 	if (!showSystem && !pattern)
@@ -3687,26 +3721,26 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 
 		resetPQExpBuffer(&buf);
 		if (strcmp(PQgetvalue(res, i, 1), "t") == 0)
-			add_role_attribute(&buf, _("Superuser"));
+			add_role_attribute(&buf, _("Superuser"), verbose);
 
 		if (strcmp(PQgetvalue(res, i, 2), "t") != 0)
-			add_role_attribute(&buf, _("No inheritance"));
+			add_role_attribute(&buf, _("No inheritance"), verbose);
 
 		if (strcmp(PQgetvalue(res, i, 3), "t") == 0)
-			add_role_attribute(&buf, _("Create role"));
+			add_role_attribute(&buf, _("Create role"), verbose);
 
 		if (strcmp(PQgetvalue(res, i, 4), "t") == 0)
-			add_role_attribute(&buf, _("Create DB"));
+			add_role_attribute(&buf, _("Create DB"), verbose);
 
 		if (strcmp(PQgetvalue(res, i, 5), "t") != 0)
-			add_role_attribute(&buf, _("Cannot login"));
+			add_role_attribute(&buf, _("Cannot login"), verbose);
 
-		if (strcmp(PQgetvalue(res, i, (verbose ? 10 : 9)), "t") == 0)
-			add_role_attribute(&buf, _("Replication"));
+		if (strcmp(PQgetvalue(res, i, 8), "t") == 0)
+			add_role_attribute(&buf, _("Replication"), verbose);
 
 		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, 10), "t") == 0)
+				add_role_attribute(&buf, _("Bypass RLS"), verbose);
 
 		conns = atoi(PQgetvalue(res, i, 6));
 		if (conns >= 0)
@@ -3735,10 +3769,15 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 
 		printTableAddCell(&cont, attr[i], false, false);
 
-		printTableAddCell(&cont, PQgetvalue(res, i, 8), false, false);
+		printTableAddCell(&cont, PQgetvalue(res, i, 9), false, false);
 
 		if (verbose)
-			printTableAddCell(&cont, PQgetvalue(res, i, 9), false, false);
+		{
+			if (pset.sversion >= 90500)
+				printTableAddCell(&cont, PQgetvalue(res, i, 11), false, false);
+			else
+				printTableAddCell(&cont, PQgetvalue(res, i, 10), false, false);
+		}
 	}
 	termPQExpBuffer(&buf);
 
@@ -3754,10 +3793,17 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 }
 
 static void
-add_role_attribute(PQExpBuffer buf, const char *const str)
+add_role_attribute(PQExpBuffer buf,
+				   const char *const str,
+				   bool verbose)
 {
 	if (buf->len > 0)
-		appendPQExpBufferStr(buf, ", ");
+	{
+		if (pset.sversion >= 160000)
+			appendPQExpBufferStr(buf, verbose ? "\n" : ", ");
+		else
+			appendPQExpBufferStr(buf, ", ");
+	}
 
 	appendPQExpBufferStr(buf, str);
 }
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 956e475447..396eaa8495 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -6607,3 +6607,52 @@ cross-database references are not implemented: "no.such.database"."no.such.schem
 cross-database references are not implemented: "no.such.database"."no.such.schema"."no.such.data.type"
 \dX "no.such.database"."no.such.schema"."no.such.extended.statistics"
 cross-database references are not implemented: "no.such.database"."no.such.schema"."no.such.extended.statistics"
+-- check \du
+CREATE ROLE regress_du_role0;
+CREATE ROLE regress_du_role1;
+CREATE ROLE regress_du_role2 LOGIN CREATEROLE BYPASSRLS REPLICATION CREATEDB NOINHERIT CONNECTION LIMIT 3 VALID UNTIL 'infinity';
+CREATE ROLE regress_du_admin LOGIN CREATEROLE;
+COMMENT ON ROLE regress_du_role0 IS 'Description for regress_du_role0';
+COMMENT ON ROLE regress_du_role1 IS 'Description for regress_du_role1';
+COMMENT ON ROLE regress_du_role2 IS 'Description for regress_du_role2';
+COMMENT ON ROLE regress_du_admin IS 'Description for regress_du_admin';
+GRANT regress_du_role0 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role1 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role2 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN TRUE,  INHERIT TRUE,  SET TRUE  GRANTED BY regress_du_admin;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN TRUE,  INHERIT FALSE, SET FALSE GRANTED BY regress_du_admin;
+GRANT regress_du_role1 TO regress_du_role2 WITH ADMIN TRUE , INHERIT FALSE, SET TRUE  GRANTED BY regress_du_admin;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN FALSE, INHERIT TRUE,  SET FALSE GRANTED BY regress_du_role1;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN FALSE, INHERIT TRUE , SET TRUE  GRANTED BY regress_du_role1;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN FALSE, INHERIT FALSE, SET TRUE  GRANTED BY regress_du_role2;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN FALSE, INHERIT FALSE, SET FALSE GRANTED BY regress_du_role2;
+\du regress_du_role*
+                                                      List of roles
+    Role name     |                           Attributes                            |              Member of              
+------------------+-----------------------------------------------------------------+-------------------------------------
+ regress_du_role0 | Cannot login                                                    | {}
+ regress_du_role1 | Cannot login                                                    | {regress_du_role0}
+ regress_du_role2 | No inheritance, Create role, Create DB, Replication, Bypass RLS+| {regress_du_role0,regress_du_role1}
+                  | 3 connections                                                  +| 
+                  | Password valid until infinity                                   | 
+
+\du+ regress_du_role*
+                                                                   List of roles
+    Role name     |          Attributes           |                          Member of                           |           Description            
+------------------+-------------------------------+--------------------------------------------------------------+----------------------------------
+ regress_du_role0 | Cannot login                  |                                                              | Description for regress_du_role0
+ regress_du_role1 | Cannot login                  | regress_du_role0 from regress_du_admin (admin, inherit, set)+| Description for regress_du_role1
+                  |                               | regress_du_role0 from regress_du_role1 (inherit)            +| 
+                  |                               | regress_du_role0 from regress_du_role2 (set)                 | 
+ regress_du_role2 | No inheritance               +| regress_du_role0 from regress_du_admin (admin)              +| Description for regress_du_role2
+                  | Create role                  +| regress_du_role0 from regress_du_role1 (inherit, set)       +| 
+                  | Create DB                    +| regress_du_role0 from regress_du_role2 (empty)              +| 
+                  | Replication                  +| regress_du_role1 from regress_du_admin (admin, set)          | 
+                  | Bypass RLS                   +|                                                              | 
+                  | 3 connections                +|                                                              | 
+                  | Password valid until infinity |                                                              | 
+
+DROP ROLE regress_du_role0;
+DROP ROLE regress_du_role1;
+DROP ROLE regress_du_role2;
+DROP ROLE regress_du_admin;
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 630f638f02..5ef5e40e46 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1815,3 +1815,33 @@ DROP FUNCTION psql_error;
 \dP "no.such.database"."no.such.schema"."no.such.partitioned.relation"
 \dT "no.such.database"."no.such.schema"."no.such.data.type"
 \dX "no.such.database"."no.such.schema"."no.such.extended.statistics"
+
+-- check \du
+CREATE ROLE regress_du_role0;
+CREATE ROLE regress_du_role1;
+CREATE ROLE regress_du_role2 LOGIN CREATEROLE BYPASSRLS REPLICATION CREATEDB NOINHERIT CONNECTION LIMIT 3 VALID UNTIL 'infinity';
+CREATE ROLE regress_du_admin LOGIN CREATEROLE;
+COMMENT ON ROLE regress_du_role0 IS 'Description for regress_du_role0';
+COMMENT ON ROLE regress_du_role1 IS 'Description for regress_du_role1';
+COMMENT ON ROLE regress_du_role2 IS 'Description for regress_du_role2';
+COMMENT ON ROLE regress_du_admin IS 'Description for regress_du_admin';
+
+GRANT regress_du_role0 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role1 TO regress_du_admin WITH ADMIN TRUE;
+GRANT regress_du_role2 TO regress_du_admin WITH ADMIN TRUE;
+
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN TRUE,  INHERIT TRUE,  SET TRUE  GRANTED BY regress_du_admin;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN TRUE,  INHERIT FALSE, SET FALSE GRANTED BY regress_du_admin;
+GRANT regress_du_role1 TO regress_du_role2 WITH ADMIN TRUE , INHERIT FALSE, SET TRUE  GRANTED BY regress_du_admin;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN FALSE, INHERIT TRUE,  SET FALSE GRANTED BY regress_du_role1;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN FALSE, INHERIT TRUE , SET TRUE  GRANTED BY regress_du_role1;
+GRANT regress_du_role0 TO regress_du_role1 WITH ADMIN FALSE, INHERIT FALSE, SET TRUE  GRANTED BY regress_du_role2;
+GRANT regress_du_role0 TO regress_du_role2 WITH ADMIN FALSE, INHERIT FALSE, SET FALSE GRANTED BY regress_du_role2;
+
+\du regress_du_role*
+\du+ regress_du_role*
+
+DROP ROLE regress_du_role0;
+DROP ROLE regress_du_role1;
+DROP ROLE regress_du_role2;
+DROP ROLE regress_du_admin;
-- 
2.34.1

---

From: "Jonathan S. Katz" <[email-masked]>
Date: Wed, 3 May 2023 12:00:05 -0400

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

[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

---

From: "David G. Johnston" <[email-masked]>
Date: Wed, 3 May 2023 09:13:57 -0700

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

---

From: Tom Lane <[email-masked]>
Date: Wed, 03 May 2023 12:25:09 -0400

> 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

---

From: "Jonathan S. Katz" <[email-masked]>
Date: Wed, 3 May 2023 12:30:34 -0400

[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

---

From: "David G. Johnston" <[email-masked]>
Date: Fri, 5 May 2023 09:51:58 -0700

> understand, though do we need to explicitly list "empty" if there are no
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.

---

From: Pavel Luzanov <[email-masked]>
Date: Sun, 7 May 2023 22:14:41 +0300

On 05.05.2023 19:51, David G. Johnston wrote:
> 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

---

From: "Jonathan S. Katz" <[email-masked]>
Date: Wed, 17 May 2023 22:42:40 -0400

On 5/7/23 3:14 PM, Pavel Luzanov wrote:
> ------------------+------------------+---------------------+------------------ 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)  
> 

[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