Re: Fix output of zero privileges in psql

On Wed, Nov 8, 2023 at 10:46 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> 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.

I tested the Patch for the modified changes and it is working fine.

Thanks and regards,
Shubham Khanna.