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] > > > > https://www.postgresql.org/message-id/efdd465d-a795-6188-7f71-7cdb4b2be031%40mtneva.com > > > > [2] > > > > https://www.postgresql.org/message-id/31246.1693337238%40sss.pgh.pa.us > > > > > > Reading 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 |
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
>From a3ea9549a4467fb4ab186d652e0afec6adc2f725 Mon Sep 17 00:00:00 2001 From: Laurenz Albe <laurenz.a...@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
