Subject: Things I don't like about \du's "Attributes" column

From: Tom Lane <[email-masked]>
Date: Thu, 22 Jun 2023 20:50:16 -0400

Nearby I dissed psql's \du command for its incoherent "Attributes"
column [1].  It's too late to think about changing that for v16,
but here's some things I think we should consider for v17:

* It seems weird that some attributes are described in the negative
("Cannot login", "No inheritance").  I realize that this corresponds
to the defaults, so that a user created by CREATE USER with no options
shows nothing in the Attributes column; but I wonder how much that's
worth.  As far as "Cannot login" goes, you could argue that the silent
default ought to be for the properties assigned by CREATE ROLE, since
the table describes itself as "List of roles".  I'm not dead set on
changing this, but it seems like a topic that deserves a fresh look.

* I do not like the display of rolconnlimit, ie "No connections" or
"%d connection(s)".  A person not paying close attention might think
that that means the number of *current* connections the user has.
A minimal fix could be to word it like "No connections allowed" or
"%d connection(s) allowed".  But see below.

* I do not like the display of rolvaliduntil, either.  Consider

regression=# create user alice password 'secret';
CREATE ROLE
regression=# create user bob valid until 'infinity';
CREATE ROLE
regression=# \du
...
 alice     |
 bob       | Password valid until infinity
...

This output claims that bob has an indefinitely-valid password, when in
fact he has no password at all.  On the other hand, nothing is said about
alice, who actually *does* have a password valid until infinity.  It's
difficult to imagine a more misleading way to present this.

Now, it's hard to do better given that the \du command is examining the
universally-readable pg_roles view, because that doesn't betray any hint
of whether the user has a password or not.  I wonder though what is the
rationale for letting unprivileged users see the rolvaliduntil column
but not whether a password exists at all.  I suggest that maybe it'd
be okay to change the pg_roles view along the lines of

-       '********'::text as rolpassword,
+       case when rolpassword is not null then '********'::text end as rolpassword,

Then we could fix \du to say nothing if rolpassword is null,
and when it isn't, print "Password valid until infinity" whenever
that is the case (ie, rolvaliduntil is null or infinity).

* On a purely presentation level, how did we possibly arrive
at the idea that the connection-limit and valid-until properties
deserve their own lines in the Attributes column while the other
properties are comma-separated?  That makes no sense whatsoever,
nor does it look nice in \x display format.

I do grasp the distinction that the other properties are permission
bits while these two aren't, but that doesn't naturally lead to
this formatting.  I'd vote for either

(a) each property gets its own line, or

(b) move these two things into separate columns.  Some of the
verbiage could then be dropped in favor of the column title.

Right now (b) would lead to an undesirably wide table; but
if we push the "Member of" column out to a different \d command
as discussed in the other thread, maybe it'd be practical.

Anyway, for now I'm just throwing this topic out for discussion.

			regards, tom lane

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

---

From: Pavel Luzanov <[email-masked]>
Date: Sat, 24 Jun 2023 18:16:04 +0300

> but here's some things I think we should consider for v17:

If there are no others willing, I am ready to take up this topic. There 
is definitely room for improvement here.
But first I want to finish with the \du command.

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

---

From: Pavel Luzanov <[email-masked]>
Date: Mon, 10 Jul 2023 16:10:45 +0300

> nor does it look nice in \x display format.

I think this a reason why footer property explicitly disabled in the output.
As part of reworking footer should be enabled, as it worked for other 
meta-commands.

Just to don't forget.

---

From: Pavel Luzanov <[email-masked]>
Date: Sat, 30 Dec 2023 17:23:40 +0300

Now I'm ready for discussion.

> changing this, but it seems like a topic that deserves a fresh look.

Agree. The negative form looks strange.

Fresh look suggests to move login attribute to own column.
The attribute separates users and group roles, this is very important 
information,
it deserves to be placed in a separate column. Of course, it can be
returned back to "Attributes" if such change is very radical.

