Re: Fix output of zero privileges in psql

On Sat, 2023-10-21 at 04:29 +0200, Erik Wienhold wrote:
> The attached v3 of my initial patch
> does that.  It also includes Laurenz' fix to no longer ignore \pset null
> (minus the doc changes that suggest using \pset null to distinguish
> between default and empty privileges because that's no longer needed).

Thanks!

I went over the patch, fixed some problems and added some more stuff from
my patch.

In particular:

--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2353,7 +2353,9 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO miriam_rw;
<para>
If the <quote>Access privileges</quote> column is empty for a given
object, it means the object has default privileges (that is, its
- privileges entry in the relevant system catalog is null). Default
+ privileges entry in the relevant system catalog is null). The column shows
+ <literal>(none)</literal> for empty privileges (that is, no privileges at
+ all, even for the object owner &mdash; a rare occurrence). Default
privileges always include all privileges for the owner, and can include
some privileges for <literal>PUBLIC</literal> depending on the object
type, as explained above. The first <command>GRANT</command>

This description of empty privileges is smack in the middle of describing
default privileges. I thought that was confusing and moved it to its
own paragraph.

--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6718,7 +6680,13 @@ static void
printACLColumn(PQExpBuffer buf, const char *colname)
{
appendPQExpBuffer(buf,
- "pg_catalog.array_to_string(%s, E'\\n') AS \"%s\"",
+ "CASE\n"
+ " WHEN %s IS NULL THEN ''\n"
+ " WHEN pg_catalog.cardinality(%s) = 0 THEN '%s'\n"
+ " ELSE pg_catalog.array_to_string(%s, E'\\n')\n"
+ "END AS \"%s\"",
+ colname,
+ colname, gettext_noop("(none)"),
colname, gettext_noop("Access privileges"));
}

This erroneously displays NULL as empty string and subverts my changes.
I have removed the first branch of the CASE expression.

--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -6663,3 +6663,97 @@ DROP ROLE regress_du_role0;
DROP ROLE regress_du_role1;
DROP ROLE regress_du_role2;
DROP ROLE regress_du_admin;
+-- Test empty privileges.
+BEGIN;
+WARNING: there is already a transaction in progress

This warning is caused by a pre-existing error in the regression test, which
forgot to close the transaction. I have added a COMMIT at the appropriate place.

+ALTER TABLESPACE regress_tblspace OWNER TO CURRENT_USER;
+REVOKE ALL ON TABLESPACE regress_tblspace FROM CURRENT_USER;
+\db+ regress_tblspace
+ List of tablespaces
+ Name | Owner | Location | Access privileges | Options | Size | Description
+------------------+------------------------+-----------------+-------------------+---------+---------+-------------
+ regress_tblspace | regress_zeropriv_owner | pg_tblspc/16385 | (none) | | 0 bytes |
+(1 row)

This test is not stable, since it contains the OID of the tablespace, which
is different every time.

+ALTER DATABASE :"DBNAME" OWNER TO CURRENT_USER;
+REVOKE ALL ON DATABASE :"DBNAME" FROM CURRENT_USER, PUBLIC;
+\l :"DBNAME"
+ List of databases
+ Name | Owner | Encoding | Locale Provider | Collate | Ctype | ICU Locale | ICU Rules | Access privileges
+------------+------------------------+-----------+-----------------+---------+-------+------------+-----------+-------------------
+ regression | regress_zeropriv_owner | SQL_ASCII | libc | C | C | | | (none)
+(1 row)

This test is also not stable, since it depends on the locale definition
of the regression test database. If you use "make installcheck", that could
be a different locale.

I think that these tests are not absolutely necessary, and the other tests
are sufficient. Consequently, I took the simple road of removing them.

I also tried to improve the commit message.

Patch attached.

Yours,
Laurenz Albe