Fix output of zero privileges in psql
I wrote a patch to change psql's display of zero privileges after a user's
reported confusion with the psql output for zero vs. default privileges [1]/messages/by-id/efdd465d-a795-6188-7f71-7cdb4b2be031@mtneva.com.
Admittedly, zero privileges is a rare use case [2]/messages/by-id/31246.1693337238@sss.pgh.pa.us but I think psql should not
confuse the user in the off chance that this happens.
With this change psql now prints "(none)" for zero privileges instead of
nothing. This affects the following meta commands:
\db+ \dD+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l
Default privileges start as NULL::aclitem[] in various catalog columns but
revoking the default privileges leaves an empty aclitem array. Using
\pset null '(null)' as a workaround to spot default privileges does not work
because the meta commands ignore this setting.
The privileges shown by \dconfig+ and \ddp as well as the column privileges
shown by \dp are not affected by this change because those privileges are reset
to NULL instead of leaving empty arrays.
Commands \des+ and \dew+ are not covered in src/test/regress because no foreign
data wrapper is available at this point to create a foreign server.
[1]: /messages/by-id/efdd465d-a795-6188-7f71-7cdb4b2be031@mtneva.com
[2]: /messages/by-id/31246.1693337238@sss.pgh.pa.us
--
Erik
Attachments:
v1-0001-Fix-output-of-zero-privileges-in-psql.patchtext/x-patch; charset=UTF-8; name=v1-0001-Fix-output-of-zero-privileges-in-psql.patchDownload
From 16af995acb4c0e5fb5981308610a76b7e87c3358 Mon Sep 17 00:00:00 2001
From: Erik Wienhold <ewie@ewie.name>
Date: Sun, 17 Sep 2023 20:54:48 +0200
Subject: [PATCH v1] Fix output of zero privileges in psql
Print "(none)" for zero privileges instead of nothing so that the user
is able to distinguish zero from default privileges. This affects the
following meta commands:
\db+ \dD+ \des+ \dew+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l
Default privileges start as NULL::aclitem[] in various catalog columns
but revoking the default privileges leaves an empty aclitem array.
Using \pset null '(null)' as a workaround to spot default privileges
does not work because the meta commands ignore this setting.
The privileges shown by \dconfig+ and \ddp as well as the column
privileges shown by \dp are not affected by this change because those
privileges are reset to NULL instead of leaving empty arrays.
Commands \des+ and \dew+ are not covered in src/test/regress because no
foreign data wrapper is available at this point to create a foreign
server.
---
src/bin/psql/describe.c | 6 +-
src/test/regress/expected/privileges.out | 93 ++++++++++++++++++++++++
src/test/regress/sql/privileges.sql | 46 ++++++++++++
3 files changed, 144 insertions(+), 1 deletion(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..154d244d97 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6718,7 +6718,11 @@ static void
printACLColumn(PQExpBuffer buf, const char *colname)
{
appendPQExpBuffer(buf,
- "pg_catalog.array_to_string(%s, E'\\n') AS \"%s\"",
+ "CASE pg_catalog.cardinality(%s)\n"
+ " WHEN 0 THEN '%s'\n"
+ " ELSE pg_catalog.array_to_string(%s, E'\\n')\n"
+ "END AS \"%s\"",
+ colname, gettext_noop("(none)"),
colname, gettext_noop("Access privileges"));
}
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index c1e610e62f..b9eb52f7b1 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -2895,3 +2895,96 @@ DROP SCHEMA regress_roleoption;
DROP ROLE regress_roleoption_protagonist;
DROP ROLE regress_roleoption_donor;
DROP ROLE regress_roleoption_recipient;
+-- Test zero privileges.
+BEGIN;
+-- Create an owner for tested objects because output contains owner info.
+-- Must be superuser to be owner of tablespace.
+CREATE ROLE regress_zeropriv_owner SUPERUSER;
+SET LOCAL ROLE regress_zeropriv_owner;
+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)
+
+CREATE DOMAIN regress_zeropriv_domain AS int;
+REVOKE ALL ON DOMAIN regress_zeropriv_domain FROM CURRENT_USER, PUBLIC;
+\dD+ regress_zeropriv_domain
+ List of domains
+ Schema | Name | Type | Collation | Nullable | Default | Check | Access privileges | Description
+--------+-------------------------+---------+-----------+----------+---------+-------+-------------------+-------------
+ public | regress_zeropriv_domain | integer | | | | | (none) |
+(1 row)
+
+CREATE PROCEDURE regress_zeropriv_proc() LANGUAGE sql AS '';
+REVOKE ALL ON PROCEDURE regress_zeropriv_proc() FROM CURRENT_USER, PUBLIC;
+\df+ regress_zeropriv_proc
+ List of functions
+ Schema | Name | Result data type | Argument data types | Type | Volatility | Parallel | Owner | Security | Access privileges | Language | Internal name | Description
+--------+-----------------------+------------------+---------------------+------+------------+----------+------------------------+----------+-------------------+----------+---------------+-------------
+ public | regress_zeropriv_proc | | | proc | volatile | unsafe | regress_zeropriv_owner | invoker | (none) | sql | |
+(1 row)
+
+ALTER LANGUAGE plpgsql OWNER TO CURRENT_USER;
+REVOKE ALL ON LANGUAGE plpgsql FROM CURRENT_USER, PUBLIC;
+\dL+ plpgsql
+ List of languages
+ Name | Owner | Trusted | Internal language | Call handler | Validator | Inline handler | Access privileges | Description
+---------+------------------------+---------+-------------------+------------------------+------------------------+----------------------------------+-------------------+------------------------------
+ plpgsql | regress_zeropriv_owner | t | f | plpgsql_call_handler() | plpgsql_validator(oid) | plpgsql_inline_handler(internal) | (none) | PL/pgSQL procedural language
+(1 row)
+
+SELECT lo_create(3001);
+ lo_create
+-----------
+ 3001
+(1 row)
+
+REVOKE ALL ON LARGE OBJECT 3001 FROM CURRENT_USER;
+\dl+
+ Large objects
+ ID | Owner | Access privileges | Description
+------+------------------------+-------------------+-------------
+ 3001 | regress_zeropriv_owner | (none) |
+(1 row)
+
+CREATE SCHEMA regress_zeropriv_schema;
+REVOKE ALL ON SCHEMA regress_zeropriv_schema FROM CURRENT_USER;
+\dn+ regress_zeropriv_schema
+ List of schemas
+ Name | Owner | Access privileges | Description
+-------------------------+------------------------+-------------------+-------------
+ regress_zeropriv_schema | regress_zeropriv_owner | (none) |
+(1 row)
+
+CREATE TABLE regress_zeropriv_tbl (a int);
+REVOKE ALL ON TABLE regress_zeropriv_tbl FROM CURRENT_USER;
+\dp regress_zeropriv_tbl
+ Access privileges
+ Schema | Name | Type | Access privileges | Column privileges | Policies
+--------+----------------------+-------+-------------------+-------------------+----------
+ public | regress_zeropriv_tbl | table | (none) | |
+(1 row)
+
+CREATE TYPE regress_zeropriv_type AS (a int);
+REVOKE ALL ON TYPE regress_zeropriv_type FROM CURRENT_USER, PUBLIC;
+\dT+ regress_zeropriv_type
+ List of data types
+ Schema | Name | Internal name | Size | Elements | Owner | Access privileges | Description
+--------+-----------------------+-----------------------+-------+----------+------------------------+-------------------+-------------
+ public | regress_zeropriv_type | regress_zeropriv_type | tuple | | regress_zeropriv_owner | (none) |
+(1 row)
+
+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)
+
+ROLLBACK;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index bf0035d96d..70c06b2fa4 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1861,3 +1861,49 @@ DROP SCHEMA regress_roleoption;
DROP ROLE regress_roleoption_protagonist;
DROP ROLE regress_roleoption_donor;
DROP ROLE regress_roleoption_recipient;
+
+
+-- Test zero privileges.
+BEGIN;
+-- Create an owner for tested objects because output contains owner info.
+-- Must be superuser to be owner of tablespace.
+CREATE ROLE regress_zeropriv_owner SUPERUSER;
+SET LOCAL ROLE regress_zeropriv_owner;
+
+ALTER TABLESPACE regress_tblspace OWNER TO CURRENT_USER;
+REVOKE ALL ON TABLESPACE regress_tblspace FROM CURRENT_USER;
+\db+ regress_tblspace
+
+CREATE DOMAIN regress_zeropriv_domain AS int;
+REVOKE ALL ON DOMAIN regress_zeropriv_domain FROM CURRENT_USER, PUBLIC;
+\dD+ regress_zeropriv_domain
+
+CREATE PROCEDURE regress_zeropriv_proc() LANGUAGE sql AS '';
+REVOKE ALL ON PROCEDURE regress_zeropriv_proc() FROM CURRENT_USER, PUBLIC;
+\df+ regress_zeropriv_proc
+
+ALTER LANGUAGE plpgsql OWNER TO CURRENT_USER;
+REVOKE ALL ON LANGUAGE plpgsql FROM CURRENT_USER, PUBLIC;
+\dL+ plpgsql
+
+SELECT lo_create(3001);
+REVOKE ALL ON LARGE OBJECT 3001 FROM CURRENT_USER;
+\dl+
+
+CREATE SCHEMA regress_zeropriv_schema;
+REVOKE ALL ON SCHEMA regress_zeropriv_schema FROM CURRENT_USER;
+\dn+ regress_zeropriv_schema
+
+CREATE TABLE regress_zeropriv_tbl (a int);
+REVOKE ALL ON TABLE regress_zeropriv_tbl FROM CURRENT_USER;
+\dp regress_zeropriv_tbl
+
+CREATE TYPE regress_zeropriv_type AS (a int);
+REVOKE ALL ON TYPE regress_zeropriv_type FROM CURRENT_USER, PUBLIC;
+\dT+ regress_zeropriv_type
+
+ALTER DATABASE :"DBNAME" OWNER TO CURRENT_USER;
+REVOKE ALL ON DATABASE :"DBNAME" FROM CURRENT_USER, PUBLIC;
+\l :"DBNAME"
+
+ROLLBACK;
--
2.42.0
On 17/09/2023 21:31 CEST Erik Wienhold <ewie@ewie.name> wrote:
This affects the following meta commands:
\db+ \dD+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l
also \des+ and \dew+
--
Erik
On Sun, 2023-09-17 at 21:31 +0200, Erik Wienhold wrote:
I wrote a patch to change psql's display of zero privileges after a user's
reported confusion with the psql output for zero vs. default privileges [1].
Admittedly, zero privileges is a rare use case [2] but I think psql should not
confuse the user in the off chance that this happens.With this change psql now prints "(none)" for zero privileges instead of
nothing. This affects the following meta commands:\db+ \dD+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l
Default privileges start as NULL::aclitem[] in various catalog columns but
revoking the default privileges leaves an empty aclitem array. Using
\pset null '(null)' as a workaround to spot default privileges does not work
because the meta commands ignore this setting.The privileges shown by \dconfig+ and \ddp as well as the column privileges
shown by \dp are not affected by this change because those privileges are reset
to NULL instead of leaving empty arrays.Commands \des+ and \dew+ are not covered in src/test/regress because no foreign
data wrapper is available at this point to create a foreign server.[1] /messages/by-id/efdd465d-a795-6188-7f71-7cdb4b2be031@mtneva.com
[2] /messages/by-id/31246.1693337238@sss.pgh.pa.us
Reading that thread, I had the impression that there was more support for
honoring "\pset null" rather than unconditionally displaying "(none)".
The simple attached patch does it like that. What do you think?
Yours,
Laurenz Albe
Attachments:
0001-psql-honor-pset-null-in-backslash-commands.patchtext/x-patch; charset=UTF-8; name=0001-psql-honor-pset-null-in-backslash-commands.patchDownload
From 6c67f15f011ddf1e309cb7e84580b266d674a1e2 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Fri, 6 Oct 2023 22:12:33 +0200
Subject: [PATCH] psql: honor "\pset null" in backslash commands
In the output of backslash commands, NULL was always displayed
as empty string, rather than honoring the values set with
"\pset null".
This was surprising, and in some cases it was downright confusing:
for example, default privileges (stored as NULL) were displayed
just like an empty aclitem[], making these cases undistinguishable
in the output.
Discussion: https://postgr.es/m/96d6885a-5e25-9ae8-4a1a-d7e557a5fe9c%40mtneva.com
---
src/bin/psql/describe.c | 43 -----------------------------------------
1 file changed, 43 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..224aa22575 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -124,7 +124,6 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of aggregate functions");
myopt.translate_header = true;
@@ -197,7 +196,6 @@ describeAccessMethods(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of access methods");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -262,7 +260,6 @@ describeTablespaces(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of tablespaces");
myopt.translate_header = true;
@@ -585,7 +582,6 @@ describeFunctions(const char *functypes, const char *func_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of functions");
myopt.translate_header = true;
if (pset.sversion >= 90600)
@@ -702,7 +698,6 @@ describeTypes(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of data types");
myopt.translate_header = true;
@@ -893,7 +888,6 @@ describeOperators(const char *oper_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators");
myopt.translate_header = true;
@@ -995,7 +989,6 @@ listAllDbs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of databases");
myopt.translate_header = true;
@@ -1146,7 +1139,6 @@ permissionsList(const char *pattern, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1218,7 +1210,6 @@ listDefaultACLs(const char *pattern)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Default access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1417,7 +1408,6 @@ objectDescription(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Object descriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -3852,7 +3842,6 @@ listDbRoleSettings(const char *pattern, const char *pattern2)
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of settings");
myopt.translate_header = true;
@@ -3926,7 +3915,6 @@ describeRoleGrants(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of role grants");
myopt.translate_header = true;
@@ -4122,7 +4110,6 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of relations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4332,7 +4319,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
initPQExpBuffer(&title);
appendPQExpBufferStr(&title, tabletitle);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4412,7 +4398,6 @@ listLanguages(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of languages");
myopt.translate_header = true;
@@ -4497,7 +4482,6 @@ listDomains(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of domains");
myopt.translate_header = true;
@@ -4576,7 +4560,6 @@ listConversions(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of conversions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4644,7 +4627,6 @@ describeConfigurationParameters(const char *pattern, bool verbose,
if (!res)
return false;
- myopt.nullPrint = NULL;
if (pattern)
myopt.title = _("List of configuration parameters");
else
@@ -4726,7 +4708,6 @@ listEventTriggers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of event triggers");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4825,7 +4806,6 @@ listExtendedStats(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of extended statistics");
myopt.translate_header = true;
@@ -4938,7 +4918,6 @@ listCasts(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of casts");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5057,7 +5036,6 @@ listCollations(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of collations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5119,7 +5097,6 @@ listSchemas(const char *pattern, bool verbose, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
myopt.title = _("List of schemas");
myopt.translate_header = true;
@@ -5236,7 +5213,6 @@ listTSParsers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search parsers");
myopt.translate_header = true;
@@ -5384,7 +5360,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
if (nspname)
printfPQExpBuffer(&title, _("Text search parser \"%s.%s\""),
@@ -5421,7 +5396,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
return false;
}
- myopt.nullPrint = NULL;
if (nspname)
printfPQExpBuffer(&title, _("Token types for parser \"%s.%s\""),
nspname, prsname);
@@ -5497,7 +5471,6 @@ listTSDictionaries(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search dictionaries");
myopt.translate_header = true;
@@ -5563,7 +5536,6 @@ listTSTemplates(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search templates");
myopt.translate_header = true;
@@ -5618,7 +5590,6 @@ listTSConfigs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search configurations");
myopt.translate_header = true;
@@ -5764,7 +5735,6 @@ describeOneTSConfig(const char *oid, const char *nspname, const char *cfgname,
appendPQExpBuffer(&title, _("\nParser: \"%s\""),
prsname);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.footers = NULL;
myopt.topt.default_footer = false;
@@ -5841,7 +5811,6 @@ listForeignDataWrappers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign-data wrappers");
myopt.translate_header = true;
@@ -5918,7 +5887,6 @@ listForeignServers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign servers");
myopt.translate_header = true;
@@ -5974,7 +5942,6 @@ listUserMappings(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of user mappings");
myopt.translate_header = true;
@@ -6047,7 +6014,6 @@ listForeignTables(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign tables");
myopt.translate_header = true;
@@ -6099,7 +6065,6 @@ listExtensions(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of installed extensions");
myopt.translate_header = true;
@@ -6203,7 +6168,6 @@ listOneExtensionContents(const char *extname, const char *oid)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
printfPQExpBuffer(&title, _("Objects in extension \"%s\""), extname);
myopt.title = title.data;
@@ -6340,7 +6304,6 @@ listPublications(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of publications");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6695,7 +6658,6 @@ describeSubscriptions(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of subscriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6808,7 +6770,6 @@ listOperatorClasses(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator classes");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6897,7 +6858,6 @@ listOperatorFamilies(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6996,7 +6956,6 @@ listOpFamilyOperators(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7089,7 +7048,6 @@ listOpFamilyFunctions(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of support functions of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7141,7 +7099,6 @@ listLargeObjects(bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Large objects");
myopt.translate_header = true;
--
2.41.0
On 2023-10-06 22:32 +0200, Laurenz Albe write:
On Sun, 2023-09-17 at 21:31 +0200, Erik Wienhold wrote:
I wrote a patch to change psql's display of zero privileges after a user's
reported confusion with the psql output for zero vs. default privileges [1].
Admittedly, zero privileges is a rare use case [2] but I think psql should not
confuse the user in the off chance that this happens.With this change psql now prints "(none)" for zero privileges instead of
nothing. This affects the following meta commands:\db+ \dD+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l
Default privileges start as NULL::aclitem[] in various catalog columns but
revoking the default privileges leaves an empty aclitem array. Using
\pset null '(null)' as a workaround to spot default privileges does not work
because the meta commands ignore this setting.The privileges shown by \dconfig+ and \ddp as well as the column privileges
shown by \dp are not affected by this change because those privileges are reset
to NULL instead of leaving empty arrays.Commands \des+ and \dew+ are not covered in src/test/regress because no foreign
data wrapper is available at this point to create a foreign server.[1] /messages/by-id/efdd465d-a795-6188-7f71-7cdb4b2be031@mtneva.com
[2] /messages/by-id/31246.1693337238@sss.pgh.pa.usReading that thread, I had the impression that there was more support for
honoring "\pset null" rather than unconditionally displaying "(none)".
I took Tom's response in the -general thread to mean that we could fix
\pset null also as a "nice to have" but not as a solution to the display
of zero privileges.
Only fixing \pset null has one drawback IMO because it only affects how
default privileges (more common) are printed. The edge case of zero
privileges (less common) gets lost in a bunch of NULL output. And I
assume most users change the default \pset null to some non-empty string
in their psqlrc (I do).
For example with your patch applied:
create table t1 (a int);
create table t2 (a int);
create table t3 (a int);
revoke all on t2 from :USER;
\pset null <NULL>
\dp t1|t2|t3
Access privileges
Schema | Name | Type | Access privileges | Column privileges | Policies
--------+------+-------+-------------------+-------------------+----------
public | t1 | table | <NULL> | |
public | t2 | table | | |
public | t3 | table | <NULL> | |
(3 rows)
Instead of only displaying the zero privileges with my patch and default
\pset null:
\pset null ''
\dp t1|t2|t3
Access privileges
Schema | Name | Type | Access privileges | Column privileges | Policies
--------+------+-------+-------------------+-------------------+----------
public | t1 | table | | |
public | t2 | table | (none) | |
public | t3 | table | | |
(3 rows)
I guess if most tables have any non-default privileges then both
solutions are equally good.
The simple attached patch does it like that. What do you think?
LGTM.
--
Erik
On Sat, 2023-10-07 at 05:07 +0200, Erik Wienhold wrote:
On 2023-10-06 22:32 +0200, Laurenz Albe write:
On Sun, 2023-09-17 at 21:31 +0200, Erik Wienhold wrote:
I wrote a patch to change psql's display of zero privileges after a user's
reported confusion with the psql output for zero vs. default privileges [1].
Admittedly, zero privileges is a rare use case [2] but I think psql should not
confuse the user in the off chance that this happens.[1] /messages/by-id/efdd465d-a795-6188-7f71-7cdb4b2be031@mtneva.com
[2] /messages/by-id/31246.1693337238@sss.pgh.pa.usReading that thread, I had the impression that there was more support for
honoring "\pset null" rather than unconditionally displaying "(none)".For example with your patch applied:
create table t1 (a int);
create table t2 (a int);
create table t3 (a int);revoke all on t2 from :USER;
\pset null <NULL>
\dp t1|t2|t3
Access privileges
Schema | Name | Type | Access privileges | Column privileges | Policies
--------+------+-------+-------------------+-------------------+----------
public | t1 | table | <NULL> | |
public | t2 | table | | |
public | t3 | table | <NULL> | |
(3 rows)Instead of only displaying the zero privileges with my patch and default
\pset null:\pset null ''
\dp t1|t2|t3
Access privileges
Schema | Name | Type | Access privileges | Column privileges | Policies
--------+------+-------+-------------------+-------------------+----------
public | t1 | table | | |
public | t2 | table | (none) | |
public | t3 | table | | |
(3 rows)I guess if most tables have any non-default privileges then both
solutions are equally good.
It is a tough call.
For somebody who knows PostgreSQL well enough to know that default privileges are
represented by NULL values, my solution is probably more appealing.
It seems that we both had the goal of distinguishing the cases of default and
zero privileges, but for a beginner, both versions are confusing. better would
probably be
Access privileges
Schema | Name | Type | Access privileges | Column privileges | Policies
--------+------+-------+-------------------+-------------------+----------
public | t1 | table | default | default |
public | t2 | table | | default |
public | t3 | table | default | default |
The disadvantage of this (and the advantage of my proposal) is that it might
confuse experienced users (and perhaps automated tools) if the output changes
too much.
The simple attached patch does it like that. What do you think?
LGTM.
If you are happy enough with my patch, shall we mark it as ready for committer?
Or do you want to have a stab at something like I suggested above?
Yours,
Laurenz Albe
On 2023-10-07 14:29 +0200, Laurenz Albe write:
On Sat, 2023-10-07 at 05:07 +0200, Erik Wienhold wrote:
On 2023-10-06 22:32 +0200, Laurenz Albe write:
On Sun, 2023-09-17 at 21:31 +0200, Erik Wienhold wrote:
I wrote a patch to change psql's display of zero privileges after a user's
reported confusion with the psql output for zero vs. default privileges [1].
Admittedly, zero privileges is a rare use case [2] but I think psql should not
confuse the user in the off chance that this happens.[1] /messages/by-id/efdd465d-a795-6188-7f71-7cdb4b2be031@mtneva.com
[2] /messages/by-id/31246.1693337238@sss.pgh.pa.usReading that thread, I had the impression that there was more support for
honoring "\pset null" rather than unconditionally displaying "(none)".For example with your patch applied:
create table t1 (a int);
create table t2 (a int);
create table t3 (a int);revoke all on t2 from :USER;
\pset null <NULL>
\dp t1|t2|t3
Access privileges
Schema | Name | Type | Access privileges | Column privileges | Policies
--------+------+-------+-------------------+-------------------+----------
public | t1 | table | <NULL> | |
public | t2 | table | | |
public | t3 | table | <NULL> | |
(3 rows)Instead of only displaying the zero privileges with my patch and default
\pset null:\pset null ''
\dp t1|t2|t3
Access privileges
Schema | Name | Type | Access privileges | Column privileges | Policies
--------+------+-------+-------------------+-------------------+----------
public | t1 | table | | |
public | t2 | table | (none) | |
public | t3 | table | | |
(3 rows)I guess if most tables have any non-default privileges then both
solutions are equally good.It is a tough call.
For somebody who knows PostgreSQL well enough to know that default
privileges are represented by NULL values, my solution is probably
more appealing.It seems that we both had the goal of distinguishing the cases of
default and zero privileges, but for a beginner, both versions are
confusing. better would probably beAccess privileges
Schema | Name | Type | Access privileges | Column privileges | Policies
--------+------+-------+-------------------+-------------------+----------
public | t1 | table | default | default |
public | t2 | table | | default |
public | t3 | table | default | default |
Ah yes. The problem seems to be more with default privileges producing
no output right now. I was just focusing on the zero privs edge case.
The disadvantage of this (and the advantage of my proposal) is that it
might confuse experienced users (and perhaps automated tools) if the
output changes too much.
I agree that your patch is less invasive under default settings. But is
the output of meta commands considered part of the interface where we
need to be cautious about not breaking clients?
I've written quite a few scripts that parse results from psql's stdout,
but always with simple queries to have control over columns and the
formatting of values. I always expect meta command output to change
with the next release because to me they look more like a human-readable
interface, e.g. the localizable header which of course one can still
hide with --tuples-only.
The simple attached patch does it like that. What do you think?
LGTM.
If you are happy enough with my patch, shall we mark it as ready for
committer?
I amended your patch to also document the effect of \pset null in this
case. See the attached v2.
Or do you want to have a stab at something like I suggested above?
Not right now if the user can just use \pset null 'default' with your
patch.
--
Erik
Attachments:
v2-0001-psql-honor-pset-null-in-backslash-commands.patchtext/plain; charset=us-asciiDownload
From a3ea9549a4467fb4ab186d652e0afec6adc2f725 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Fri, 6 Oct 2023 22:12:33 +0200
Subject: [PATCH v2] psql: honor "\pset null" in backslash commands
In the output of backslash commands, NULL was always displayed
as empty string, rather than honoring the values set with
"\pset null".
This was surprising, and in some cases it was downright confusing:
for example, default privileges (stored as NULL) were displayed
just like an empty aclitem[], making these cases undistinguishable
in the output.
Discussion: https://postgr.es/m/96d6885a-5e25-9ae8-4a1a-d7e557a5fe9c%40mtneva.com
---
doc/src/sgml/ddl.sgml | 6 ++++--
src/bin/psql/describe.c | 43 -----------------------------------------
2 files changed, 4 insertions(+), 45 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 075ff32991..62a65429d6 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2353,8 +2353,8 @@ 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 always include all privileges for the owner, and can include
+ privileges entry in the relevant system catalog is null) or zero privileges.
+ 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>
or <command>REVOKE</command> on an object will instantiate the default
@@ -2362,6 +2362,8 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO miriam_rw;
example, <literal>miriam=arwdDxt/miriam</literal>) and then modify them
per the specified request. Similarly, entries are shown in <quote>Column
privileges</quote> only for columns with nondefault privileges.
+ One might prefer <literal>\pset null '(null)'</literal> to distinguish
+ default from zero privileges.
(Note: for this purpose, <quote>default privileges</quote> always means
the built-in default privileges for the object's type. An object whose
privileges have been affected by an <command>ALTER DEFAULT
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..224aa22575 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -124,7 +124,6 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of aggregate functions");
myopt.translate_header = true;
@@ -197,7 +196,6 @@ describeAccessMethods(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of access methods");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -262,7 +260,6 @@ describeTablespaces(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of tablespaces");
myopt.translate_header = true;
@@ -585,7 +582,6 @@ describeFunctions(const char *functypes, const char *func_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of functions");
myopt.translate_header = true;
if (pset.sversion >= 90600)
@@ -702,7 +698,6 @@ describeTypes(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of data types");
myopt.translate_header = true;
@@ -893,7 +888,6 @@ describeOperators(const char *oper_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators");
myopt.translate_header = true;
@@ -995,7 +989,6 @@ listAllDbs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of databases");
myopt.translate_header = true;
@@ -1146,7 +1139,6 @@ permissionsList(const char *pattern, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1218,7 +1210,6 @@ listDefaultACLs(const char *pattern)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Default access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1417,7 +1408,6 @@ objectDescription(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Object descriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -3852,7 +3842,6 @@ listDbRoleSettings(const char *pattern, const char *pattern2)
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of settings");
myopt.translate_header = true;
@@ -3926,7 +3915,6 @@ describeRoleGrants(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of role grants");
myopt.translate_header = true;
@@ -4122,7 +4110,6 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of relations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4332,7 +4319,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
initPQExpBuffer(&title);
appendPQExpBufferStr(&title, tabletitle);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4412,7 +4398,6 @@ listLanguages(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of languages");
myopt.translate_header = true;
@@ -4497,7 +4482,6 @@ listDomains(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of domains");
myopt.translate_header = true;
@@ -4576,7 +4560,6 @@ listConversions(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of conversions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4644,7 +4627,6 @@ describeConfigurationParameters(const char *pattern, bool verbose,
if (!res)
return false;
- myopt.nullPrint = NULL;
if (pattern)
myopt.title = _("List of configuration parameters");
else
@@ -4726,7 +4708,6 @@ listEventTriggers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of event triggers");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4825,7 +4806,6 @@ listExtendedStats(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of extended statistics");
myopt.translate_header = true;
@@ -4938,7 +4918,6 @@ listCasts(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of casts");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5057,7 +5036,6 @@ listCollations(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of collations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5119,7 +5097,6 @@ listSchemas(const char *pattern, bool verbose, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
myopt.title = _("List of schemas");
myopt.translate_header = true;
@@ -5236,7 +5213,6 @@ listTSParsers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search parsers");
myopt.translate_header = true;
@@ -5384,7 +5360,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
if (nspname)
printfPQExpBuffer(&title, _("Text search parser \"%s.%s\""),
@@ -5421,7 +5396,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
return false;
}
- myopt.nullPrint = NULL;
if (nspname)
printfPQExpBuffer(&title, _("Token types for parser \"%s.%s\""),
nspname, prsname);
@@ -5497,7 +5471,6 @@ listTSDictionaries(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search dictionaries");
myopt.translate_header = true;
@@ -5563,7 +5536,6 @@ listTSTemplates(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search templates");
myopt.translate_header = true;
@@ -5618,7 +5590,6 @@ listTSConfigs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search configurations");
myopt.translate_header = true;
@@ -5764,7 +5735,6 @@ describeOneTSConfig(const char *oid, const char *nspname, const char *cfgname,
appendPQExpBuffer(&title, _("\nParser: \"%s\""),
prsname);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.footers = NULL;
myopt.topt.default_footer = false;
@@ -5841,7 +5811,6 @@ listForeignDataWrappers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign-data wrappers");
myopt.translate_header = true;
@@ -5918,7 +5887,6 @@ listForeignServers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign servers");
myopt.translate_header = true;
@@ -5974,7 +5942,6 @@ listUserMappings(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of user mappings");
myopt.translate_header = true;
@@ -6047,7 +6014,6 @@ listForeignTables(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign tables");
myopt.translate_header = true;
@@ -6099,7 +6065,6 @@ listExtensions(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of installed extensions");
myopt.translate_header = true;
@@ -6203,7 +6168,6 @@ listOneExtensionContents(const char *extname, const char *oid)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
printfPQExpBuffer(&title, _("Objects in extension \"%s\""), extname);
myopt.title = title.data;
@@ -6340,7 +6304,6 @@ listPublications(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of publications");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6695,7 +6658,6 @@ describeSubscriptions(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of subscriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6808,7 +6770,6 @@ listOperatorClasses(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator classes");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6897,7 +6858,6 @@ listOperatorFamilies(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6996,7 +6956,6 @@ listOpFamilyOperators(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7089,7 +7048,6 @@ listOpFamilyFunctions(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of support functions of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7141,7 +7099,6 @@ listLargeObjects(bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Large objects");
myopt.translate_header = true;
--
2.42.0
On Sat, 2023-10-07 at 20:41 +0200, Erik Wienhold wrote:
If you are happy enough with my patch, shall we mark it as ready for
committer?I amended your patch to also document the effect of \pset null in this
case. See the attached v2.
+1
If you mention in ddl.sgml that you can use "\pset null" to distinguish
default from no privileges, you should mention that this only works with
psql. Many people out there don't use psql.
Also, I'm not sure if "zero privileges" will be readily understood by
everybody. Perhaps "no privileges at all, even for the object owner"
would be a better wording.
Perhaps it would also be good to mention this in the psql documentation.
Yours,
Laurenz Albe
On 2023-10-08 06:14 +0200, Laurenz Albe write:
On Sat, 2023-10-07 at 20:41 +0200, Erik Wienhold wrote:
If you are happy enough with my patch, shall we mark it as ready for
committer?I amended your patch to also document the effect of \pset null in this
case. See the attached v2.+1
If you mention in ddl.sgml that you can use "\pset null" to distinguish
default from no privileges, you should mention that this only works with
psql. Many people out there don't use psql.
I don't think this is necessary because that section in ddl.sgml is
already about psql and \dp.
Also, I'm not sure if "zero privileges" will be readily understood by
everybody. Perhaps "no privileges at all, even for the object owner"
would be a better wording.
Changed in v3 to "empty privileges" with an explanation that this means
"no privileges at all, even for the object owner".
Perhaps it would also be good to mention this in the psql documentation.
Just once under \pset null with a reference to section 5.7? Something
like "This is also useful for distinguishing default privileges from
empty privileges as explained in Section 5.7."
Or instead under every command affected by this change? \dp and \ddp
already have such a reference ("The meaning of the privilege display is
explained in Section 5.7.")
I prefer the first one because it's less effort ;) Also done in v3.
--
Erik
Attachments:
v3-0001-psql-honor-pset-null-in-backslash-commands.patchtext/plain; charset=us-asciiDownload
From b7ddd4daa87baab83ad7e857c996b5a4a0b3ffa7 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Fri, 6 Oct 2023 22:12:33 +0200
Subject: [PATCH v3] psql: honor "\pset null" in backslash commands
In the output of backslash commands, NULL was always displayed
as empty string, rather than honoring the values set with
"\pset null".
This was surprising, and in some cases it was downright confusing:
for example, default privileges (stored as NULL) were displayed
just like an empty aclitem[], making these cases undistinguishable
in the output. Add this missing detail to the documentation.
Discussion: https://postgr.es/m/96d6885a-5e25-9ae8-4a1a-d7e557a5fe9c%40mtneva.com
---
doc/src/sgml/ddl.sgml | 7 ++++--
doc/src/sgml/ref/psql-ref.sgml | 3 ++-
src/bin/psql/describe.c | 43 ----------------------------------
3 files changed, 7 insertions(+), 46 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 075ff32991..436d655d3c 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2353,8 +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 always include all privileges for the owner, and can include
+ privileges entry in the relevant system catalog is null) or empty privileges
+ (that is, no privileges at all, even for the object owner).
+ 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>
or <command>REVOKE</command> on an object will instantiate the default
@@ -2362,6 +2363,8 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO miriam_rw;
example, <literal>miriam=arwdDxt/miriam</literal>) and then modify them
per the specified request. Similarly, entries are shown in <quote>Column
privileges</quote> only for columns with nondefault privileges.
+ You can, for example, set <literal>\pset null '(default)'</literal> to
+ distinguish default privileges from empty privileges.
(Note: for this purpose, <quote>default privileges</quote> always means
the built-in default privileges for the object's type. An object whose
privileges have been affected by an <command>ALTER DEFAULT
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index d94e3cacfc..966697eb50 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3079,7 +3079,8 @@ lo_import 152801
Sets the string to be printed in place of a null value.
The default is to print nothing, which can easily be mistaken for
an empty string. For example, one might prefer <literal>\pset null
- '(null)'</literal>.
+ '(null)'</literal>. This is also useful for distinguishing default
+ privileges from empty privileges as explained in <xref linkend="ddl-priv"/>.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..224aa22575 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -124,7 +124,6 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of aggregate functions");
myopt.translate_header = true;
@@ -197,7 +196,6 @@ describeAccessMethods(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of access methods");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -262,7 +260,6 @@ describeTablespaces(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of tablespaces");
myopt.translate_header = true;
@@ -585,7 +582,6 @@ describeFunctions(const char *functypes, const char *func_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of functions");
myopt.translate_header = true;
if (pset.sversion >= 90600)
@@ -702,7 +698,6 @@ describeTypes(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of data types");
myopt.translate_header = true;
@@ -893,7 +888,6 @@ describeOperators(const char *oper_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators");
myopt.translate_header = true;
@@ -995,7 +989,6 @@ listAllDbs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of databases");
myopt.translate_header = true;
@@ -1146,7 +1139,6 @@ permissionsList(const char *pattern, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1218,7 +1210,6 @@ listDefaultACLs(const char *pattern)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Default access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1417,7 +1408,6 @@ objectDescription(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Object descriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -3852,7 +3842,6 @@ listDbRoleSettings(const char *pattern, const char *pattern2)
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of settings");
myopt.translate_header = true;
@@ -3926,7 +3915,6 @@ describeRoleGrants(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of role grants");
myopt.translate_header = true;
@@ -4122,7 +4110,6 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of relations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4332,7 +4319,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
initPQExpBuffer(&title);
appendPQExpBufferStr(&title, tabletitle);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4412,7 +4398,6 @@ listLanguages(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of languages");
myopt.translate_header = true;
@@ -4497,7 +4482,6 @@ listDomains(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of domains");
myopt.translate_header = true;
@@ -4576,7 +4560,6 @@ listConversions(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of conversions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4644,7 +4627,6 @@ describeConfigurationParameters(const char *pattern, bool verbose,
if (!res)
return false;
- myopt.nullPrint = NULL;
if (pattern)
myopt.title = _("List of configuration parameters");
else
@@ -4726,7 +4708,6 @@ listEventTriggers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of event triggers");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4825,7 +4806,6 @@ listExtendedStats(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of extended statistics");
myopt.translate_header = true;
@@ -4938,7 +4918,6 @@ listCasts(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of casts");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5057,7 +5036,6 @@ listCollations(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of collations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5119,7 +5097,6 @@ listSchemas(const char *pattern, bool verbose, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
myopt.title = _("List of schemas");
myopt.translate_header = true;
@@ -5236,7 +5213,6 @@ listTSParsers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search parsers");
myopt.translate_header = true;
@@ -5384,7 +5360,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
if (nspname)
printfPQExpBuffer(&title, _("Text search parser \"%s.%s\""),
@@ -5421,7 +5396,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
return false;
}
- myopt.nullPrint = NULL;
if (nspname)
printfPQExpBuffer(&title, _("Token types for parser \"%s.%s\""),
nspname, prsname);
@@ -5497,7 +5471,6 @@ listTSDictionaries(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search dictionaries");
myopt.translate_header = true;
@@ -5563,7 +5536,6 @@ listTSTemplates(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search templates");
myopt.translate_header = true;
@@ -5618,7 +5590,6 @@ listTSConfigs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search configurations");
myopt.translate_header = true;
@@ -5764,7 +5735,6 @@ describeOneTSConfig(const char *oid, const char *nspname, const char *cfgname,
appendPQExpBuffer(&title, _("\nParser: \"%s\""),
prsname);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.footers = NULL;
myopt.topt.default_footer = false;
@@ -5841,7 +5811,6 @@ listForeignDataWrappers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign-data wrappers");
myopt.translate_header = true;
@@ -5918,7 +5887,6 @@ listForeignServers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign servers");
myopt.translate_header = true;
@@ -5974,7 +5942,6 @@ listUserMappings(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of user mappings");
myopt.translate_header = true;
@@ -6047,7 +6014,6 @@ listForeignTables(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign tables");
myopt.translate_header = true;
@@ -6099,7 +6065,6 @@ listExtensions(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of installed extensions");
myopt.translate_header = true;
@@ -6203,7 +6168,6 @@ listOneExtensionContents(const char *extname, const char *oid)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
printfPQExpBuffer(&title, _("Objects in extension \"%s\""), extname);
myopt.title = title.data;
@@ -6340,7 +6304,6 @@ listPublications(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of publications");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6695,7 +6658,6 @@ describeSubscriptions(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of subscriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6808,7 +6770,6 @@ listOperatorClasses(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator classes");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6897,7 +6858,6 @@ listOperatorFamilies(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6996,7 +6956,6 @@ listOpFamilyOperators(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7089,7 +7048,6 @@ listOpFamilyFunctions(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of support functions of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7141,7 +7099,6 @@ listLargeObjects(bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Large objects");
myopt.translate_header = true;
--
2.42.0
On Sun, Oct 8, 2023 at 6:55 PM Erik Wienhold <ewie@ewie.name> wrote:
On 2023-10-08 06:14 +0200, Laurenz Albe write:
On Sat, 2023-10-07 at 20:41 +0200, Erik Wienhold wrote:
If you are happy enough with my patch, shall we mark it as ready for
committer?I amended your patch to also document the effect of \pset null in this
case. See the attached v2.+1
If you mention in ddl.sgml that you can use "\pset null" to distinguish
default from no privileges, you should mention that this only works with
psql. Many people out there don't use psql.I don't think this is necessary because that section in ddl.sgml is
already about psql and \dp.
I agree that we are simply detailing how psql makes this information
available to the reader and leave users of other clients on their own to
figure out their own methods - which many clients probably have handled for
them anyway.
For us, I would suggest the following wording:
In addition to the situation of printing all acl items, the Access and
Column privileges columns report two other situations specially. In the
rare case where all privileges for an object have been explicitly removed,
including from the owner and PUBLIC, (i.e., has empty privileges) these
columns will display NULL. The other case is where the built-in default
privileges are in effect, in which case these columns will display the
empty string. (Note that by default psql will print NULL as an empty
string, so in order to visually distinguish these two cases you will need
to issue the \pset null meta-command and choose some other string to print
for NULLs). Built-in default privileges include all privileges for the
owner, as well as those granted to PUBLIC per for relevant object types as
described above. The built-in default privileges are only in effect if the
object has not been the target of a GRANT or REVOKE and also has not had
its default privileges modified using ALTER DEFAULT PRIVILEGES. (???: if it
is possible to revert back to the state of built-in privileges that would
need to be described here.)
The above removes the parenthetical regarding null in the catalogs, this is
intentional as it seems that the goal here is to use psql instead of the
catalogs and adding its use of null being printed as the empty string just
seems likely to add confusion.
Also, I'm not sure if "zero privileges" will be readily understood by
everybody. Perhaps "no privileges at all, even for the object owner"
would be a better wording.Changed in v3 to "empty privileges" with an explanation that this means
"no privileges at all, even for the object owner".
+1
We probably should add the two terms to the glossary:
empty privileges: all privileges explicitly revoked from the owner and
PUBLIC (where applicable), and none otherwise granted.
built-in default privileges: owner having all privileges and no privileges
granted or removed via ALTER DEFAULT PRIVILEGES
Perhaps it would also be good to mention this in the psql documentation.
Just once under \pset null with a reference to section 5.7? Something
like "This is also useful for distinguishing default privileges from
empty privileges as explained in Section 5.7."Or instead under every command affected by this change? \dp and \ddp
already have such a reference ("The meaning of the privilege display is
explained in Section 5.7.")I prefer the first one because it's less effort ;) Also done in v3.
We've chosen a poor default and I'd rather inform the user of specific
meta-commands to be wary of this poor default and change it at the point
they will be learning about the meta-commands adversely affected.
That said, I'd be willing to document that these commands, because they are
affected by empty string versus null, require a non-empty-string value for
\pset null and will choose the string '<null>' for the duration of the
meta-command's execution if the user's setting is incompatible.
David J.
On Mon, 2023-10-09 at 03:53 +0200, Erik Wienhold wrote:
On 2023-10-08 06:14 +0200, Laurenz Albe write:
On Sat, 2023-10-07 at 20:41 +0200, Erik Wienhold wrote:
If you are happy enough with my patch, shall we mark it as ready for
committer?I amended your patch to also document the effect of \pset null in this
case. See the attached v2.+1
If you mention in ddl.sgml that you can use "\pset null" to distinguish
default from no privileges, you should mention that this only works with
psql. Many people out there don't use psql.I don't think this is necessary because that section in ddl.sgml is
already about psql and \dp.
You are right.
Also, I'm not sure if "zero privileges" will be readily understood by
everybody. Perhaps "no privileges at all, even for the object owner"
would be a better wording.Changed in v3 to "empty privileges" with an explanation that this means
"no privileges at all, even for the object owner".
Looks good.
Perhaps it would also be good to mention this in the psql documentation.
Just once under \pset null with a reference to section 5.7? Something
like "This is also useful for distinguishing default privileges from
empty privileges as explained in Section 5.7."Or instead under every command affected by this change? \dp and \ddp
already have such a reference ("The meaning of the privilege display is
explained in Section 5.7.")I prefer the first one because it's less effort ;) Also done in v3.
I think that sufficient.
I tinkered a bit with your documentation. For example, the suggestion to
"\pset null" seemed to be in an inappropriate place. Tell me what you think.
Yours,
Laurenz Albe
Attachments:
v4-0001-psql-honor-pset-null-in-backslash-commands.patchtext/x-patch; charset=UTF-8; name=v4-0001-psql-honor-pset-null-in-backslash-commands.patchDownload
From 2be3e63a6330cbf7767e19f86940ae97f87bf5f2 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Mon, 9 Oct 2023 09:49:20 +0200
Subject: [PATCH] psql: honor "\pset null" in backslash commands
In the output of backslash commands, NULL was always displayed
as empty string, rather than honoring the values set with
"\pset null".
This was surprising, and in some cases it was downright confusing:
for example, default privileges (stored as NULL) were displayed
just like an empty aclitem[], making these cases undistinguishable
in the output. Add this missing detail to the documentation.
Discussion: https://postgr.es/m/96d6885a-5e25-9ae8-4a1a-d7e557a5fe9c%40mtneva.com
---
doc/src/sgml/ddl.sgml | 7 ++++--
doc/src/sgml/ref/psql-ref.sgml | 4 +++-
src/bin/psql/describe.c | 43 ----------------------------------
3 files changed, 8 insertions(+), 46 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 075ff32991..52f17e79ed 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2353,8 +2353,11 @@ 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 always include all privileges for the owner, and can include
+ privileges entry in the relevant system catalog is null) or empty privileges
+ (that is, no privileges at all, even for the object owner — a rare
+ occurrence). One way to distinguish default privileges from empty privileges
+ is to set <userinput>\pset null '(default)'</userinput>.
+ 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>
or <command>REVOKE</command> on an object will instantiate the default
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index d94e3cacfc..7cd12eb867 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3079,7 +3079,9 @@ lo_import 152801
Sets the string to be printed in place of a null value.
The default is to print nothing, which can easily be mistaken for
an empty string. For example, one might prefer <literal>\pset null
- '(null)'</literal>.
+ '(null)'</literal>. Setting a non-empty null display string will
+ help to distinguish between default and empty privileges, as
+ explained in <xref linkend="ddl-priv"/>.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..224aa22575 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -124,7 +124,6 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of aggregate functions");
myopt.translate_header = true;
@@ -197,7 +196,6 @@ describeAccessMethods(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of access methods");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -262,7 +260,6 @@ describeTablespaces(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of tablespaces");
myopt.translate_header = true;
@@ -585,7 +582,6 @@ describeFunctions(const char *functypes, const char *func_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of functions");
myopt.translate_header = true;
if (pset.sversion >= 90600)
@@ -702,7 +698,6 @@ describeTypes(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of data types");
myopt.translate_header = true;
@@ -893,7 +888,6 @@ describeOperators(const char *oper_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators");
myopt.translate_header = true;
@@ -995,7 +989,6 @@ listAllDbs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of databases");
myopt.translate_header = true;
@@ -1146,7 +1139,6 @@ permissionsList(const char *pattern, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1218,7 +1210,6 @@ listDefaultACLs(const char *pattern)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Default access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1417,7 +1408,6 @@ objectDescription(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Object descriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -3852,7 +3842,6 @@ listDbRoleSettings(const char *pattern, const char *pattern2)
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of settings");
myopt.translate_header = true;
@@ -3926,7 +3915,6 @@ describeRoleGrants(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of role grants");
myopt.translate_header = true;
@@ -4122,7 +4110,6 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of relations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4332,7 +4319,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
initPQExpBuffer(&title);
appendPQExpBufferStr(&title, tabletitle);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4412,7 +4398,6 @@ listLanguages(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of languages");
myopt.translate_header = true;
@@ -4497,7 +4482,6 @@ listDomains(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of domains");
myopt.translate_header = true;
@@ -4576,7 +4560,6 @@ listConversions(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of conversions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4644,7 +4627,6 @@ describeConfigurationParameters(const char *pattern, bool verbose,
if (!res)
return false;
- myopt.nullPrint = NULL;
if (pattern)
myopt.title = _("List of configuration parameters");
else
@@ -4726,7 +4708,6 @@ listEventTriggers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of event triggers");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4825,7 +4806,6 @@ listExtendedStats(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of extended statistics");
myopt.translate_header = true;
@@ -4938,7 +4918,6 @@ listCasts(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of casts");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5057,7 +5036,6 @@ listCollations(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of collations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5119,7 +5097,6 @@ listSchemas(const char *pattern, bool verbose, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
myopt.title = _("List of schemas");
myopt.translate_header = true;
@@ -5236,7 +5213,6 @@ listTSParsers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search parsers");
myopt.translate_header = true;
@@ -5384,7 +5360,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
if (nspname)
printfPQExpBuffer(&title, _("Text search parser \"%s.%s\""),
@@ -5421,7 +5396,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
return false;
}
- myopt.nullPrint = NULL;
if (nspname)
printfPQExpBuffer(&title, _("Token types for parser \"%s.%s\""),
nspname, prsname);
@@ -5497,7 +5471,6 @@ listTSDictionaries(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search dictionaries");
myopt.translate_header = true;
@@ -5563,7 +5536,6 @@ listTSTemplates(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search templates");
myopt.translate_header = true;
@@ -5618,7 +5590,6 @@ listTSConfigs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search configurations");
myopt.translate_header = true;
@@ -5764,7 +5735,6 @@ describeOneTSConfig(const char *oid, const char *nspname, const char *cfgname,
appendPQExpBuffer(&title, _("\nParser: \"%s\""),
prsname);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.footers = NULL;
myopt.topt.default_footer = false;
@@ -5841,7 +5811,6 @@ listForeignDataWrappers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign-data wrappers");
myopt.translate_header = true;
@@ -5918,7 +5887,6 @@ listForeignServers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign servers");
myopt.translate_header = true;
@@ -5974,7 +5942,6 @@ listUserMappings(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of user mappings");
myopt.translate_header = true;
@@ -6047,7 +6014,6 @@ listForeignTables(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign tables");
myopt.translate_header = true;
@@ -6099,7 +6065,6 @@ listExtensions(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of installed extensions");
myopt.translate_header = true;
@@ -6203,7 +6168,6 @@ listOneExtensionContents(const char *extname, const char *oid)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
printfPQExpBuffer(&title, _("Objects in extension \"%s\""), extname);
myopt.title = title.data;
@@ -6340,7 +6304,6 @@ listPublications(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of publications");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6695,7 +6658,6 @@ describeSubscriptions(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of subscriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6808,7 +6770,6 @@ listOperatorClasses(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator classes");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6897,7 +6858,6 @@ listOperatorFamilies(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6996,7 +6956,6 @@ listOpFamilyOperators(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7089,7 +7048,6 @@ listOpFamilyFunctions(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of support functions of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7141,7 +7099,6 @@ listLargeObjects(bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Large objects");
myopt.translate_header = true;
--
2.41.0
On Sun, 2023-10-08 at 19:58 -0700, David G. Johnston wrote:
For us, I would suggest the following wording:
In addition to the situation of printing all acl items, the Access and Column
privileges columns report two other situations specially. In the rare case
where all privileges for an object have been explicitly removed, including
from the owner and PUBLIC, (i.e., has empty privileges) these columns will
display NULL. The other case is where the built-in default privileges are
in effect, in which case these columns will display the empty string.
(Note that by default psql will print NULL as an empty string, so in order
to visually distinguish these two cases you will need to issue the \pset null
meta-command and choose some other string to print for NULLs). Built-in
default privileges include all privileges for the owner, as well as those
granted to PUBLIC per for relevant object types as described above.
That doesn't look like an improvement over the latest patches to me.
The built-in default privileges are only in effect if the object has not been
the target of a GRANT or REVOKE and also has not had its default privileges
modified using ALTER DEFAULT PRIVILEGES. (???: if it is possible to revert
back to the state of built-in privileges that would need to be described here.)
I don't think that we need to mention ALTER DEFAULT PRIVILEGES there. If
the default privileges have been altered, the ACL will not be stored as
NULL in the catalogs.
The above removes the parenthetical regarding null in the catalogs, this is
intentional as it seems that the goal here is to use psql instead of the
catalogs and adding its use of null being printed as the empty string just
seems likely to add confusion.
To me, mentioning the default privileges are stored as NULLs in the catalogs
is not an invitation to view the privileges with catalog queries, but
information about implementation details that explains why default privileges
are displayed the way they are.
We probably should add the two terms to the glossary:
empty privileges: all privileges explicitly revoked from the owner and PUBLIC
(where applicable), and none otherwise granted.built-in default privileges: owner having all privileges and no privileges
granted or removed via ALTER DEFAULT PRIVILEGES
"Empty privileges" are too unimportant to warrant an index entry.
I can see the value of an index entry
<indexterm>
<primary>privilege</primary>
<secondary>default</secondary>
</indexterm>
Done in the attached v5 of the patch, even though this is not really
the business of this patch.
Perhaps it would also be good to mention this in the psql documentation.
We've chosen a poor default and I'd rather inform the user of specific meta-commands
to be wary of this poor default and change it at the point they will be learning
about the meta-commands adversely affected.That said, I'd be willing to document that these commands, because they are affected
by empty string versus null, require a non-empty-string value for \pset null and will
choose the string '<null>' for the duration of the meta-command's execution if the
user's setting is incompatible.
I am not certain I understood you correctly.
Are you advocating for adding a mention of "\pset null" to every backslash command
that displays privileges? That is excessive, in my opinion.
Yours,
Laurenz Albe
Attachments:
v5-0001-psql-honor-pset-null-in-backslash-commands.patchtext/x-patch; charset=UTF-8; name=v5-0001-psql-honor-pset-null-in-backslash-commands.patchDownload
From 2afe3cbf674e163c146ea29582f7aa3839bd184d Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Mon, 9 Oct 2023 10:27:58 +0200
Subject: [PATCH] psql: honor "\pset null" in backslash commands
In the output of backslash commands, NULL was always displayed
as empty string, rather than honoring the values set with
"\pset null".
This was surprising, and in some cases it was downright confusing:
for example, default privileges (stored as NULL) were displayed
just like an empty aclitem[], making these cases undistinguishable
in the output. Add this missing detail to the documentation.
In passing, add entry for "privileges, default" to the index.
Discussion: https://postgr.es/m/96d6885a-5e25-9ae8-4a1a-d7e557a5fe9c%40mtneva.com
---
doc/src/sgml/ddl.sgml | 14 ++++++++---
doc/src/sgml/ref/psql-ref.sgml | 4 +++-
src/bin/psql/describe.c | 43 ----------------------------------
3 files changed, 14 insertions(+), 47 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 075ff32991..76e62250e4 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -1737,6 +1737,11 @@ ALTER TABLE products RENAME TO items;
<primary>ACL</primary>
</indexterm>
+ <indexterm zone="ddl-priv-default">
+ <primary>privilege</primary>
+ <secondary>default</secondary>
+ </indexterm>
+
<para>
When an object is created, it is assigned an owner. The
owner is normally the role that executed the creation statement.
@@ -2049,7 +2054,7 @@ REVOKE ALL ON accounts FROM PUBLIC;
reference page of the respective command.
</para>
- <para>
+ <para id="ddl-priv-default">
PostgreSQL grants privileges on some types of objects to
<literal>PUBLIC</literal> by default when the objects are created.
No privileges are granted to <literal>PUBLIC</literal> by default on
@@ -2353,8 +2358,11 @@ 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 always include all privileges for the owner, and can include
+ privileges entry in the relevant system catalog is null) or empty privileges
+ (that is, no privileges at all, even for the object owner — a rare
+ occurrence). One way to distinguish default privileges from empty privileges
+ is to set <userinput>\pset null '(default)'</userinput>.
+ 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>
or <command>REVOKE</command> on an object will instantiate the default
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index d94e3cacfc..7cd12eb867 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3079,7 +3079,9 @@ lo_import 152801
Sets the string to be printed in place of a null value.
The default is to print nothing, which can easily be mistaken for
an empty string. For example, one might prefer <literal>\pset null
- '(null)'</literal>.
+ '(null)'</literal>. Setting a non-empty null display string will
+ help to distinguish between default and empty privileges, as
+ explained in <xref linkend="ddl-priv"/>.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..224aa22575 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -124,7 +124,6 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of aggregate functions");
myopt.translate_header = true;
@@ -197,7 +196,6 @@ describeAccessMethods(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of access methods");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -262,7 +260,6 @@ describeTablespaces(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of tablespaces");
myopt.translate_header = true;
@@ -585,7 +582,6 @@ describeFunctions(const char *functypes, const char *func_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of functions");
myopt.translate_header = true;
if (pset.sversion >= 90600)
@@ -702,7 +698,6 @@ describeTypes(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of data types");
myopt.translate_header = true;
@@ -893,7 +888,6 @@ describeOperators(const char *oper_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators");
myopt.translate_header = true;
@@ -995,7 +989,6 @@ listAllDbs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of databases");
myopt.translate_header = true;
@@ -1146,7 +1139,6 @@ permissionsList(const char *pattern, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1218,7 +1210,6 @@ listDefaultACLs(const char *pattern)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Default access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1417,7 +1408,6 @@ objectDescription(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Object descriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -3852,7 +3842,6 @@ listDbRoleSettings(const char *pattern, const char *pattern2)
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of settings");
myopt.translate_header = true;
@@ -3926,7 +3915,6 @@ describeRoleGrants(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of role grants");
myopt.translate_header = true;
@@ -4122,7 +4110,6 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of relations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4332,7 +4319,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
initPQExpBuffer(&title);
appendPQExpBufferStr(&title, tabletitle);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4412,7 +4398,6 @@ listLanguages(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of languages");
myopt.translate_header = true;
@@ -4497,7 +4482,6 @@ listDomains(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of domains");
myopt.translate_header = true;
@@ -4576,7 +4560,6 @@ listConversions(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of conversions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4644,7 +4627,6 @@ describeConfigurationParameters(const char *pattern, bool verbose,
if (!res)
return false;
- myopt.nullPrint = NULL;
if (pattern)
myopt.title = _("List of configuration parameters");
else
@@ -4726,7 +4708,6 @@ listEventTriggers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of event triggers");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4825,7 +4806,6 @@ listExtendedStats(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of extended statistics");
myopt.translate_header = true;
@@ -4938,7 +4918,6 @@ listCasts(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of casts");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5057,7 +5036,6 @@ listCollations(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of collations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5119,7 +5097,6 @@ listSchemas(const char *pattern, bool verbose, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
myopt.title = _("List of schemas");
myopt.translate_header = true;
@@ -5236,7 +5213,6 @@ listTSParsers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search parsers");
myopt.translate_header = true;
@@ -5384,7 +5360,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
if (nspname)
printfPQExpBuffer(&title, _("Text search parser \"%s.%s\""),
@@ -5421,7 +5396,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
return false;
}
- myopt.nullPrint = NULL;
if (nspname)
printfPQExpBuffer(&title, _("Token types for parser \"%s.%s\""),
nspname, prsname);
@@ -5497,7 +5471,6 @@ listTSDictionaries(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search dictionaries");
myopt.translate_header = true;
@@ -5563,7 +5536,6 @@ listTSTemplates(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search templates");
myopt.translate_header = true;
@@ -5618,7 +5590,6 @@ listTSConfigs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search configurations");
myopt.translate_header = true;
@@ -5764,7 +5735,6 @@ describeOneTSConfig(const char *oid, const char *nspname, const char *cfgname,
appendPQExpBuffer(&title, _("\nParser: \"%s\""),
prsname);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.footers = NULL;
myopt.topt.default_footer = false;
@@ -5841,7 +5811,6 @@ listForeignDataWrappers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign-data wrappers");
myopt.translate_header = true;
@@ -5918,7 +5887,6 @@ listForeignServers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign servers");
myopt.translate_header = true;
@@ -5974,7 +5942,6 @@ listUserMappings(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of user mappings");
myopt.translate_header = true;
@@ -6047,7 +6014,6 @@ listForeignTables(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign tables");
myopt.translate_header = true;
@@ -6099,7 +6065,6 @@ listExtensions(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of installed extensions");
myopt.translate_header = true;
@@ -6203,7 +6168,6 @@ listOneExtensionContents(const char *extname, const char *oid)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
printfPQExpBuffer(&title, _("Objects in extension \"%s\""), extname);
myopt.title = title.data;
@@ -6340,7 +6304,6 @@ listPublications(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of publications");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6695,7 +6658,6 @@ describeSubscriptions(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of subscriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6808,7 +6770,6 @@ listOperatorClasses(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator classes");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6897,7 +6858,6 @@ listOperatorFamilies(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6996,7 +6956,6 @@ listOpFamilyOperators(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7089,7 +7048,6 @@ listOpFamilyFunctions(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of support functions of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7141,7 +7099,6 @@ listLargeObjects(bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Large objects");
myopt.translate_header = true;
--
2.41.0
On Mon, Oct 9, 2023 at 1:29 AM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:
On Sun, 2023-10-08 at 19:58 -0700, David G. Johnston wrote:
The built-in default privileges are only in effect if the object has not
been
the target of a GRANT or REVOKE and also has not had its default
privileges
modified using ALTER DEFAULT PRIVILEGES. (???: if it is possible to
revert
back to the state of built-in privileges that would need to be described
here.)
I don't think that we need to mention ALTER DEFAULT PRIVILEGES there. If
the default privileges have been altered, the ACL will not be stored as
NULL in the catalogs.
It's already mentioned there, I just felt bringing the mention of ADP and
grant/revoke both invalidating the built-in default privileges closer
together would be an improvement.
The above removes the parenthetical regarding null in the catalogs, this
is
intentional as it seems that the goal here is to use psql instead of the
catalogs and adding its use of null being printed as the empty stringjust
seems likely to add confusion.
To me, mentioning the default privileges are stored as NULLs in the
catalogs
is not an invitation to view the privileges with catalog queries, but
information about implementation details that explains why default
privileges
are displayed the way they are.
Calling it an implementation detail leads me to conclude the point does not
belong in the user-facing administration documentation.
Perhaps it would also be good to mention this in the psql
documentation.
We've chosen a poor default and I'd rather inform the user of specific
meta-commands
to be wary of this poor default and change it at the point they will be
learning
about the meta-commands adversely affected.
That said, I'd be willing to document that these commands, because they
are affected
by empty string versus null, require a non-empty-string value for \pset
null and will
choose the string '<null>' for the duration of the meta-command's
execution if the
user's setting is incompatible.
I am not certain I understood you correctly.
Are you advocating for adding a mention of "\pset null" to every backslash
command
that displays privileges? That is excessive, in my opinion.
Yes, I am. I suppose the argument that any user of those commands is
expected to have read the ddl/privileges chapter would suffice for me
though.
My point with the second paragraph is that we could, instead of documenting
the caveat about null printing as empty strings is to instead issue an
implicit "\pset null '<null>'" as part of these commands, and a "\pset
null" afterward, conditioned upon it not already being set to a non-empty
value. IOW, the special-casing we do today but actually do it in a way
that distinguishes the two cases instead of forcing them to be
indistinguishable.
David J.
On Mon, 2023-10-09 at 09:30 -0700, David G. Johnston wrote:
On Mon, Oct 9, 2023 at 1:29 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
On Sun, 2023-10-08 at 19:58 -0700, David G. Johnston wrote:
The built-in default privileges are only in effect if the object has not been
the target of a GRANT or REVOKE and also has not had its default privileges
modified using ALTER DEFAULT PRIVILEGES. (???: if it is possible to revert
back to the state of built-in privileges that would need to be described here.)I don't think that we need to mention ALTER DEFAULT PRIVILEGES there. If
the default privileges have been altered, the ACL will not be stored as
NULL in the catalogs.It's already mentioned there, I just felt bringing the mention of ADP and
grant/revoke both invalidating the built-in default privileges closer together
would be an improvement.
Ah, sorry, I misread your comment. You are right. But then, the effects of
ALTER DEFAULT PRIVILEGES are discussed later in the paragraph anyway, and we don't
have to repeat that here.
To me, mentioning the default privileges are stored as NULLs in the catalogs
is not an invitation to view the privileges with catalog queries, but
information about implementation details that explains why default privileges
are displayed the way they are.Calling it an implementation detail leads me to conclude the point does not
belong in the user-facing administration documentation.
Again, you have a point there. However, I find that detail useful, as it explains
the user-facing behavior. Anyway, I don't think it is the job of this patch to
remove that pre-existing detail.
Are you advocating for adding a mention of "\pset null" to every backslash command
that displays privileges? That is excessive, in my opinion.Yes, I am. I suppose the argument that any user of those commands is expected
to have read the ddl/privileges chapter would suffice for me though.
Thanks. Then let's leave it like that.
My point with the second paragraph is that we could, instead of documenting the
caveat about null printing as empty strings is to instead issue an implicit
"\pset null '<null>'" as part of these commands, and a "\pset null" afterward,
conditioned upon it not already being set to a non-empty value. IOW, the
special-casing we do today but actually do it in a way that distinguishes the
two cases instead of forcing them to be indistinguishable.
-1
The whole point of this patch is to make psql behave consistently with respect to
NULLs in meta-commands. Your suggestion would subvert that idea.
Yours,
Laurenz Albe
Laurenz Albe <laurenz.albe@cybertec.at> writes:
On Mon, 2023-10-09 at 09:30 -0700, David G. Johnston wrote:
My point with the second paragraph is that we could, instead of documenting the
caveat about null printing as empty strings is to instead issue an implicit
"\pset null '<null>'" as part of these commands, and a "\pset null" afterward,
conditioned upon it not already being set to a non-empty value. IOW, the
special-casing we do today but actually do it in a way that distinguishes the
two cases instead of forcing them to be indistinguishable.
-1
The whole point of this patch is to make psql behave consistently with respect to
NULLs in meta-commands. Your suggestion would subvert that idea.
Yeah. There is a lot of attraction in having \pset null affect these
displays just like all other ones. The fact that they act differently
now is a wart, not something we should replace with a different special
case behavior.
Also, I'm fairly concerned about not changing the default behavior here.
The fact that this behavior has stood for a couple dozen years without
many complaints indicates that there's not all that big a problem to be
solved. I doubt that a new default behavior will be well received,
even if it's arguably better.
regards, tom lane
On Mon, Oct 9, 2023 at 12:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Laurenz Albe <laurenz.albe@cybertec.at> writes:
On Mon, 2023-10-09 at 09:30 -0700, David G. Johnston wrote:
My point with the second paragraph is that we could, instead of
documenting the
caveat about null printing as empty strings is to instead issue an
implicit
"\pset null '<null>'" as part of these commands, and a "\pset null"
afterward,
conditioned upon it not already being set to a non-empty value. IOW,
the
special-casing we do today but actually do it in a way that
distinguishes the
two cases instead of forcing them to be indistinguishable.
-1
The whole point of this patch is to make psql behave consistently with
respect to
NULLs in meta-commands. Your suggestion would subvert that idea.
Yeah. There is a lot of attraction in having \pset null affect these
displays just like all other ones. The fact that they act differently
now is a wart, not something we should replace with a different special
case behavior.Also, I'm fairly concerned about not changing the default behavior here.
The fact that this behavior has stood for a couple dozen years without
many complaints indicates that there's not all that big a problem to be
solved. I doubt that a new default behavior will be well received,
even if it's arguably better.
My position is that the default behavior should be changed such that the
distinction these reports need to make between empty privileges and default
privileges is impossible for the user to remove. I suppose the best direct
solution given that goal is for the query to simply do away with any
reliance on NULL being an indicator. Output an i18n'd "no permissions
present" line in the rare empty permissions situation and leave the empty
string for built-in default.
If the consensus is to not actually fix these views to make them
environment invariant in their accuracy then so be it. Having them obey
\pset null seems like a half-measure but it at least is an improvement over
the status quo such that users are capable of altering their system to make
the reports distinguish the two cases if they realize the need.
I do agree that my suggestion regarding the implicit \pset null, while
plausible, leaves the wart that NULL is being used to mean something
specific. Better is to just use a label for that specific thing.
David J.
On Mon, 2023-10-09 at 15:13 -0400, Tom Lane wrote:
Laurenz Albe <laurenz.albe@cybertec.at> writes:
The whole point of this patch is to make psql behave consistently with respect to
NULLs in meta-commands.Yeah. There is a lot of attraction in having \pset null affect these
displays just like all other ones. The fact that they act differently
now is a wart, not something we should replace with a different special
case behavior.Also, I'm fairly concerned about not changing the default behavior here.
The fact that this behavior has stood for a couple dozen years without
many complaints indicates that there's not all that big a problem to be
solved. I doubt that a new default behavior will be well received,
even if it's arguably better.
I understand your concern. People who have "\pset null" in their .psqlrc
might be surprised if suddenly "<null>" starts appearing in the outout
of \l.
But then the people who have "\pset null" in their .psqlrc are typically
already somewhat experienced and might have less trouble dealing with that
change (but they are more likely to bleat on the mailing list, granted).
Users with little experience won't notice any difference, so they won't
be confused by the change.
Yours,
Laurenz Albe
On 2023-10-09 09:54 +0200, Laurenz Albe write:
I tinkered a bit with your documentation. For example, the suggestion to
"\pset null" seemed to be in an inappropriate place. Tell me what you think.
+1 You're right to put that sentence right after the explanation of
empty privileges.
--
Erik
On 2023-10-09 10:29 +0200, Laurenz Albe write:
On Sun, 2023-10-08 at 19:58 -0700, David G. Johnston wrote:
We probably should add the two terms to the glossary:
empty privileges: all privileges explicitly revoked from the owner and PUBLIC
(where applicable), and none otherwise granted.built-in default privileges: owner having all privileges and no privileges
granted or removed via ALTER DEFAULT PRIVILEGES"Empty privileges" are too unimportant to warrant an index entry.
I can see the value of an index entry
<indexterm>
<primary>privilege</primary>
<secondary>default</secondary>
</indexterm>Done in the attached v5 of the patch, even though this is not really
the business of this patch.
+1
--
Erik
On 2023-10-09 22:34 +0200, David G. Johnston write:
On Mon, Oct 9, 2023 at 12:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Yeah. There is a lot of attraction in having \pset null affect these
displays just like all other ones. The fact that they act differently
now is a wart, not something we should replace with a different special
case behavior.Also, I'm fairly concerned about not changing the default behavior here.
The fact that this behavior has stood for a couple dozen years without
many complaints indicates that there's not all that big a problem to be
solved. I doubt that a new default behavior will be well received,
even if it's arguably better.My position is that the default behavior should be changed such that the
distinction these reports need to make between empty privileges and default
privileges is impossible for the user to remove. I suppose the best direct
solution given that goal is for the query to simply do away with any
reliance on NULL being an indicator. Output an i18n'd "no permissions
present" line in the rare empty permissions situation and leave the empty
string for built-in default.
My initial patch does exactly that.
If the consensus is to not actually fix these views to make them
environment invariant in their accuracy then so be it. Having them obey
\pset null seems like a half-measure but it at least is an improvement over
the status quo such that users are capable of altering their system to make
the reports distinguish the two cases if they realize the need.
I agree.
--
Erik
On Sat, 2023-10-14 at 02:45 +0200, Erik Wienhold wrote:
On 2023-10-09 09:54 +0200, Laurenz Albe write:
I tinkered a bit with your documentation. For example, the suggestion to
"\pset null" seemed to be in an inappropriate place. Tell me what you think.+1 You're right to put that sentence right after the explanation of
empty privileges.
Thanks for looking.
David, how do you feel about this? I am wondering whether to set this patch
"ready for committer" or not.
There is Tom wondering upthread whether changing psql's behavior like that
is too much of a compatibility break or not, but I guess it is alright to
leave that final verdict to a committer.
Yours,
Laurenz Albe
On 2023-10-16 17:56 +0200, Laurenz Albe write:
David, how do you feel about this? I am wondering whether to set this patch
"ready for committer" or not.There is Tom wondering upthread whether changing psql's behavior like that
is too much of a compatibility break or not, but I guess it is alright to
leave that final verdict to a committer.
What's the process for the CommitFest now since we settled on your
patch? This is my first time being involved in this, so still learning.
I'see that you've withdrawn your initial patch [1]https://commitfest.postgresql.org/45/4603/ but this thread is
still attached to my patch [2]https://commitfest.postgresql.org/45/4593/. Should I edit my CF entry (or withdraw
it) and you reactivate yours?
[1]: https://commitfest.postgresql.org/45/4603/
[2]: https://commitfest.postgresql.org/45/4593/
--
Erik
On Mon, 2023-10-16 at 23:51 +0200, Erik Wienhold wrote:
What's the process for the CommitFest now since we settled on your
patch? This is my first time being involved in this, so still learning.
I'see that you've withdrawn your initial patch [1] but this thread is
still attached to my patch [2]. Should I edit my CF entry (or withdraw
it) and you reactivate yours?
I don't think it makes sense to have two competing commitfest entries,
and we should leave it on this thread. If you are concerned about
authorship, we could both be mentioned as co-authors, if the patch ever
gets committed.
I'd still like to wait for feedback from David before I change anything.
Yours,
Laurenz Albe
On Mon, Oct 16, 2023 at 6:19 PM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:
On Mon, 2023-10-16 at 23:51 +0200, Erik Wienhold wrote:
What's the process for the CommitFest now since we settled on your
patch? This is my first time being involved in this, so still learning.
I'see that you've withdrawn your initial patch [1] but this thread is
still attached to my patch [2]. Should I edit my CF entry (or withdraw
it) and you reactivate yours?I don't think it makes sense to have two competing commitfest entries,
and we should leave it on this thread. If you are concerned about
authorship, we could both be mentioned as co-authors, if the patch ever
gets committed.I'd still like to wait for feedback from David before I change anything.
Reading both threads I'm not seeing any specific rejection of the solution
that we simply represent empty privileges as "(none)".
I see an apparent consensus that if we do continue to represent it as NULL
that the printout should respect \pset null.
Tom commented in favor of (none) on the other thread with some commentary
regarding how extremely rare it is; then turns around and is "fairly
concerned" about changing the current blank presentation of its value which
is going to happen for some people regardless of which approach is chosen.
(idk...maybe in favor of (none))
Peter's comment doesn't strike me as recognizing that (none) is even an
option on the table...(n/a)
Stuart, the original user complainant, seems to favor (none)
Erik seems to favors (none)
I favor (none)
It's unclear to me whether you Laurenz are against (none) or just trying to
go with the group consensus that doesn't actually seem to be against (none).
I'm in favor of iterating on v1 on this thread (haven't read it yet) and
presenting it as the proposed solution. If that ends up getting shot down
we can revive v5 (still need to review as well).
We should probably post on that thread that this one exists and post a link
to it.
David J.
On Mon, 2023-10-16 at 19:05 -0700, David G. Johnston wrote:
Reading both threads I'm not seeing any specific rejection of the
solution that we simply represent empty privileges as "(none)".I see an apparent consensus that if we do continue to represent it
as NULL that the printout should respect \pset null.Tom commented in favor of (none) on the other thread with some
commentary regarding how extremely rare it is; then turns around
and is "fairly concerned" about changing the current blank presentation
of its value which is going to happen for some people regardless
of which approach is chosen.Stuart, the original user complainant, seems to favor (none)
Erik seems to favors (none)
I favor (none)
It's unclear to me whether you Laurenz are against (none) or just
trying to go with the group consensus that doesn't actually seem to
be against (none).I'm in favor of iterating on v1 on this thread (haven't read it yet)
and presenting it as the proposed solution. If that ends up getting
shot down we can revive v5 (still need to review as well).
Thanks for that summary. I prefer my version (simply display NULLs
as NULLs), but I am not wedded to it. I had the impression that Tom
would prefer that too, but is woried if the incompatibility introduced
would outweigh the small benefit (of either patch).
So it is clear that we don't have a consensus.
I don't think I want to go ahead with my version of the patch unless
there is more support for it. I can review Erik's original code, if
that design meets with more favor.
We should probably post on that thread that this one exists and post a link to it.
Perhaps a good idea, yes.
Yours,
Laurenz Albe
Laurenz Albe <laurenz.albe@cybertec.at> writes:
On Mon, 2023-10-16 at 19:05 -0700, David G. Johnston wrote:
Reading both threads I'm not seeing any specific rejection of the
solution that we simply represent empty privileges as "(none)".
Thanks for that summary. I prefer my version (simply display NULLs
as NULLs), but I am not wedded to it. I had the impression that Tom
would prefer that too, but is woried if the incompatibility introduced
would outweigh the small benefit (of either patch).
So it is clear that we don't have a consensus.
FWIW, my druthers are to make the describe.c queries honor \pset null
(not only for privileges, but anywhere else they fail to) and do
nothing beyond that. I think that'll generally reduce the surprise
factor, while anything else we might opt to do will increase it.
If that fails to garner a consensus, my second choice would be to
do that plus translate empty-but-not-null ACLs to "(none)".
regards, tom lane
On 2023-10-17 04:05 +0200, David G. Johnston wrote:
Erik seems to favors (none)
Yes, with a slight favor for "(none)" because it's the least disruptive
to users who change \pset null to a non-blank string. The output of \dp
etc. would still look the same for default privileges.
But I'm also okay with respecting \pset null because it is so simple.
We will no longer hide the already documented null representation of
default privileges by allowing the user to display the ACL as it is.
And with Laurenz' patch we will also document the special case of zero
privileges and how to distinguish it.
--
Erik
On Fri, 2023-10-20 at 04:13 +0200, Erik Wienhold wrote:
On 2023-10-17 04:05 +0200, David G. Johnston wrote:
Erik seems to favors (none)
Yes, with a slight favor for "(none)" because it's the least disruptive
to users who change \pset null to a non-blank string. The output of \dp
etc. would still look the same for default privileges.But I'm also okay with respecting \pset null because it is so simple.
We will no longer hide the already documented null representation of
default privileges by allowing the user to display the ACL as it is.
And with Laurenz' patch we will also document the special case of zero
privileges and how to distinguish it.
If you want to proceed with your patch, you could send a new version.
I think it could do with an added line of documentation to the
"Privileges" chapter, and I'd say that the regression tests should be
in "psql.sql" (not that it is very important).
I am not sure how to proceed. Perhaps it would indeed be better to have
two competing commitfest entries. Both could be "ready for committer",
and the committers can decide what they prefer.
Yours,
Laurenz Albe
Laurenz Albe <laurenz.albe@cybertec.at> writes:
I am not sure how to proceed. Perhaps it would indeed be better to have
two competing commitfest entries. Both could be "ready for committer",
and the committers can decide what they prefer.
As near as I can tell, doing both things (the \pset null fix and
substituting "(none)" for empty privileges) would be an acceptable
answer to everyone who has commented. Let's proceed with both
patches, or combine them into one if there are merge conflicts.
regards, tom lane
On Fri, Oct 20, 2023 at 12:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Laurenz Albe <laurenz.albe@cybertec.at> writes:
I am not sure how to proceed. Perhaps it would indeed be better to have
two competing commitfest entries. Both could be "ready for committer",
and the committers can decide what they prefer.As near as I can tell, doing both things (the \pset null fix and
substituting "(none)" for empty privileges) would be an acceptable
answer to everyone who has commented. Let's proceed with both
patches, or combine them into one if there are merge conflicts.
I'm under the impression that removing the null representation of empty
privileges by making them (none) removes all known \d commands that output
nulls and don't obey \pset null. At least, the existing \pset null patch
doesn't purport to fix anything that would become not applicable if the
(none) patch goes in. I.e., at present they are mutually exclusive.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
On Fri, Oct 20, 2023 at 12:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
As near as I can tell, doing both things (the \pset null fix and
substituting "(none)" for empty privileges) would be an acceptable
answer to everyone who has commented. Let's proceed with both
patches, or combine them into one if there are merge conflicts.
I'm under the impression that removing the null representation of empty
privileges by making them (none) removes all known \d commands that output
nulls and don't obey \pset null.
How so? IIUC the proposal is to substitute "(none)" for empty-string
ACLs, not null ACLs. The \pset change should be addressing an
independent case.
regards, tom lane
On Fri, Oct 20, 2023 at 12:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"David G. Johnston" <david.g.johnston@gmail.com> writes:
On Fri, Oct 20, 2023 at 12:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
As near as I can tell, doing both things (the \pset null fix and
substituting "(none)" for empty privileges) would be an acceptable
answer to everyone who has commented. Let's proceed with both
patches, or combine them into one if there are merge conflicts.I'm under the impression that removing the null representation of empty
privileges by making them (none) removes all known \d commands thatoutput
nulls and don't obey \pset null.
How so? IIUC the proposal is to substitute "(none)" for empty-string
ACLs, not null ACLs. The \pset change should be addressing an
independent case.
Ok, I found my mis-understanding and better understand where you are all
coming from now; I apparently had the usage of NULL flip-flopped.
Taking pg_tablespace as an example. Its "spcacl" column produces NULL for
default privileges and '{}'::text[] for empty privileges.
Thus, today:
empty: array_to_string('{}'::text[], '\n') produces an empty string
default: array_to_string(null, '\n') produces null which then was printed
as a hard-coded empty string via forcibly changing \pset null
I was thinking the two cases were reversed.
My proposal for changing printACLColumn is thus:
case when spcacl is null then ''
when cardinality(spcacl) = 0 then '(none)'
else array_to_string(spcacl, E'\\n')
end as "Access privileges"
In short, I don't want default privileges to start to obey \pset null when
it never has before and is documented as displaying the empty string. I do
want the empty string produced by empty privileges to change to (none) so
that it no longer is indistinguishable from our choice of presentation for
the default privilege case.
Mechanically, we remove the existing \pset null for these metacommands and
move it into the query via the added CASE expression in the printACLColumn
function.
This gets rid of NULL as an output value for this column and makes the
patch regarding \pset null discussion unnecessary. And it leaves the
existing well-established empty string for default privileges alone (and
changing this is what I believe Tom is against and I agree on that point).
David J.
On 2023-10-20 08:42 +0200, Laurenz Albe wrote:
If you want to proceed with your patch, you could send a new version.
v2 attached.
I think it could do with an added line of documentation to the
"Privileges" chapter, and I'd say that the regression tests should be
in "psql.sql" (not that it is very important).
I added some docs. There will be merge conflicts when combining with
your v5! Also moved the regression tests to psql.sql which is the right
place for testing meta commands.
--
Erik
Attachments:
v2-0001-Fix-output-of-empty-privileges-in-psql.patchtext/plain; charset=us-asciiDownload
From 170ca82fa7b74cc9102582cdf48042131d5ae016 Mon Sep 17 00:00:00 2001
From: Erik Wienhold <ewie@ewie.name>
Date: Sun, 17 Sep 2023 20:54:48 +0200
Subject: [PATCH v2] Fix output of empty privileges in psql
Print "(none)" for empty privileges instead of nothing so that the user
is able to distinguish empty from default privileges. This affects the
following meta commands:
\db+ \dD+ \des+ \dew+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l
Default privileges start as NULL::aclitem[] in various catalog columns
but revoking the default privileges leaves an empty aclitem array.
Using \pset null '(null)' as a workaround to spot default privileges
does not work because the meta commands ignore this setting.
The privileges shown by \dconfig+ and \ddp as well as the column
privileges shown by \dp are not affected by this change because the
respective aclitem[] is reset to NULL or deleted from the catalog
instead of leaving empty arrays.
Commands \des+ and \dew+ are not covered in src/test/regress because no
foreign data wrapper is available at this point to create a foreign
server.
---
doc/src/sgml/ddl.sgml | 4 +-
src/bin/psql/describe.c | 6 +-
src/test/regress/expected/psql.out | 94 ++++++++++++++++++++++++++++++
src/test/regress/sql/psql.sql | 45 ++++++++++++++
4 files changed, 147 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 075ff32991..1c17c4a967 100644
--- 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>
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..154d244d97 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6718,7 +6718,11 @@ static void
printACLColumn(PQExpBuffer buf, const char *colname)
{
appendPQExpBuffer(buf,
- "pg_catalog.array_to_string(%s, E'\\n') AS \"%s\"",
+ "CASE pg_catalog.cardinality(%s)\n"
+ " WHEN 0 THEN '%s'\n"
+ " ELSE pg_catalog.array_to_string(%s, E'\\n')\n"
+ "END AS \"%s\"",
+ colname, gettext_noop("(none)"),
colname, gettext_noop("Access privileges"));
}
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index c70205b98a..08f854d0a8 100644
--- 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
+-- Create an owner for tested objects because output contains owner info.
+-- Must be superuser to be owner of tablespace.
+CREATE ROLE regress_zeropriv_owner SUPERUSER;
+SET LOCAL ROLE regress_zeropriv_owner;
+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)
+
+CREATE DOMAIN regress_zeropriv_domain AS int;
+REVOKE ALL ON DOMAIN regress_zeropriv_domain FROM CURRENT_USER, PUBLIC;
+\dD+ regress_zeropriv_domain
+ List of domains
+ Schema | Name | Type | Collation | Nullable | Default | Check | Access privileges | Description
+--------+-------------------------+---------+-----------+----------+---------+-------+-------------------+-------------
+ public | regress_zeropriv_domain | integer | | | | | (none) |
+(1 row)
+
+CREATE PROCEDURE regress_zeropriv_proc() LANGUAGE sql AS '';
+REVOKE ALL ON PROCEDURE regress_zeropriv_proc() FROM CURRENT_USER, PUBLIC;
+\df+ regress_zeropriv_proc
+ List of functions
+ Schema | Name | Result data type | Argument data types | Type | Volatility | Parallel | Owner | Security | Access privileges | Language | Internal name | Description
+--------+-----------------------+------------------+---------------------+------+------------+----------+------------------------+----------+-------------------+----------+---------------+-------------
+ public | regress_zeropriv_proc | | | proc | volatile | unsafe | regress_zeropriv_owner | invoker | (none) | sql | |
+(1 row)
+
+ALTER LANGUAGE plpgsql OWNER TO CURRENT_USER;
+REVOKE ALL ON LANGUAGE plpgsql FROM CURRENT_USER, PUBLIC;
+\dL+ plpgsql
+ List of languages
+ Name | Owner | Trusted | Internal language | Call handler | Validator | Inline handler | Access privileges | Description
+---------+------------------------+---------+-------------------+------------------------+------------------------+----------------------------------+-------------------+------------------------------
+ plpgsql | regress_zeropriv_owner | t | f | plpgsql_call_handler() | plpgsql_validator(oid) | plpgsql_inline_handler(internal) | (none) | PL/pgSQL procedural language
+(1 row)
+
+SELECT lo_create(3001);
+ lo_create
+-----------
+ 3001
+(1 row)
+
+REVOKE ALL ON LARGE OBJECT 3001 FROM CURRENT_USER;
+\dl+
+ Large objects
+ ID | Owner | Access privileges | Description
+------+------------------------+-------------------+-------------
+ 3001 | regress_zeropriv_owner | (none) |
+(1 row)
+
+CREATE SCHEMA regress_zeropriv_schema;
+REVOKE ALL ON SCHEMA regress_zeropriv_schema FROM CURRENT_USER;
+\dn+ regress_zeropriv_schema
+ List of schemas
+ Name | Owner | Access privileges | Description
+-------------------------+------------------------+-------------------+-------------
+ regress_zeropriv_schema | regress_zeropriv_owner | (none) |
+(1 row)
+
+CREATE TABLE regress_zeropriv_tbl (a int);
+REVOKE ALL ON TABLE regress_zeropriv_tbl FROM CURRENT_USER;
+\dp regress_zeropriv_tbl
+ Access privileges
+ Schema | Name | Type | Access privileges | Column privileges | Policies
+--------+----------------------+-------+-------------------+-------------------+----------
+ public | regress_zeropriv_tbl | table | (none) | |
+(1 row)
+
+CREATE TYPE regress_zeropriv_type AS (a int);
+REVOKE ALL ON TYPE regress_zeropriv_type FROM CURRENT_USER, PUBLIC;
+\dT+ regress_zeropriv_type
+ List of data types
+ Schema | Name | Internal name | Size | Elements | Owner | Access privileges | Description
+--------+-----------------------+-----------------------+-------+----------+------------------------+-------------------+-------------
+ public | regress_zeropriv_type | regress_zeropriv_type | tuple | | regress_zeropriv_owner | (none) |
+(1 row)
+
+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)
+
+ROLLBACK;
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 66ff64a160..a8fcb898ba 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1853,3 +1853,48 @@ DROP ROLE regress_du_role0;
DROP ROLE regress_du_role1;
DROP ROLE regress_du_role2;
DROP ROLE regress_du_admin;
+
+-- Test empty privileges.
+BEGIN;
+-- Create an owner for tested objects because output contains owner info.
+-- Must be superuser to be owner of tablespace.
+CREATE ROLE regress_zeropriv_owner SUPERUSER;
+SET LOCAL ROLE regress_zeropriv_owner;
+
+ALTER TABLESPACE regress_tblspace OWNER TO CURRENT_USER;
+REVOKE ALL ON TABLESPACE regress_tblspace FROM CURRENT_USER;
+\db+ regress_tblspace
+
+CREATE DOMAIN regress_zeropriv_domain AS int;
+REVOKE ALL ON DOMAIN regress_zeropriv_domain FROM CURRENT_USER, PUBLIC;
+\dD+ regress_zeropriv_domain
+
+CREATE PROCEDURE regress_zeropriv_proc() LANGUAGE sql AS '';
+REVOKE ALL ON PROCEDURE regress_zeropriv_proc() FROM CURRENT_USER, PUBLIC;
+\df+ regress_zeropriv_proc
+
+ALTER LANGUAGE plpgsql OWNER TO CURRENT_USER;
+REVOKE ALL ON LANGUAGE plpgsql FROM CURRENT_USER, PUBLIC;
+\dL+ plpgsql
+
+SELECT lo_create(3001);
+REVOKE ALL ON LARGE OBJECT 3001 FROM CURRENT_USER;
+\dl+
+
+CREATE SCHEMA regress_zeropriv_schema;
+REVOKE ALL ON SCHEMA regress_zeropriv_schema FROM CURRENT_USER;
+\dn+ regress_zeropriv_schema
+
+CREATE TABLE regress_zeropriv_tbl (a int);
+REVOKE ALL ON TABLE regress_zeropriv_tbl FROM CURRENT_USER;
+\dp regress_zeropriv_tbl
+
+CREATE TYPE regress_zeropriv_type AS (a int);
+REVOKE ALL ON TYPE regress_zeropriv_type FROM CURRENT_USER, PUBLIC;
+\dT+ regress_zeropriv_type
+
+ALTER DATABASE :"DBNAME" OWNER TO CURRENT_USER;
+REVOKE ALL ON DATABASE :"DBNAME" FROM CURRENT_USER, PUBLIC;
+\l :"DBNAME"
+
+ROLLBACK;
--
2.42.0
On 2023-10-20 22:35 +0200, David G. Johnston wrote:
Ok, I found my mis-understanding and better understand where you are all
coming from now; I apparently had the usage of NULL flip-flopped.Taking pg_tablespace as an example. Its "spcacl" column produces NULL for
default privileges and '{}'::text[] for empty privileges.Thus, today:
empty: array_to_string('{}'::text[], '\n') produces an empty string
default: array_to_string(null, '\n') produces null which then was printed
as a hard-coded empty string via forcibly changing \pset nullI was thinking the two cases were reversed.
My proposal for changing printACLColumn is thus:
case when spcacl is null then ''
when cardinality(spcacl) = 0 then '(none)'
else array_to_string(spcacl, E'\\n')
end as "Access privileges"In short, I don't want default privileges to start to obey \pset null when
it never has before and is documented as displaying the empty string. I do
want the empty string produced by empty privileges to change to (none) so
that it no longer is indistinguishable from our choice of presentation for
the default privilege case.Mechanically, we remove the existing \pset null for these metacommands and
move it into the query via the added CASE expression in the printACLColumn
function.This gets rid of NULL as an output value for this column and makes the
patch regarding \pset null discussion unnecessary. And it leaves the
existing well-established empty string for default privileges alone (and
changing this is what I believe Tom is against and I agree on that point).
I haven't thought off this yet. 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).
--
Erik
Attachments:
v3-0001-Fix-output-of-empty-privileges-in-psql.patchtext/plain; charset=us-asciiDownload
From 5e3f1840a5a6f49ccfa7f61c040b24638daad421 Mon Sep 17 00:00:00 2001
From: Erik Wienhold <ewie@ewie.name>
Date: Sun, 17 Sep 2023 20:54:48 +0200
Subject: [PATCH v3] Fix output of empty privileges in psql
Print "(none)" for empty privileges instead of nothing so that the user
is able to distinguish them from default privileges. \pset null was
ignored to always print default privileges as empty strings. This
kludge is now removed by explicitly returning the empty string for
default privileges with the nice side effect that all describe commands
now honor \pset null.
Meta commands affected by empty privileges:
\db+ \dD+ \des+ \dew+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l
Default privileges start as NULL::aclitem[] in various catalog columns
but revoking the default privileges leaves an empty aclitem[]. Using
\pset null '(default)' as a workaround for spotting default privileges
did not work because the meta commands ignored this setting.
The privileges shown by \dconfig+ and \ddp as well as the column
privileges shown by \dp are not affected by this change because the
respective aclitem[] is reset to NULL or deleted from the catalog
instead of leaving empty arrays.
Commands \des+ and \dew+ are not covered in src/test/regress because no
foreign data wrapper is available at this point to create a foreign
server.
Handling of empty privileges by Erik Wienhold. Fixing \pset null by
Laurenz Albe.
---
doc/src/sgml/ddl.sgml | 4 +-
src/bin/psql/describe.c | 51 +++-------------
src/test/regress/expected/psql.out | 94 ++++++++++++++++++++++++++++++
src/test/regress/sql/psql.sql | 45 ++++++++++++++
4 files changed, 149 insertions(+), 45 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 075ff32991..1c17c4a967 100644
--- 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>
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..0d7f86d80f 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -124,7 +124,6 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of aggregate functions");
myopt.translate_header = true;
@@ -197,7 +196,6 @@ describeAccessMethods(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of access methods");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -262,7 +260,6 @@ describeTablespaces(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of tablespaces");
myopt.translate_header = true;
@@ -585,7 +582,6 @@ describeFunctions(const char *functypes, const char *func_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of functions");
myopt.translate_header = true;
if (pset.sversion >= 90600)
@@ -702,7 +698,6 @@ describeTypes(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of data types");
myopt.translate_header = true;
@@ -893,7 +888,6 @@ describeOperators(const char *oper_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators");
myopt.translate_header = true;
@@ -995,7 +989,6 @@ listAllDbs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of databases");
myopt.translate_header = true;
@@ -1146,7 +1139,6 @@ permissionsList(const char *pattern, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1218,7 +1210,6 @@ listDefaultACLs(const char *pattern)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Default access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1417,7 +1408,6 @@ objectDescription(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Object descriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -3852,7 +3842,6 @@ listDbRoleSettings(const char *pattern, const char *pattern2)
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of settings");
myopt.translate_header = true;
@@ -3926,7 +3915,6 @@ describeRoleGrants(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of role grants");
myopt.translate_header = true;
@@ -4122,7 +4110,6 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of relations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4332,7 +4319,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
initPQExpBuffer(&title);
appendPQExpBufferStr(&title, tabletitle);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4412,7 +4398,6 @@ listLanguages(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of languages");
myopt.translate_header = true;
@@ -4497,7 +4482,6 @@ listDomains(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of domains");
myopt.translate_header = true;
@@ -4576,7 +4560,6 @@ listConversions(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of conversions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4644,7 +4627,6 @@ describeConfigurationParameters(const char *pattern, bool verbose,
if (!res)
return false;
- myopt.nullPrint = NULL;
if (pattern)
myopt.title = _("List of configuration parameters");
else
@@ -4726,7 +4708,6 @@ listEventTriggers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of event triggers");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4825,7 +4806,6 @@ listExtendedStats(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of extended statistics");
myopt.translate_header = true;
@@ -4938,7 +4918,6 @@ listCasts(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of casts");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5057,7 +5036,6 @@ listCollations(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of collations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5119,7 +5097,6 @@ listSchemas(const char *pattern, bool verbose, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
myopt.title = _("List of schemas");
myopt.translate_header = true;
@@ -5236,7 +5213,6 @@ listTSParsers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search parsers");
myopt.translate_header = true;
@@ -5384,7 +5360,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
if (nspname)
printfPQExpBuffer(&title, _("Text search parser \"%s.%s\""),
@@ -5421,7 +5396,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
return false;
}
- myopt.nullPrint = NULL;
if (nspname)
printfPQExpBuffer(&title, _("Token types for parser \"%s.%s\""),
nspname, prsname);
@@ -5497,7 +5471,6 @@ listTSDictionaries(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search dictionaries");
myopt.translate_header = true;
@@ -5563,7 +5536,6 @@ listTSTemplates(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search templates");
myopt.translate_header = true;
@@ -5618,7 +5590,6 @@ listTSConfigs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search configurations");
myopt.translate_header = true;
@@ -5764,7 +5735,6 @@ describeOneTSConfig(const char *oid, const char *nspname, const char *cfgname,
appendPQExpBuffer(&title, _("\nParser: \"%s\""),
prsname);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.footers = NULL;
myopt.topt.default_footer = false;
@@ -5841,7 +5811,6 @@ listForeignDataWrappers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign-data wrappers");
myopt.translate_header = true;
@@ -5918,7 +5887,6 @@ listForeignServers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign servers");
myopt.translate_header = true;
@@ -5974,7 +5942,6 @@ listUserMappings(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of user mappings");
myopt.translate_header = true;
@@ -6047,7 +6014,6 @@ listForeignTables(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign tables");
myopt.translate_header = true;
@@ -6099,7 +6065,6 @@ listExtensions(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of installed extensions");
myopt.translate_header = true;
@@ -6203,7 +6168,6 @@ listOneExtensionContents(const char *extname, const char *oid)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
printfPQExpBuffer(&title, _("Objects in extension \"%s\""), extname);
myopt.title = title.data;
@@ -6340,7 +6304,6 @@ listPublications(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of publications");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6695,7 +6658,6 @@ describeSubscriptions(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of subscriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -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"));
}
@@ -6808,7 +6776,6 @@ listOperatorClasses(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator classes");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6897,7 +6864,6 @@ listOperatorFamilies(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6996,7 +6962,6 @@ listOpFamilyOperators(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7089,7 +7054,6 @@ listOpFamilyFunctions(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of support functions of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7141,7 +7105,6 @@ listLargeObjects(bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Large objects");
myopt.translate_header = true;
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index c70205b98a..08f854d0a8 100644
--- 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
+-- Create an owner for tested objects because output contains owner info.
+-- Must be superuser to be owner of tablespace.
+CREATE ROLE regress_zeropriv_owner SUPERUSER;
+SET LOCAL ROLE regress_zeropriv_owner;
+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)
+
+CREATE DOMAIN regress_zeropriv_domain AS int;
+REVOKE ALL ON DOMAIN regress_zeropriv_domain FROM CURRENT_USER, PUBLIC;
+\dD+ regress_zeropriv_domain
+ List of domains
+ Schema | Name | Type | Collation | Nullable | Default | Check | Access privileges | Description
+--------+-------------------------+---------+-----------+----------+---------+-------+-------------------+-------------
+ public | regress_zeropriv_domain | integer | | | | | (none) |
+(1 row)
+
+CREATE PROCEDURE regress_zeropriv_proc() LANGUAGE sql AS '';
+REVOKE ALL ON PROCEDURE regress_zeropriv_proc() FROM CURRENT_USER, PUBLIC;
+\df+ regress_zeropriv_proc
+ List of functions
+ Schema | Name | Result data type | Argument data types | Type | Volatility | Parallel | Owner | Security | Access privileges | Language | Internal name | Description
+--------+-----------------------+------------------+---------------------+------+------------+----------+------------------------+----------+-------------------+----------+---------------+-------------
+ public | regress_zeropriv_proc | | | proc | volatile | unsafe | regress_zeropriv_owner | invoker | (none) | sql | |
+(1 row)
+
+ALTER LANGUAGE plpgsql OWNER TO CURRENT_USER;
+REVOKE ALL ON LANGUAGE plpgsql FROM CURRENT_USER, PUBLIC;
+\dL+ plpgsql
+ List of languages
+ Name | Owner | Trusted | Internal language | Call handler | Validator | Inline handler | Access privileges | Description
+---------+------------------------+---------+-------------------+------------------------+------------------------+----------------------------------+-------------------+------------------------------
+ plpgsql | regress_zeropriv_owner | t | f | plpgsql_call_handler() | plpgsql_validator(oid) | plpgsql_inline_handler(internal) | (none) | PL/pgSQL procedural language
+(1 row)
+
+SELECT lo_create(3001);
+ lo_create
+-----------
+ 3001
+(1 row)
+
+REVOKE ALL ON LARGE OBJECT 3001 FROM CURRENT_USER;
+\dl+
+ Large objects
+ ID | Owner | Access privileges | Description
+------+------------------------+-------------------+-------------
+ 3001 | regress_zeropriv_owner | (none) |
+(1 row)
+
+CREATE SCHEMA regress_zeropriv_schema;
+REVOKE ALL ON SCHEMA regress_zeropriv_schema FROM CURRENT_USER;
+\dn+ regress_zeropriv_schema
+ List of schemas
+ Name | Owner | Access privileges | Description
+-------------------------+------------------------+-------------------+-------------
+ regress_zeropriv_schema | regress_zeropriv_owner | (none) |
+(1 row)
+
+CREATE TABLE regress_zeropriv_tbl (a int);
+REVOKE ALL ON TABLE regress_zeropriv_tbl FROM CURRENT_USER;
+\dp regress_zeropriv_tbl
+ Access privileges
+ Schema | Name | Type | Access privileges | Column privileges | Policies
+--------+----------------------+-------+-------------------+-------------------+----------
+ public | regress_zeropriv_tbl | table | (none) | |
+(1 row)
+
+CREATE TYPE regress_zeropriv_type AS (a int);
+REVOKE ALL ON TYPE regress_zeropriv_type FROM CURRENT_USER, PUBLIC;
+\dT+ regress_zeropriv_type
+ List of data types
+ Schema | Name | Internal name | Size | Elements | Owner | Access privileges | Description
+--------+-----------------------+-----------------------+-------+----------+------------------------+-------------------+-------------
+ public | regress_zeropriv_type | regress_zeropriv_type | tuple | | regress_zeropriv_owner | (none) |
+(1 row)
+
+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)
+
+ROLLBACK;
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 66ff64a160..a8fcb898ba 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1853,3 +1853,48 @@ DROP ROLE regress_du_role0;
DROP ROLE regress_du_role1;
DROP ROLE regress_du_role2;
DROP ROLE regress_du_admin;
+
+-- Test empty privileges.
+BEGIN;
+-- Create an owner for tested objects because output contains owner info.
+-- Must be superuser to be owner of tablespace.
+CREATE ROLE regress_zeropriv_owner SUPERUSER;
+SET LOCAL ROLE regress_zeropriv_owner;
+
+ALTER TABLESPACE regress_tblspace OWNER TO CURRENT_USER;
+REVOKE ALL ON TABLESPACE regress_tblspace FROM CURRENT_USER;
+\db+ regress_tblspace
+
+CREATE DOMAIN regress_zeropriv_domain AS int;
+REVOKE ALL ON DOMAIN regress_zeropriv_domain FROM CURRENT_USER, PUBLIC;
+\dD+ regress_zeropriv_domain
+
+CREATE PROCEDURE regress_zeropriv_proc() LANGUAGE sql AS '';
+REVOKE ALL ON PROCEDURE regress_zeropriv_proc() FROM CURRENT_USER, PUBLIC;
+\df+ regress_zeropriv_proc
+
+ALTER LANGUAGE plpgsql OWNER TO CURRENT_USER;
+REVOKE ALL ON LANGUAGE plpgsql FROM CURRENT_USER, PUBLIC;
+\dL+ plpgsql
+
+SELECT lo_create(3001);
+REVOKE ALL ON LARGE OBJECT 3001 FROM CURRENT_USER;
+\dl+
+
+CREATE SCHEMA regress_zeropriv_schema;
+REVOKE ALL ON SCHEMA regress_zeropriv_schema FROM CURRENT_USER;
+\dn+ regress_zeropriv_schema
+
+CREATE TABLE regress_zeropriv_tbl (a int);
+REVOKE ALL ON TABLE regress_zeropriv_tbl FROM CURRENT_USER;
+\dp regress_zeropriv_tbl
+
+CREATE TYPE regress_zeropriv_type AS (a int);
+REVOKE ALL ON TYPE regress_zeropriv_type FROM CURRENT_USER, PUBLIC;
+\dT+ regress_zeropriv_type
+
+ALTER DATABASE :"DBNAME" OWNER TO CURRENT_USER;
+REVOKE ALL ON DATABASE :"DBNAME" FROM CURRENT_USER, PUBLIC;
+\l :"DBNAME"
+
+ROLLBACK;
--
2.42.0
On Fri, Oct 20, 2023 at 7:29 PM Erik Wienhold <ewie@ewie.name> wrote:
On 2023-10-20 22:35 +0200, David G. Johnston wrote:
In short, I don't want default privileges to start to obey \pset null
when
it never has before and is documented as displaying the empty string. I
do
want the empty string produced by empty privileges to change to (none) so
that it no longer is indistinguishable from our choice of presentationfor
the default privilege case.
I haven't thought off this yet. 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).
Thank you.
It looks good to me as-is, with one possible nit.
I wonder if it would be clearer to say:
"If the Access privileges column is *blank* for a given object..."
instead of "empty" to avoid having both "empty [string]" and "empty
privileges" present in the same paragraph and the empty string not
pertaining to the empty privileges.
David J.
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 progressThis 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
Attachments:
v4-0001-Fix-default-and-empty-privilege-output-in-psql.patchtext/x-patch; charset=UTF-8; name=v4-0001-Fix-default-and-empty-privilege-output-in-psql.patchDownload
From df5137f537cc0bef68c126a1e20bda831689b8aa Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Mon, 23 Oct 2023 11:24:01 +0200
Subject: [PATCH] Fix default and empty privilege output in psql
Default privileges start as NULL::aclitem[] in various catalog columns,
but revoking all privileges leaves an empty aclitem[]. These two
cases used to produce the same output with psql meta-commands like \dp.
Using "\pset null '(default)'" as a workaround for spotting default
privileges did not work, because null values were always displayed
as empty strings by psql meta-commands.
This patch improves that with two changes:
1. Print "(none)" for empty privileges so that the user is able to
distinguish them from default privileges.
Meta-commands affected by this change are:
\db+ \dD+ \des+ \dew+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l \z
2. Remove the special handling of null values by psql meta-commands,
so that "\pset null" is honored like everywhere else.
The privileges shown by \dconfig+ and \ddp as well as the column
privileges shown by \dp are not affected by this change, because the
respective aclitem[] is reset to NULL or deleted from the catalog
instead of leaving empty arrays.
Add a description of empty privileges to the documentation.
In passing, add an index entry entry for "privileges, default".
Author: Erik Wienhold, Laurenz Albe
Discussion: https://postgr.es/m/96d6885a-5e25-9ae8-4a1a-d7e557a5fe9c%40mtneva.com
---
doc/src/sgml/ddl.sgml | 13 ++++-
src/bin/psql/describe.c | 49 ++---------------
src/test/regress/expected/psql.out | 88 ++++++++++++++++++++++++++++++
src/test/regress/sql/psql.sql | 45 +++++++++++++++
4 files changed, 150 insertions(+), 45 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 075ff32991..883d079a8c 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -1737,6 +1737,11 @@ ALTER TABLE products RENAME TO items;
<primary>ACL</primary>
</indexterm>
+ <indexterm zone="ddl-priv-default">
+ <primary>privilege</primary>
+ <secondary>default</secondary>
+ </indexterm>
+
<para>
When an object is created, it is assigned an owner. The
owner is normally the role that executed the creation statement.
@@ -2049,7 +2054,7 @@ REVOKE ALL ON accounts FROM PUBLIC;
reference page of the respective command.
</para>
- <para>
+ <para id="ddl-priv-default">
PostgreSQL grants privileges on some types of objects to
<literal>PUBLIC</literal> by default when the objects are created.
No privileges are granted to <literal>PUBLIC</literal> by default on
@@ -2370,6 +2375,12 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO miriam_rw;
the <command>ALTER</command>.)
</para>
+ <para>
+ The <quote>Access privileges</quote> column shows <literal>(none)</literal>
+ for empty privileges (that is, no privileges at all, even for the object
+ owner — a rare occurrence).
+ </para>
+
<para>
Notice that the owner's implicit grant options are not marked in the
access privileges display. A <literal>*</literal> will appear only when
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..a9491023de 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -124,7 +124,6 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of aggregate functions");
myopt.translate_header = true;
@@ -197,7 +196,6 @@ describeAccessMethods(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of access methods");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -262,7 +260,6 @@ describeTablespaces(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of tablespaces");
myopt.translate_header = true;
@@ -585,7 +582,6 @@ describeFunctions(const char *functypes, const char *func_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of functions");
myopt.translate_header = true;
if (pset.sversion >= 90600)
@@ -702,7 +698,6 @@ describeTypes(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of data types");
myopt.translate_header = true;
@@ -893,7 +888,6 @@ describeOperators(const char *oper_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators");
myopt.translate_header = true;
@@ -995,7 +989,6 @@ listAllDbs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of databases");
myopt.translate_header = true;
@@ -1146,7 +1139,6 @@ permissionsList(const char *pattern, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1218,7 +1210,6 @@ listDefaultACLs(const char *pattern)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
printfPQExpBuffer(&buf, _("Default access privileges"));
myopt.title = buf.data;
myopt.translate_header = true;
@@ -1417,7 +1408,6 @@ objectDescription(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Object descriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -3852,7 +3842,6 @@ listDbRoleSettings(const char *pattern, const char *pattern2)
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of settings");
myopt.translate_header = true;
@@ -3926,7 +3915,6 @@ describeRoleGrants(const char *pattern, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of role grants");
myopt.translate_header = true;
@@ -4122,7 +4110,6 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
}
else
{
- myopt.nullPrint = NULL;
myopt.title = _("List of relations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4332,7 +4319,6 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
initPQExpBuffer(&title);
appendPQExpBufferStr(&title, tabletitle);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4412,7 +4398,6 @@ listLanguages(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of languages");
myopt.translate_header = true;
@@ -4497,7 +4482,6 @@ listDomains(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of domains");
myopt.translate_header = true;
@@ -4576,7 +4560,6 @@ listConversions(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of conversions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4644,7 +4627,6 @@ describeConfigurationParameters(const char *pattern, bool verbose,
if (!res)
return false;
- myopt.nullPrint = NULL;
if (pattern)
myopt.title = _("List of configuration parameters");
else
@@ -4726,7 +4708,6 @@ listEventTriggers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of event triggers");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -4825,7 +4806,6 @@ listExtendedStats(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of extended statistics");
myopt.translate_header = true;
@@ -4938,7 +4918,6 @@ listCasts(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of casts");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5057,7 +5036,6 @@ listCollations(const char *pattern, bool verbose, bool showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of collations");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -5119,7 +5097,6 @@ listSchemas(const char *pattern, bool verbose, bool showSystem)
if (!res)
goto error_return;
- myopt.nullPrint = NULL;
myopt.title = _("List of schemas");
myopt.translate_header = true;
@@ -5236,7 +5213,6 @@ listTSParsers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search parsers");
myopt.translate_header = true;
@@ -5384,7 +5360,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
if (nspname)
printfPQExpBuffer(&title, _("Text search parser \"%s.%s\""),
@@ -5421,7 +5396,6 @@ describeOneTSParser(const char *oid, const char *nspname, const char *prsname)
return false;
}
- myopt.nullPrint = NULL;
if (nspname)
printfPQExpBuffer(&title, _("Token types for parser \"%s.%s\""),
nspname, prsname);
@@ -5497,7 +5471,6 @@ listTSDictionaries(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search dictionaries");
myopt.translate_header = true;
@@ -5563,7 +5536,6 @@ listTSTemplates(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search templates");
myopt.translate_header = true;
@@ -5618,7 +5590,6 @@ listTSConfigs(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of text search configurations");
myopt.translate_header = true;
@@ -5764,7 +5735,6 @@ describeOneTSConfig(const char *oid, const char *nspname, const char *cfgname,
appendPQExpBuffer(&title, _("\nParser: \"%s\""),
prsname);
- myopt.nullPrint = NULL;
myopt.title = title.data;
myopt.footers = NULL;
myopt.topt.default_footer = false;
@@ -5841,7 +5811,6 @@ listForeignDataWrappers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign-data wrappers");
myopt.translate_header = true;
@@ -5918,7 +5887,6 @@ listForeignServers(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign servers");
myopt.translate_header = true;
@@ -5974,7 +5942,6 @@ listUserMappings(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of user mappings");
myopt.translate_header = true;
@@ -6047,7 +6014,6 @@ listForeignTables(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of foreign tables");
myopt.translate_header = true;
@@ -6099,7 +6065,6 @@ listExtensions(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of installed extensions");
myopt.translate_header = true;
@@ -6203,7 +6168,6 @@ listOneExtensionContents(const char *extname, const char *oid)
if (!res)
return false;
- myopt.nullPrint = NULL;
initPQExpBuffer(&title);
printfPQExpBuffer(&title, _("Objects in extension \"%s\""), extname);
myopt.title = title.data;
@@ -6340,7 +6304,6 @@ listPublications(const char *pattern)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of publications");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6695,7 +6658,6 @@ describeSubscriptions(const char *pattern, bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of subscriptions");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6718,7 +6680,11 @@ static void
printACLColumn(PQExpBuffer buf, const char *colname)
{
appendPQExpBuffer(buf,
- "pg_catalog.array_to_string(%s, E'\\n') AS \"%s\"",
+ "CASE\n"
+ " WHEN pg_catalog.cardinality(%s) = 0 THEN '%s'\n"
+ " ELSE pg_catalog.array_to_string(%s, E'\\n')\n"
+ "END AS \"%s\"",
+ colname, gettext_noop("(none)"),
colname, gettext_noop("Access privileges"));
}
@@ -6808,7 +6774,6 @@ listOperatorClasses(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator classes");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6897,7 +6862,6 @@ listOperatorFamilies(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -6996,7 +6960,6 @@ listOpFamilyOperators(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of operators of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7089,7 +7052,6 @@ listOpFamilyFunctions(const char *access_method_pattern,
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of support functions of operator families");
myopt.translate_header = true;
myopt.translate_columns = translate_columns;
@@ -7141,7 +7103,6 @@ listLargeObjects(bool verbose)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("Large objects");
myopt.translate_header = true;
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index c70205b98a..dfbd74ce3a 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -5812,6 +5812,7 @@ SELECT * FROM bla ORDER BY 1;
Susie
(5 rows)
+COMMIT;
-- reset all
\set AUTOCOMMIT on
\set ON_ERROR_ROLLBACK off
@@ -6663,3 +6664,90 @@ DROP ROLE regress_du_role0;
DROP ROLE regress_du_role1;
DROP ROLE regress_du_role2;
DROP ROLE regress_du_admin;
+-- Test empty privileges.
+BEGIN;
+-- Create an owner for tested objects because output contains owner info.
+-- Must be superuser to be owner of tablespace.
+CREATE ROLE regress_zeropriv_owner SUPERUSER;
+SET LOCAL ROLE regress_zeropriv_owner;
+CREATE DOMAIN regress_zeropriv_domain AS int;
+REVOKE ALL ON DOMAIN regress_zeropriv_domain FROM CURRENT_USER, PUBLIC;
+\dD+ regress_zeropriv_domain
+ List of domains
+ Schema | Name | Type | Collation | Nullable | Default | Check | Access privileges | Description
+--------+-------------------------+---------+-----------+----------+---------+-------+-------------------+-------------
+ public | regress_zeropriv_domain | integer | | | | | (none) |
+(1 row)
+
+CREATE PROCEDURE regress_zeropriv_proc() LANGUAGE sql AS '';
+REVOKE ALL ON PROCEDURE regress_zeropriv_proc() FROM CURRENT_USER, PUBLIC;
+\df+ regress_zeropriv_proc
+ List of functions
+ Schema | Name | Result data type | Argument data types | Type | Volatility | Parallel | Owner | Security | Access privileges | Language | Internal name | Description
+--------+-----------------------+------------------+---------------------+------+------------+----------+------------------------+----------+-------------------+----------+---------------+-------------
+ public | regress_zeropriv_proc | | | proc | volatile | unsafe | regress_zeropriv_owner | invoker | (none) | sql | |
+(1 row)
+
+ALTER LANGUAGE plpgsql OWNER TO CURRENT_USER;
+REVOKE ALL ON LANGUAGE plpgsql FROM CURRENT_USER, PUBLIC;
+\dL+ plpgsql
+ List of languages
+ Name | Owner | Trusted | Internal language | Call handler | Validator | Inline handler | Access privileges | Description
+---------+------------------------+---------+-------------------+------------------------+------------------------+----------------------------------+-------------------+------------------------------
+ plpgsql | regress_zeropriv_owner | t | f | plpgsql_call_handler() | plpgsql_validator(oid) | plpgsql_inline_handler(internal) | (none) | PL/pgSQL procedural language
+(1 row)
+
+SELECT lo_create(3001);
+ lo_create
+-----------
+ 3001
+(1 row)
+
+REVOKE ALL ON LARGE OBJECT 3001 FROM CURRENT_USER;
+\dl+
+ Large objects
+ ID | Owner | Access privileges | Description
+------+------------------------+-------------------+-------------
+ 3001 | regress_zeropriv_owner | (none) |
+(1 row)
+
+CREATE SCHEMA regress_zeropriv_schema;
+REVOKE ALL ON SCHEMA regress_zeropriv_schema FROM CURRENT_USER;
+\dn+ regress_zeropriv_schema
+ List of schemas
+ Name | Owner | Access privileges | Description
+-------------------------+------------------------+-------------------+-------------
+ regress_zeropriv_schema | regress_zeropriv_owner | (none) |
+(1 row)
+
+CREATE TABLE regress_zeropriv_tbl (a int);
+REVOKE ALL ON TABLE regress_zeropriv_tbl FROM CURRENT_USER;
+\dp regress_zeropriv_tbl
+ Access privileges
+ Schema | Name | Type | Access privileges | Column privileges | Policies
+--------+----------------------+-------+-------------------+-------------------+----------
+ public | regress_zeropriv_tbl | table | (none) | |
+(1 row)
+
+CREATE TYPE regress_zeropriv_type AS (a int);
+REVOKE ALL ON TYPE regress_zeropriv_type FROM CURRENT_USER, PUBLIC;
+\dT+ regress_zeropriv_type
+ List of data types
+ Schema | Name | Internal name | Size | Elements | Owner | Access privileges | Description
+--------+-----------------------+-----------------------+-------+----------+------------------------+-------------------+-------------
+ public | regress_zeropriv_type | regress_zeropriv_type | tuple | | regress_zeropriv_owner | (none) |
+(1 row)
+
+ROLLBACK;
+-- test display of default privileges with \pset null
+CREATE TABLE defprivs ();
+\pset null '(default)'
+\z defprivs
+ Access privileges
+ Schema | Name | Type | Access privileges | Column privileges | Policies
+--------+----------+-------+-------------------+-------------------+----------
+ public | defprivs | table | (default) | |
+(1 row)
+
+\pset null ''
+DROP TABLE defprivs;
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 66ff64a160..5b14181913 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1578,6 +1578,7 @@ COMMIT;
SELECT COUNT(*) AS "#mum"
FROM bla WHERE s = 'Mum' \; -- no mum here
SELECT * FROM bla ORDER BY 1;
+COMMIT;
-- reset all
\set AUTOCOMMIT on
@@ -1853,3 +1854,47 @@ DROP ROLE regress_du_role0;
DROP ROLE regress_du_role1;
DROP ROLE regress_du_role2;
DROP ROLE regress_du_admin;
+
+-- Test empty privileges.
+BEGIN;
+-- Create an owner for tested objects because output contains owner info.
+-- Must be superuser to be owner of tablespace.
+CREATE ROLE regress_zeropriv_owner SUPERUSER;
+SET LOCAL ROLE regress_zeropriv_owner;
+
+CREATE DOMAIN regress_zeropriv_domain AS int;
+REVOKE ALL ON DOMAIN regress_zeropriv_domain FROM CURRENT_USER, PUBLIC;
+\dD+ regress_zeropriv_domain
+
+CREATE PROCEDURE regress_zeropriv_proc() LANGUAGE sql AS '';
+REVOKE ALL ON PROCEDURE regress_zeropriv_proc() FROM CURRENT_USER, PUBLIC;
+\df+ regress_zeropriv_proc
+
+ALTER LANGUAGE plpgsql OWNER TO CURRENT_USER;
+REVOKE ALL ON LANGUAGE plpgsql FROM CURRENT_USER, PUBLIC;
+\dL+ plpgsql
+
+SELECT lo_create(3001);
+REVOKE ALL ON LARGE OBJECT 3001 FROM CURRENT_USER;
+\dl+
+
+CREATE SCHEMA regress_zeropriv_schema;
+REVOKE ALL ON SCHEMA regress_zeropriv_schema FROM CURRENT_USER;
+\dn+ regress_zeropriv_schema
+
+CREATE TABLE regress_zeropriv_tbl (a int);
+REVOKE ALL ON TABLE regress_zeropriv_tbl FROM CURRENT_USER;
+\dp regress_zeropriv_tbl
+
+CREATE TYPE regress_zeropriv_type AS (a int);
+REVOKE ALL ON TYPE regress_zeropriv_type FROM CURRENT_USER, PUBLIC;
+\dT+ regress_zeropriv_type
+
+ROLLBACK;
+
+-- test display of default privileges with \pset null
+CREATE TABLE defprivs ();
+\pset null '(default)'
+\z defprivs
+\pset null ''
+DROP TABLE defprivs;
--
2.41.0
On Monday, October 23, 2023, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
--- 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.
There is no error here, the current consensus which this patch implements
is to not change the documented “default privileges display as blank”. We
are solving the empty privileges are not distinguishable complaint by
printing (none) for them.
David J.
On Mon, 2023-10-23 at 07:03 -0700, David G. Johnston wrote:
On Monday, October 23, 2023, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
--- 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.There is no error here, the current consensus which this patch implements is to
not change the documented “default privileges display as blank”. We are solving
the empty privileges are not distinguishable complaint by printing (none) for them.
Erik's latest patch included my changes to display NULL as NULL in psql,
so that "\pset null" works as expected.
But he left the above hunk from his original patch, and that hunk replaces
NULL with an empty string, so "\pset null" wouldn't work for the display
of default provoleges.
He didn't notice it because he didn't have a regression test that displays
default privileges with non-empty "\pset null".
Yours,
Laurenz Albe
On Monday, October 23, 2023, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
On Mon, 2023-10-23 at 07:03 -0700, David G. Johnston wrote:
On Monday, October 23, 2023, Laurenz Albe <laurenz.albe@cybertec.at>
wrote:
--- 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.There is no error here, the current consensus which this patch
implements is to
not change the documented “default privileges display as blank”. We are
solving
the empty privileges are not distinguishable complaint by printing
(none) for them.
Erik's latest patch included my changes to display NULL as NULL in psql,
so that "\pset null" works as expected.
No, per the commit message, issuing an explicit \pset null is a kludge and
it gets rid of the hack in favor of making the query itself produce an
empty string. i.e., we choose a poor implementation to get the documented
behavior and we are cleaning that up as an aside to the main fix.
Changing the behavior so that default privileges print in correspondence to
the setting of \pset null is, IME, off the table for this patch. Its one
and only goal is to reliably distinguish empty and default privileges.
That is our extant bug.
We document default privileges print as an empty string - I do not think we
should change the definition to "default privileges print null which can be
controlled via \pset null", and regardless of preference doing so is not a
bug.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
We document default privileges print as an empty string - I do not think we
should change the definition to "default privileges print null which can be
controlled via \pset null", and regardless of preference doing so is not a
bug.
Well, "if it's documented it's not a bug" is a defensible argument
for calling something not a bug, but it doesn't address the question
of whether changing it would be an improvement. I think it would be,
and I object to your claim that we have a consensus to not do that.
The core of the problem here, IMO, is exactly that there is confusion
between whether a visibly empty string represents NULL (default
privileges) or an empty ACL (no privileges). I believe we have agreed
that printing "(none)" or some other clearly-not-an-ACL-entry string
for the second case is an improvement, even though that's a change
in behavior. That doesn't mean that changing the other case can't
also be an improvement. I think it'd be less confusing all around
if this instance of NULL prints like other instances of NULL.
IOW, the current definition is "NULL privileges print as an empty
string no matter what", and I don't think that serves to reduce
confusion about whether an ACL is NULL or not. We ought to be doing
what we can to make clear that such an ACL *is* NULL, because
otherwise people won't understand how it differs from an empty ACL.
regards, tom lane
On Mon, Oct 23, 2023 at 7:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
IOW, the current definition is "NULL privileges print as an empty
string no matter what", and I don't think that serves to reduce
confusion about whether an ACL is NULL or not. We ought to be doing
what we can to make clear that such an ACL *is* NULL, because
otherwise people won't understand how it differs from an empty ACL.
I tend to prefer the argument that these views are for human consumption
and should be designed with that in mind. Allowing the user to decide
whether they prefer NULL to print as the empty string or something else
works within that framework. And the change of behavior for everyone with
a non-default \pset gets accepted under that framework as well.
I would suggest that we make the expected presence of NULL as an input
explicit:
case when spcacl is null then null
when cardinality(spcacl) = 0 then '(none)' -- so as not to confuse
it with null being printed also as an empty string
else array_to_string(spcacl, E'\\n')
end as "Access privileges"
I would offer up:
when spcacl is null then '(default)'
along with not translating (none) and (default) and thus making the data
contents of these views environment independent. But minimizing the
variance of these command's output across systems doesn't seem like a
design goal that is likely to gain consensus and is excessive when viewed
within the framework of these being only for human consumption.
David J.
On Mon, 2023-10-23 at 08:35 -0700, David G. Johnston wrote:
I tend to prefer the argument that these views are for human consumption and should
be designed with that in mind.
True, although given the shape of ACLs, it takes a somewhat trained human to
consume the string representation. But we won't be able to hide the fact that
default ACLs are NULL in the catalogs. We can leave them empty, we can show
them as "(default)" or we can let the user choose with "\pset null".
I would suggest that we make the expected presence of NULL as an input explicit:
I would offer up:when spcacl is null then '(default)'
Noted.
along with not translating (none) and (default) and thus making the data contents
of these views environment independent. But minimizing the variance of these command's
output across systems doesn't seem like a design goal that is likely to gain consensus
and is excessive when viewed within the framework of these being only for human consumption.
I didn't understand this completely. You want default privileges displayed as
"(default)", but are you for or against "\pset null" to have its normal effect on
the output of backslash commands in all other cases?
Speaking of consensus, it seems to me that Tom, Erik and me are in consensus.
Yours,
Laurenz Albe
On Monday, October 23, 2023, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
On Mon, 2023-10-23 at 08:35 -0700, David G. Johnston wrote:
along with not translating (none) and (default) and thus making the data
contents
of these views environment independent. But minimizing the variance of
these command's
output across systems doesn't seem like a design goal that is likely to
gain consensus
and is excessive when viewed within the framework of these being only
for human consumption.
I didn't understand this completely. You want default privileges
displayed as
"(default)", but are you for or against "\pset null" to have its normal
effect on
the output of backslash commands in all other cases?
I haven’t inspected other cases but to my knowledge we don’t typically
represent non-unknown things using NULL so I’m not expecting other places
to have this representation problem.
I don’t think any of our meta-command outputs should modify pset null.
Left join cases should be considered unknown, represented as NULL, and obey
the user’s setting.
I do believe that we should be against exposing, like in this case, any
internal implementation detail that encodes something (e.g., default
privileges) as NULL in the catalogs, to the user of the psql meta-commands.
I won’t argue that exposing such NULLS is wrong, just it would preferable
IME to avoid doing so. NULL means unknown or not applicable and default
privileges are neither of those things. I get why our catalogs choose such
an encoding and agree with it, and users that find the need to consult the
catalogs will need to learn such details. But we should strive for them to
be able to survive with psql meta-commands.
David J.
On Mon, 2023-10-23 at 11:37 -0700, David G. Johnston wrote:
I didn't understand this completely. You want default privileges displayed as
"(default)", but are you for or against "\pset null" to have its normal effect on
the output of backslash commands in all other cases?I haven’t inspected other cases but to my knowledge we don’t typically represent
non-unknown things using NULL so I’m not expecting other places to have this
representation problem.
The first example that comes to my mind is the "ICU Locale" and the "ICU Rules"
in the output of \l. There are many others.
I don’t think any of our meta-command outputs should modify pset null.
Left join cases should be considered unknown, represented as NULL, and obey the
user’s setting.
That's what I think too. psql output should respect "\pset null".
So it looks like we agree on that.
I do believe that we should be against exposing, like in this case, any internal
implementation detail that encodes something (e.g., default privileges) as NULL
in the catalogs, to the user of the psql meta-commands.I won’t argue that exposing such NULLS is wrong, just it would preferable IME
to avoid doing so. NULL means unknown or not applicable and default privileges
are neither of those things. I get why our catalogs choose such an encoding and
agree with it, and users that find the need to consult the catalogs will need to
learn such details. But we should strive for them to be able to survive with
psql meta-commands.
Sure, it would be best to hide this implementation detail from the user.
The correct way to do that would be to fake an ACL entry like "laurenz=arwdDxt/laurenz"
if there is a NULL in the catalog, but that would add a ton of special-case
code to psql, which does not look appealing at all.
So we cannot completely hide the implementation, but perhaps "(default)" would
be less confusing than a NULL value.
If everybody agrees, I can modify the patch to do that.
Yours,
Laurenz Albe
Laurenz Albe <laurenz.albe@cybertec.at> writes:
On Mon, 2023-10-23 at 11:37 -0700, David G. Johnston wrote:
I do believe that we should be against exposing, like in this case, any internal
implementation detail that encodes something (e.g., default privileges) as NULL
in the catalogs, to the user of the psql meta-commands.
Sure, it would be best to hide this implementation detail from the user.
The correct way to do that would be to fake an ACL entry like "laurenz=arwdDxt/laurenz"
if there is a NULL in the catalog, but that would add a ton of special-case
code to psql, which does not look appealing at all.
For better or worse, that *is* the backend's catalog representation,
and I don't think that psql would be doing our users a service by
trying to obscure the fact. They'd run into it anyway the moment
they look at the catalogs with anything but a \d-something command.
regards, tom lane
On Monday, October 23, 2023, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
On Mon, 2023-10-23 at 11:37 -0700, David G. Johnston wrote:
I didn't understand this completely. You want default privileges
displayed as
"(default)", but are you for or against "\pset null" to have its
normal effect on
the output of backslash commands in all other cases?
I haven’t inspected other cases but to my knowledge we don’t typically
represent
non-unknown things using NULL so I’m not expecting other places to have
this
representation problem.
The first example that comes to my mind is the "ICU Locale" and the "ICU
Rules"
in the output of \l. There are many others.
Both of those fall into “this null means there is no value for these
(because we aren’t using icu)”. I have no qualms with leaving true nulls
represented as themselves. Clean slate maybe I print “(not using icu)”
there instead of null but it isn’t worth the effort to change.
I won’t argue that exposing such NULLS is wrong, just it would
preferable IME
to avoid doing so. NULL means unknown or not applicable and default
privileges
are neither of those things. I get why our catalogs choose such an
encoding and
agree with it, and users that find the need to consult the catalogs will
need to
learn such details. But we should strive for them to be able to survive
with
psql meta-commands.
Sure, it would be best to hide this implementation detail from the user.
The correct way to do that would be to fake an ACL entry like
"laurenz=arwdDxt/laurenz"
if there is a NULL in the catalog, but that would add a ton of special-case
code to psql, which does not look appealing at all.
More generically it would be “[PUBLIC=]/???/postgres” and
{OWNER}=???/postgres
It would ideally be a function call for psql and a system info function
usable for anyone.
David J.
On Monday, October 23, 2023, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Laurenz Albe <laurenz.albe@cybertec.at> writes:
On Mon, 2023-10-23 at 11:37 -0700, David G. Johnston wrote:
I do believe that we should be against exposing, like in this case, any
internal
implementation detail that encodes something (e.g., default privileges)
as NULL
in the catalogs, to the user of the psql meta-commands.
Sure, it would be best to hide this implementation detail from the user.
The correct way to do that would be to fake an ACL entry like"laurenz=arwdDxt/laurenz"
if there is a NULL in the catalog, but that would add a ton of
special-case
code to psql, which does not look appealing at all.
For better or worse, that *is* the backend's catalog representation,
and I don't think that psql would be doing our users a service by
trying to obscure the fact. They'd run into it anyway the moment
they look at the catalogs with anything but a \d-something command.
Which many may never do, and those few that do will see immediately that
the catalog uses null where they expected to see “(default)” and realize we
made a presentational choice in the interests of readability and their
query will need to make a choice regarding the null and empty arrays as
well.
David J.
On Mon, 2023-10-23 at 22:43 -0400, Tom Lane wrote:
Laurenz Albe <laurenz.albe@cybertec.at> writes:
On Mon, 2023-10-23 at 11:37 -0700, David G. Johnston wrote:
I do believe that we should be against exposing, like in this case, any internal
implementation detail that encodes something (e.g., default privileges) as NULL
in the catalogs, to the user of the psql meta-commands.Sure, it would be best to hide this implementation detail from the user.
The correct way to do that would be to fake an ACL entry like "laurenz=arwdDxt/laurenz"
if there is a NULL in the catalog, but that would add a ton of special-case
code to psql, which does not look appealing at all.For better or worse, that *is* the backend's catalog representation,
and I don't think that psql would be doing our users a service by
trying to obscure the fact. They'd run into it anyway the moment
they look at the catalogs with anything but a \d-something command.
... for example with a client like pgAdmin, which is a frequent choice
of many PostgreSQL beginners (they display empty privileges).
Yes, it is "(default)" or NULL. The former is friendlier for beginners,
the latter incurs less backward incompatibility.
I could live with either solution, but I am still leaning towards NULL.
I ran the regression tests with a patch that displays "(default)",
and I counted 22 failures, excluding the one added by my patch.
The tests can of course be fixed, but perhaps that serves as a measure
of the backward incompatibility.
Yours,
Laurenz Albe
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 progressThis 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.
On Wed, 2023-11-08 at 10:56 +0530, Shubham Khanna wrote:
I tested the Patch for the modified changes and it is working fine.
Thanks for the review!
I wonder how to proceed with this patch. The main disagreement is
whether default privileges should be displayed as NULL (less invasive,
but more confusing for beginners) or "(default)" (more invasive,
but nicer for beginners).
David is for "(default)", Tom and me are for NULL, and I guess Erik
would also prefer "(default)", since that was how his original
patch did it, IIRC. I think I could live with both solutions.
Kind of a stalemate. Who wants to tip the scales?
Yours,
Laurenz Albe
On 2023-11-08 13:23 +0100, Laurenz Albe wrote:
I wonder how to proceed with this patch. The main disagreement is
whether default privileges should be displayed as NULL (less invasive,
but more confusing for beginners) or "(default)" (more invasive,
but nicer for beginners).
Are there any reports from beginners being confused about default
privileges being NULL or being displayed as a blank string in psql?
This is usually resolved with a pointer to the docs if it comes up in
discussions or the user makes the mental leap and checks the docs
himself. Both patches add some details to the docs to explain psql's
output.
David is for "(default)", Tom and me are for NULL, and I guess Erik
would also prefer "(default)", since that was how his original
patch did it, IIRC. I think I could live with both solutions.Kind of a stalemate. Who wants to tip the scales?
Yes I had a slight preference for my patch but I'd go with yours (\pset
null) now. I followed the discussion after my last mail but had nothing
more to add that wasn't already said. Tom then wrote that NULL is the
catalog's representation for the default privileges and obscuring that
fact in psql is not doing any service to the users. This convinced me
because users may have to deal with aclitem[] being NULL anyway at some
point if they need to check privileges in more detail. So it makes
absolutely sense that psql is transparent about that.
--
Erik
On Thu, 2023-11-09 at 03:40 +0100, Erik Wienhold wrote:
On 2023-11-08 13:23 +0100, Laurenz Albe wrote:
I wonder how to proceed with this patch. The main disagreement is
whether default privileges should be displayed as NULL (less invasive,
but more confusing for beginners) or "(default)" (more invasive,
but nicer for beginners).Are there any reports from beginners being confused about default
privileges being NULL or being displayed as a blank string in psql?
This is usually resolved with a pointer to the docs if it comes up in
discussions or the user makes the mental leap and checks the docs
himself. Both patches add some details to the docs to explain psql's
output.
Right.
David is for "(default)", Tom and me are for NULL, and I guess Erik
would also prefer "(default)", since that was how his original
patch did it, IIRC. I think I could live with both solutions.Kind of a stalemate. Who wants to tip the scales?
Yes I had a slight preference for my patch but I'd go with yours (\pset
null) now. I followed the discussion after my last mail but had nothing
more to add that wasn't already said. Tom then wrote that NULL is the
catalog's representation for the default privileges and obscuring that
fact in psql is not doing any service to the users. This convinced me
because users may have to deal with aclitem[] being NULL anyway at some
point if they need to check privileges in more detail. So it makes
absolutely sense that psql is transparent about that.
Thanks for the feedback. I'll set the patch to "ready for committer" then.
Yours,
Laurenz Albe
Laurenz Albe <laurenz.albe@cybertec.at> writes:
Thanks for the feedback. I'll set the patch to "ready for committer" then.
So, just to clarify, we're settling on your v4 from [1]/messages/by-id/d799f996f422231a99655f1223667d6d887e4c95.camel@cybertec.at?
regards, tom lane
[1]: /messages/by-id/d799f996f422231a99655f1223667d6d887e4c95.camel@cybertec.at
On 2023-11-09 20:19 +0100, Tom Lane wrote:
Laurenz Albe <laurenz.albe@cybertec.at> writes:
Thanks for the feedback. I'll set the patch to "ready for committer" then.
So, just to clarify, we're settling on your v4 from [1]?
[1] /messages/by-id/d799f996f422231a99655f1223667d6d887e4c95.camel@cybertec.at
Yes from my side.
--
Erik
On Mon, 2023-11-13 at 11:27 +0100, Erik Wienhold wrote:
On 2023-11-09 20:19 +0100, Tom Lane wrote:
Laurenz Albe <laurenz.albe@cybertec.at> writes:
Thanks for the feedback. I'll set the patch to "ready for committer" then.
So, just to clarify, we're settling on your v4 from [1]?
[1] /messages/by-id/d799f996f422231a99655f1223667d6d887e4c95.camel@cybertec.at
Yes from my side.
+1
Yours,
Laurenz Albe
On Mon, Nov 13, 2023 at 12:36 PM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:
On Mon, 2023-11-13 at 11:27 +0100, Erik Wienhold wrote:
On 2023-11-09 20:19 +0100, Tom Lane wrote:
Laurenz Albe <laurenz.albe@cybertec.at> writes:
Thanks for the feedback. I'll set the patch to "ready for
committer" then.
So, just to clarify, we're settling on your v4 from [1]?
[1]
/messages/by-id/d799f996f422231a99655f1223667d6d887e4c95.camel@cybertec.at
Yes from my side.
+1
+0.5 for the reasons already stated; but I get and accept the argument for
NULL.
I will reiterate my preference for writing an explicit IS NULL branch in
the case expression instead of relying upon the strict-ness of
array_to_string.
+ "CASE\n"
WHEN %s IS NULL THEN NULL
+ " WHEN pg_catalog.cardinality(%s) = 0 THEN '%s'\n"
+ " ELSE pg_catalog.array_to_string(%s, E'\\n')\n"
+ "END AS \"%s\"",David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
On Mon, Nov 13, 2023 at 12:36 PM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:On Mon, 2023-11-13 at 11:27 +0100, Erik Wienhold wrote:
On 2023-11-09 20:19 +0100, Tom Lane wrote:
So, just to clarify, we're settling on your v4 from [1]?
Yes from my side.
+1
+0.5 for the reasons already stated; but I get and accept the argument for
NULL.
Patch pushed with minor adjustments, mainly rewriting some comments.
One notable change is that I dropped the newline whitespace printed
by printACLColumn. That was contrary to the policy expressed in the
function's comment, and IMO it made -E output look worse not better.
The problem is that the calling code determines the indentation
that this targetlist item should have, and we don't want to outdent
from that. I think it's better to make it one line, even though
that will run a bit over 80 columns.
I also got rid of the use of a created superuser in the test case.
The test seems pretty duplicative to me anyway, so let's just not
test the object types that need superuser.
I will reiterate my preference for writing an explicit IS NULL branch in
the case expression instead of relying upon the strict-ness of
array_to_string.
Meh. We were relying on that already, and it wasn't a problem.
I might have done it, except that it'd have made the one line
even longer and harder to read (and slower to execute, probably).
regards, tom lane
On Mon, 2023-11-13 at 15:49 -0500, Tom Lane wrote:
Patch pushed with minor adjustments, mainly rewriting some comments.
Thank you!
Yours,
Laurenz Albe