On the other hand, rolinherit attribute lost its importance in v16.
I don't see serious reasons in changing the default value, so we can 
leave it
in the "Attributes" column. In most cases it will be hidden.

> "%d connection(s) allowed".  But see below.

connlimit attribute moved from "Attributes" column to separate column
"Max connections" in extended mode. But without any modifications to 
it's values.
For me it looks normal.

> * I do not like the display of rolvaliduntil, either.

Moved from "Attributes" column to separate column  "Password expire time"
in extended mode (+).

> I suggest that maybe it'd

Done.
The same changes to pg_user.passwd for consistency.
> that is the case (ie, rolvaliduntil is null or infinity).

I think that writing the value "infinity" in places where there is no 
value is
not a good thing. This hides the real value of the column. In addition,
there is no reason to set "infinity" when the password is always valid with
default NULL.

My suggestion to add new column "Has password?" in extended mode with
yes/no values and leave rolvaliduntil values as is.

> (b) move these two things into separate columns.

Implemented this approach.

In a result describeRoles function significantly simplified and 
rewritten for the convenience
of printing the whole query result. All the magic of building 
"Attributes" column
moved to SELECT statement for easy viewing by users via ECHO_HIDDEN 
variable.

Here is an example output.

--DROP ROLE alice, bob, charlie, admin;

CREATE ROLE alice LOGIN SUPERUSER NOINHERIT PASSWORD 'alice' VALID UNTIL 'infinity' CONNECTION LIMIT 5;
CREATE ROLE bob LOGIN REPLICATION BYPASSRLS CREATEDB VALID UNTIL '2022-01-01';
CREATE ROLE charlie LOGIN CREATEROLE PASSWORD 'charlie' CONNECTION LIMIT 0;
CREATE ROLE admin;

COMMENT ON ROLE alice IS 'Superuser but with connection limit and with no inheritance';
COMMENT ON ROLE bob IS 'No password but with expire time';
COMMENT ON ROLE charlie IS 'No connections allowed';
COMMENT ON ROLE admin IS 'Group role without login';

Master.
=# \du
                              List of roles
  Role name |                         Attributes
-----------+------------------------------------------------------------
  admin     | Cannot login
  alice     | Superuser, No inheritance                                 +
            | 5 connections                                             +
            | Password valid until infinity
  bob       | Create DB, Replication, Bypass RLS                        +
            | Password valid until 2022-01-01 00:00:00+03
  charlie   | Create role                                               +
            | No connections
  postgres  | Superuser, Create role, Create DB, Replication, Bypass RLS

=# \du+
                                                             List of roles
  Role name |                         Attributes                         |                         Description
-----------+------------------------------------------------------------+-------------------------------------------------------------
  admin     | Cannot login                                               | Group role without login
  alice     | Superuser, No inheritance                                 +| Superuser but with connection limit and with no inheritance
            | 5 connections                                             +|
            | Password valid until infinity                              |
  bob       | Create DB, Replication, Bypass RLS                        +| No password but with expire time
            | Password valid until 2022-01-01 00:00:00+03                |
  charlie   | Create role                                               +| No connections allowed
            | No connections                                             |
  postgres  | Superuser, Create role, Create DB, Replication, Bypass RLS |

Patched.
=# \du
                                     List of roles
  Role name | Can login? |                         Attributes
-----------+------------+------------------------------------------------------------
  admin     | no         |
  alice     | yes        | Superuser, No inheritance
  bob       | yes        | Create DB, Replication, Bypass RLS
  charlie   | yes        | Create role
  postgres  | yes        | Superuser, Create role, Create DB, Replication, Bypass RLS
(5 rows)

=# \du+
                                                                                                 List of roles
  Role name | Can login? |                         Attributes                         | Has password? |  Password expire time  | Max connections |                         Description
