Re: Fix output of zero privileges in psql - Mailing list pgsql-hackers

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 — 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 |
Accessprivileges  

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