On Wed, Nov 8, 2023 at 10:46 AM Laurenz Albe <laurenz.a...@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 | 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.
I tested the Patch for the modified changes and it is working fine.
Thanks and regards,
Shubham Khanna.