-----------+------------+------------------------------------------------------------+---------------+------------------------+-----------------+-------------------------------------------------------------
  admin     | no         |                                                            | no            |                        |              -1 | Group role without login
  alice     | yes        | Superuser, No inheritance                                  | yes           | infinity               |               5 | Superuser but with connection limit and with no inheritance
  bob       | yes        | Create DB, Replication, Bypass RLS                         | no            | 2022-01-01 00:00:00+03 |              -1 | No password but with expire time
  charlie   | yes        | Create role                                                | yes           |                        |               0 | No connections allowed
  postgres  | yes        | Superuser, Create role, Create DB, Replication, Bypass RLS | yes           |                        |              -1 |
(5 rows)

v1 of the patch attached. There are no tests and documentation yet.
make check fall in 2 existing tests due changes in pg_roles and \du. 
Will be corrected.

Any opinions?

I plan to add a patch to the January commitfest.

-- 
Pavel Luzanov
Postgres Professional:https://postgrespro.com
From 78caf093e68d6166477baf59934d53f50103836a Mon Sep 17 00:00:00 2001
From: Pavel Luzanov <[email-masked]>
Date: Sat, 30 Dec 2023 15:48:18 +0300
Subject: [PATCH v1] psql: Rethinking of \du command

Changes:
* login attribute moved from "Attributes" column to separate column
  "Can login?" with yes/no values.
* connlimit attribute moved from "Attributes" column to separate column
  "Max connections" in extended mode(+).
* 'valid until' attribute moved from "Attributes" column to separate
   column "Password expire time" in extended mode.
* Added new column "Has password?" in extended mode based on changed
  pg_roles.rolpassword. The rolpassword column of the pg_roles view
  modified so that it is easy to identify the presence of a password.
  The same changes made for pg_user.passwd for consistency.
* describeRoles function rewritten for the convenience of printing
  the whole query result. All the magic of building "Attributes" column
  moved to SELECT statement for easy viewing by users via ECHO_HIDDEN
  variable.

Per suggestions from Tom Lane.

Author: Pavel Luzanov
Discussion: https://www.postgresql.org/message-id/4133242.1687481416%40sss.pgh.pa.us
---
 src/backend/catalog/system_views.sql |   6 +-
 src/bin/psql/describe.c              | 146 ++++++++-------------------
 2 files changed, 43 insertions(+), 109 deletions(-)

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 058fc47c91..fed221f787 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -24,10 +24,10 @@ CREATE VIEW pg_roles AS
         rolcanlogin,
         rolreplication,
         rolconnlimit,
-        '********'::text as rolpassword,
+        CASE WHEN rolpassword IS NOT NULL THEN '********'::text END AS rolpassword,
         rolvaliduntil,
         rolbypassrls,
-        setconfig as rolconfig,
+        setconfig AS rolconfig,
         pg_authid.oid
     FROM pg_authid LEFT JOIN pg_db_role_setting s
     ON (pg_authid.oid = setrole AND setdatabase = 0);
@@ -65,7 +65,7 @@ CREATE VIEW pg_user AS
         usesuper,
         userepl,
         usebypassrls,
-        '********'::text as passwd,
+        CASE WHEN passwd IS NOT NULL THEN '********'::text END AS passwd,
         valuntil,
         useconfig
     FROM pg_shadow;
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 5077e7b358..b6ccb013aa 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -36,7 +36,6 @@ 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 bool listTSParsersVerbose(const char *pattern);
 static bool describeOneTSParser(const char *oid, const char *nspname,
 								const char *prsname);
@@ -3654,34 +3653,49 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 {
 	PQExpBufferData buf;
 	PGresult   *res;
-	printTableContent cont;
-	printTableOpt myopt = pset.popt.topt;
-	int			ncols = 2;
-	int			nrows = 0;
-	int			i;
-	int			conns;
-	const char	align = 'l';
-	char	  **attr;
-
-	myopt.default_footer = false;
+	printQueryOpt myopt = pset.popt;
 
 	initPQExpBuffer(&buf);
-
 	printfPQExpBuffer(&buf,
-					  "SELECT r.rolname, r.rolsuper, r.rolinherit,\n"
-					  "  r.rolcreaterole, r.rolcreatedb, r.rolcanlogin,\n"
-					  "  r.rolconnlimit, r.rolvaliduntil");
+					  "SELECT r.rolname AS \"%s\",\n"
+					  "  CASE WHEN r.rolcanlogin THEN '%s' ELSE '%s' END AS \"%s\",\n"
+					  "  pg_catalog.concat_ws(', ',\n"
+					  "    CASE WHEN r.rolsuper THEN '%s' END,\n"
+					  "    CASE WHEN NOT r.rolinherit THEN '%s' END,\n"
+					  "    CASE WHEN r.rolcreaterole THEN '%s' END,\n"
+					  "    CASE WHEN r.rolcreatedb THEN '%s' END,\n"
+					  "    CASE WHEN r.rolreplication THEN '%s' END",
+					  gettext_noop("Role name"),
+					  gettext_noop("yes"), gettext_noop("no"),
+					  gettext_noop("Can login?"),
+					  gettext_noop("Superuser"),
+					  gettext_noop("No inheritance"),
+					  gettext_noop("Create role"),
+					  gettext_noop("Create DB"),
+					  gettext_noop("Replication"));
+
+	if (pset.sversion >= 90500)
+		appendPQExpBuffer(&buf,
+						  ",\n    CASE WHEN r.rolbypassrls THEN '%s' END",
+						  gettext_noop("Bypass RLS"));
+
+	appendPQExpBuffer(&buf, "\n  ) AS \"%s\"", gettext_noop("Attributes"));
 
 	if (verbose)
 	{
-		appendPQExpBufferStr(&buf, "\n, pg_catalog.shobj_description(r.oid, 'pg_authid') AS description");
-		ncols++;
-	}
-	appendPQExpBufferStr(&buf, "\n, r.rolreplication");
+		if (pset.sversion >= 170000)
+			appendPQExpBuffer(&buf,
+							  ",\n  CASE WHEN r.rolpassword IS NULL THEN '%s' ELSE '%s' END AS \"%s\"",
+							  gettext_noop("no"), gettext_noop("yes"),
+							  gettext_noop("Has password?"));
 
-	if (pset.sversion >= 90500)
-	{
-		appendPQExpBufferStr(&buf, "\n, r.rolbypassrls");
+		appendPQExpBuffer(&buf,
+						  ",\n  r.rolvaliduntil AS \"%s\",\n"
+						  "  r.rolconnlimit AS \"%s\",\n"
+						  "  pg_catalog.shobj_description(r.oid, 'pg_authid') AS \"%s\"",
+						  gettext_noop("Password expire time"),
+						  gettext_noop("Max connections"),
+						  gettext_noop("Description"));
 	}
 
 	appendPQExpBufferStr(&buf, "\nFROM pg_catalog.pg_roles r\n");
@@ -3700,99 +3714,19 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 	appendPQExpBufferStr(&buf, "ORDER BY 1;");
 
 	res = PSQLexec(buf.data);
+	termPQExpBuffer(&buf);
 	if (!res)
 		return false;
 
-	nrows = PQntuples(res);
-	attr = pg_malloc0((nrows + 1) * sizeof(*attr));
-
-	printTableInit(&cont, &myopt, _("List of roles"), ncols, nrows);
-
-	printTableAddHeader(&cont, gettext_noop("Role name"), true, align);
-	printTableAddHeader(&cont, gettext_noop("Attributes"), true, align);
-
-	if (verbose)
-		printTableAddHeader(&cont, gettext_noop("Description"), true, align);
-
-	for (i = 0; i < nrows; i++)
-	{
-		printTableAddCell(&cont, PQgetvalue(res, i, 0), false, false);
-
-		resetPQExpBuffer(&buf);
-		if (strcmp(PQgetvalue(res, i, 1), "t") == 0)
-			add_role_attribute(&buf, _("Superuser"));
-
-		if (strcmp(PQgetvalue(res, i, 2), "t") != 0)
-			add_role_attribute(&buf, _("No inheritance"));
-
-		if (strcmp(PQgetvalue(res, i, 3), "t") == 0)
-			add_role_attribute(&buf, _("Create role"));
-
-		if (strcmp(PQgetvalue(res, i, 4), "t") == 0)
-			add_role_attribute(&buf, _("Create DB"));
-
-		if (strcmp(PQgetvalue(res, i, 5), "t") != 0)
-			add_role_attribute(&buf, _("Cannot login"));
-
-		if (strcmp(PQgetvalue(res, i, (verbose ? 9 : 8)), "t") == 0)
-			add_role_attribute(&buf, _("Replication"));
-
-		if (pset.sversion >= 90500)
-			if (strcmp(PQgetvalue(res, i, (verbose ? 10 : 9)), "t") == 0)
-				add_role_attribute(&buf, _("Bypass RLS"));
-
-		conns = atoi(PQgetvalue(res, i, 6));
-		if (conns >= 0)
-		{
-			if (buf.len > 0)
-				appendPQExpBufferChar(&buf, '\n');
-
-			if (conns == 0)
-				appendPQExpBufferStr(&buf, _("No connections"));
-			else
-				appendPQExpBuffer(&buf, ngettext("%d connection",
-												 "%d connections",
-												 conns),
-								  conns);
-		}
-
-		if (strcmp(PQgetvalue(res, i, 7), "") != 0)
-		{
-			if (buf.len > 0)
-				appendPQExpBufferChar(&buf, '\n');
-			appendPQExpBufferStr(&buf, _("Password valid until "));
-			appendPQExpBufferStr(&buf, PQgetvalue(res, i, 7));
-		}
-
-		attr[i] = pg_strdup(buf.data);
-
-		printTableAddCell(&cont, attr[i], false, false);
-
-		if (verbose)
-			printTableAddCell(&cont, PQgetvalue(res, i, 8), false, false);
-	}
-	termPQExpBuffer(&buf);
-
-	printTable(&cont, pset.queryFout, false, pset.logfile);
-	printTableCleanup(&cont);
+	myopt.title = _("List of roles");
+	myopt.translate_header = true;
 
-	for (i = 0; i < nrows; i++)
-		free(attr[i]);
-	free(attr);
+	printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
 
 	PQclear(res);
 	return true;
 }
 
-static void
-add_role_attribute(PQExpBuffer buf, const char *const str)
-{
-	if (buf->len > 0)
-		appendPQExpBufferStr(buf, ", ");
-
-	appendPQExpBufferStr(buf, str);
-}
-
 /*
  * \drds
  */
-- 
2.34.1

---

From: Isaac Morland <[email-masked]>
Date: Sat, 30 Dec 2023 09:33:59 -0500

> there is no reason to set "infinity" when the password is always valid with

Would it make sense to make the column non-nullable and always set it to
infinity when there is no expiry?

In this case, I think NULL simply means infinity, so why not write that? If
the timestamp type didn't have infinity, then NULL would be a natural way
of saying that there is no expiry, but with infinity as a possible value, I
don't see any reason to think of no expiry as being the absence of an
expiry time rather than an infinite expiry time.

---

From: Pavel Luzanov <[email-masked]>
Date: Sun, 31 Dec 2023 13:52:28 +0300

On 30.12.2023 17:33, Isaac Morland wrote:
> Would it make sense to make the column non-nullable and always set it 
> to infinity when there is no expiry?

A password is not required for roles. In many cases, external 
authentication is used in ph_hba.conf.
I think it would be strange to have 'infinity' for roles without a password.

Tom suggested to have 'infinity' in the \du output for roles with a 
password.
My doubt is that this will hide the real values (absence of values). So 
I suggested a separate column
'Has password?' to show roles with password and unmodified column 
'Password expire time'.

Yes, it's easy to replace NULL with "infinity" for roles with a 
password, but why?
What is the reason for this action? Absence of value for 'expire time' 
clear indicates that there is no time limit.
Also I don't see a practical reasons to execute next command, since it 
do nothing:

ALTER ROLE .. PASSWORD 'infinity';

So I think that in most cases there is no "infinity" in the 
rolvaliduntil column.

But of course, I can be wrong.

Thank you for giving your opinion.