public inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH v24 08/11] pg_ls_*dir to return all the metadata from pg_stat_file..
5+ messages / 3 participants
[nested] [flat]

* [PATCH v24 08/11] pg_ls_*dir to return all the metadata from pg_stat_file..
@ 2020-03-10 02:56  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 5+ messages in thread

From: Justin Pryzby @ 2020-03-10 02:56 UTC (permalink / raw)

..but it doesn't seem worth factoring out the common bits, since stat_file
doesn't return a name, so all the field numbers are off by one.

NOTE, the atime is now shown where the mtime used to be.

Need catversion bump
---
 doc/src/sgml/func.sgml                       | 34 +++++++---
 src/backend/utils/adt/genfile.c              | 71 +++++++++-----------
 src/include/catalog/pg_proc.dat              | 28 ++++----
 src/test/regress/expected/misc_functions.out | 12 ++--
 src/test/regress/output/tablespace.source    |  8 +--
 5 files changed, 81 insertions(+), 72 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 191cf7c9fa..e3d7df2dd7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25743,13 +25743,16 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size
         <returnvalue>setof record</returnvalue>
         ( <parameter>name</parameter> <type>text</type>,
         <parameter>size</parameter> <type>bigint</type>,
+        <parameter>access</parameter> <type>timestamp with time zone</type>,
         <parameter>modification</parameter> <type>timestamp with time zone</type>,
+        <parameter>change</parameter> <type>timestamp with time zone</type>,
+        <parameter>creation</parameter> <type>timestamp with time zone</type>,
         <parameter>isdir</parameter> <type>boolean</type> )
        </para>
        <para>
         For each file in the server's log directory,
-        return the file's name, size, last modification time (mtime), and a boolean
-        indicating if the file is a directory.
+        return the file's name, along with the metadata columns returned by
+        <function>pg_stat_file</function>.
         Filenames beginning with a dot and special file types are excluded.
        </para>
        <para>
@@ -25770,7 +25773,10 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size
         <returnvalue>setof record</returnvalue>
         ( <parameter>filename</parameter> <type>text</type>,
         <parameter>size</parameter> <type>bigint</type>,
+        <parameter>access</parameter> <type>timestamp with time zone</type>,
         <parameter>modification</parameter> <type>timestamp with time zone</type>,
+        <parameter>change</parameter> <type>timestamp with time zone</type>,
+        <parameter>creation</parameter> <type>timestamp with time zone</type>,
         <parameter>isdir</parameter> <type>boolean</type> )
        </para>
        <para>
@@ -25790,13 +25796,16 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size
         <returnvalue>setof record</returnvalue>
         ( <parameter>name</parameter> <type>text</type>,
         <parameter>size</parameter> <type>bigint</type>,
+        <parameter>access</parameter> <type>timestamp with time zone</type>,
         <parameter>modification</parameter> <type>timestamp with time zone</type>,
+        <parameter>change</parameter> <type>timestamp with time zone</type>,
+        <parameter>creation</parameter> <type>timestamp with time zone</type>,
         <parameter>isdir</parameter> <type>boolean</type> )
        </para>
        <para>
         For each file in the server's write-ahead log (WAL) directory, list the
-        file's name, size, last modification time (mtime), and a boolean
-        indicating if the file is a directory.
+        file's name along with the metadata columns returned by
+        <function>pg_stat_file</function>.
         Filenames beginning with a dot and special files types are excluded.
        </para>
        <para>
@@ -25815,14 +25824,17 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size
         <returnvalue>setof record</returnvalue>
         ( <parameter>name</parameter> <type>text</type>,
         <parameter>size</parameter> <type>bigint</type>,
+        <parameter>access</parameter> <type>timestamp with time zone</type>,
         <parameter>modification</parameter> <type>timestamp with time zone</type>,
+        <parameter>change</parameter> <type>timestamp with time zone</type>,
+        <parameter>creation</parameter> <type>timestamp with time zone</type>,
         <parameter>isdir</parameter> <type>boolean</type> )
        </para>
        <para>
         For each file in the server's WAL archive status directory
-        (<filename>pg_wal/archive_status</filename>), list the file's
-        name, size, last modification time (mtime), and a boolean indicating if
-        the file is a directory.
+        (<filename>pg_wal/archive_status</filename>), list the file's name
+        along with the metadata columns returned by
+        <function>pg_stat_file</function>.
         Filenames beginning with a dot and special file types are excluded.
        </para>
        <para>
@@ -25842,13 +25854,17 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size
         <returnvalue>setof record</returnvalue>
         ( <parameter>name</parameter> <type>text</type>,
         <parameter>size</parameter> <type>bigint</type>,
+        <parameter>access</parameter> <type>timestamp with time zone</type>,
         <parameter>modification</parameter> <type>timestamp with time zone</type>,
+        <parameter>change</parameter> <type>timestamp with time zone</type>,
+        <parameter>creation</parameter> <type>timestamp with time zone</type>,
         <parameter>isdir</parameter> <type>boolean</type> )
        </para>
        <para>
         For each file in the temporary directory within the given
-        <parameter>tablespace</parameter>, return the file's name, size, last
-        modification time (mtime) and a boolean indicating if the file is a directory.
+        <parameter>tablespace</parameter>, list the file's name
+        along with the metadata columns returned by
+        <function>pg_stat_file</function>.
         Directories are used for temporary files shared by parallel processes.
         If <parameter>tablespace</parameter> is not provided, the
         <literal>pg_default</literal> tablespace is examined.
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index 07fce2a9bc..9ff06f4158 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -36,6 +36,8 @@
 #include "utils/syscache.h"
 #include "utils/timestamp.h"
 
+static void tuple_from_stat(struct stat *fst, const char *path, Datum *values,
+		bool *nulls);
 static Datum pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags);
 
 #define	LS_DIR_ISDIR				(1<<0) /* Show column: isdir */
@@ -397,6 +399,28 @@ pg_read_binary_file_all(PG_FUNCTION_ARGS)
 	return pg_read_binary_file(fcinfo);
 }
 
+/*
+ * Populate values and nulls from fst and path.
+ * Used for pg_stat_file() and pg_ls_dir_files()
+ * nulls is assumed to have been zerod.
+ */
+static void
+tuple_from_stat(struct stat *fst, const char *path, Datum *values, bool *nulls)
+{
+	values[0] = Int64GetDatum((int64) fst->st_size);
+	values[1] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_atime));
+	values[2] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_mtime));
+	/* Unix has file status change time, while Win32 has creation time */
+#if !defined(WIN32) && !defined(__CYGWIN__)
+	values[3] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_ctime));
+	nulls[4] = true;
+#else
+	nulls[3] = true;
+	values[4] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_ctime));
+#endif
+	values[5] = BoolGetDatum(S_ISDIR(fst->st_mode));
+}
+
 /*
  * stat a file
  */
@@ -407,7 +431,7 @@ pg_stat_file(PG_FUNCTION_ARGS)
 	char	   *filename;
 	struct stat fst;
 	Datum		values[6];
-	bool		isnull[6];
+	bool		nulls[6];
 	HeapTuple	tuple;
 	TupleDesc	tupdesc;
 	bool		missing_ok = false;
@@ -447,27 +471,9 @@ pg_stat_file(PG_FUNCTION_ARGS)
 					   "isdir", BOOLOID, -1, 0);
 	BlessTupleDesc(tupdesc);
 
-	memset(isnull, false, sizeof(isnull));
-
-	values[0] = Int64GetDatum((int64) fst.st_size);
-	values[1] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_atime));
-	values[2] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_mtime));
-	/* Unix has file status change time, while Win32 has creation time */
-#if !defined(WIN32) && !defined(__CYGWIN__)
-	values[3] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_ctime));
-	isnull[4] = true;
-#else
-	isnull[3] = true;
-	values[4] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_ctime));
-#endif
-	values[5] = BoolGetDatum(S_ISDIR(fst.st_mode));
-#ifdef WIN32
-	/* Links should have isdir=false */
-	if (pgwin32_is_junction(filename))
-		values[5] = BoolGetDatum(false);
-#endif
-
-	tuple = heap_form_tuple(tupdesc, values, isnull);
+	memset(nulls, false, sizeof(nulls));
+	tuple_from_stat(&fst, filename, values, nulls);
+	tuple = heap_form_tuple(tupdesc, values, nulls);
 
 	pfree(filename);
 
@@ -608,8 +614,8 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags)
 
 	while ((de = ReadDir(dirdesc, dir)) != NULL)
 	{
-		Datum		values[4];
-		bool		nulls[4];
+		Datum		values[7];
+		bool		nulls[7];
 		char		path[MAXPGPATH * 2];
 		struct stat attrib;
 
@@ -648,23 +654,10 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags)
 				continue;
 		}
 
+		memset(nulls, false, sizeof(nulls));
 		values[0] = CStringGetTextDatum(de->d_name);
 		if (flags & LS_DIR_METADATA)
-		{
-			values[1] = Int64GetDatum((int64) attrib.st_size);
-			values[2] = TimestampTzGetDatum(time_t_to_timestamptz(attrib.st_mtime));
-			if (flags & LS_DIR_ISDIR)
-			{
-				values[3] = BoolGetDatum(S_ISDIR(attrib.st_mode));
-#ifdef WIN32
-				/* Links should have isdir=false */
-				if (pgwin32_is_junction(path))
-					values[3] = BoolGetDatum(false);
-#endif
-			}
-		}
-
-		memset(nulls, 0, sizeof(nulls));
+			tuple_from_stat(&attrib, path, 1+values, 1+nulls);
 
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 	}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index bb57f3bc70..7fea405068 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10951,41 +10951,41 @@
 { oid => '3353', descr => 'list files in the log directory',
   proname => 'pg_ls_logdir', procost => '10', prorows => '20', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => '',
-  proallargtypes => '{text,int8,timestamptz,bool}', proargmodes => '{o,o,o,o}',
-  proargnames => '{name,size,modification,isdir}', prosrc => 'pg_ls_logdir' },
+  proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}',
+  proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_logdir' },
 { oid => '3354', descr => 'list of files in the WAL directory',
   proname => 'pg_ls_waldir', procost => '10', prorows => '20', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => '',
-  proallargtypes => '{text,int8,timestamptz,bool}', proargmodes => '{o,o,o,o}',
-  proargnames => '{name,size,modification,isdir}', prosrc => 'pg_ls_waldir' },
+  proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}',
+  proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_waldir' },
 { oid => '5031', descr => 'list of files in the archive_status directory',
   proname => 'pg_ls_archive_statusdir', procost => '10', prorows => '20',
   proretset => 't', provolatile => 'v', prorettype => 'record',
-  proargtypes => '', proallargtypes => '{text,int8,timestamptz,bool}',
-  proargmodes => '{o,o,o,o}', proargnames => '{name,size,modification,isdir}',
+  proargtypes => '', proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}',
+  proargmodes => '{o,o,o,o,o,o,o}', proargnames => '{name,size,access,modification,change,creation,isdir}',
   prosrc => 'pg_ls_archive_statusdir' },
 { oid => '5029', descr => 'list files in the pgsql_tmp directory',
   proname => 'pg_ls_tmpdir', procost => '10', prorows => '20', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => '',
-  proallargtypes => '{text,int8,timestamptz,bool}', proargmodes => '{o,o,o,o}',
-  proargnames => '{name,size,modification,isdir}', prosrc => 'pg_ls_tmpdir_noargs' },
+  proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}',
+  proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_tmpdir_noargs' },
 { oid => '5030', descr => 'list files in the pgsql_tmp directory',
   proname => 'pg_ls_tmpdir', procost => '10', prorows => '20', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => 'oid',
-  proallargtypes => '{oid,text,int8,timestamptz,bool}', proargmodes => '{i,o,o,o,o}',
-  proargnames => '{tablespace,name,size,modification,isdir}',
+  proallargtypes => '{oid,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,o,o,o,o,o,o,o}',
+  proargnames => '{tablespace,name,size,access,modification,change,creation,isdir}',
   prosrc => 'pg_ls_tmpdir_1arg' },
 { oid => '9979', descr => 'list directory with metadata',
   proname => 'pg_ls_dir_metadata', procost => '10', prorows => '20', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => 'text bool bool',
-  proallargtypes => '{text,bool,bool,text,int8,timestamptz,bool}', proargmodes => '{i,i,i,o,o,o,o}',
-  proargnames => '{dirname,missing_ok,include_dot_dirs,filename,size,modification,isdir}',
+  proallargtypes => '{text,bool,bool,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,i,i,o,o,o,o,o,o,o}',
+  proargnames => '{dirname,missing_ok,include_dot_dirs,filename,size,access,modification,change,creation,isdir}',
   prosrc => 'pg_ls_dir_metadata' },
 { oid => '9980', descr => 'list directory with metadata',
   proname => 'pg_ls_dir_metadata', procost => '10', prorows => '20', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => 'text',
-  proallargtypes => '{text,text,int8,timestamptz,bool}', proargmodes => '{i,o,o,o,o}',
-  proargnames => '{dirname,filename,size,modification,isdir}',
+  proallargtypes => '{text,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,o,o,o,o,o,o,o}',
+  proargnames => '{dirname,filename,size,access,modification,change,creation,isdir}',
   prosrc => 'pg_ls_dir_metadata_1arg' },
 
 # hash partitioning constraint function
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index 800d12b4b6..1f4106cd9c 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -157,8 +157,8 @@ select count(*) > 0 as ok from (select pg_ls_waldir()) ss;
 
 -- Test not-run-to-completion cases.
 select * from pg_ls_waldir() limit 0;
- name | size | modification | isdir 
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir 
+------+------+--------+--------------+--------+----------+-------
 (0 rows)
 
 select count(*) > 0 as ok from (select * from pg_ls_waldir() limit 1) ss;
@@ -228,8 +228,8 @@ select * from pg_stat_file('.') limit 0;
 -- This tests the missing_ok parameter, which causes pg_ls_tmpdir to succeed even if the tmpdir doesn't exist yet
 -- The name='' condition is never true, so the function runs to completion but returns zero rows.
 select * from pg_ls_tmpdir() where name='Does not exist';
- name | size | modification | isdir 
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir 
+------+------+--------+--------------+--------+----------+-------
 (0 rows)
 
 select filename, isdir from pg_ls_dir_metadata('.') where filename='.';
@@ -245,8 +245,8 @@ select filename, isdir from pg_ls_dir_metadata('.', false, false) where filename
 
 -- Check that expected columns are present
 select * from pg_ls_dir_metadata('.') limit 0;
- filename | size | modification | isdir 
-----------+------+--------------+-------
+ filename | size | access | modification | change | creation | isdir 
+----------+------+--------+--------------+--------+----------+-------
 (0 rows)
 
 --
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index ba9a3fe29a..1e1e02b589 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -17,15 +17,15 @@ CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@';
 -- The name='' condition is never true, so the function runs to completion but returns zero rows.
 -- The query is written to ERROR if the tablespace doesn't exist, rather than silently failing to call pg_ls_tmpdir()
 SELECT c.* FROM (SELECT oid FROM pg_tablespace b WHERE b.spcname='regress_tblspace' UNION SELECT 0 ORDER BY 1 DESC LIMIT 1) AS b , pg_ls_tmpdir(oid) AS c WHERE c.name='Does not exist';
- name | size | modification | isdir 
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir 
+------+------+--------+--------------+--------+----------+-------
 (0 rows)
 
 -- This tests the missing_ok parameter.  If that's not functioning, this would ERROR if the logdir doesn't exist yet.
 -- The name='' condition is never true, so the function runs to completion but returns zero rows.
 SELECT * FROM pg_ls_logdir() WHERE name='Does not exist';
- name | size | modification | isdir 
-------+------+--------------+-------
+ name | size | access | modification | change | creation | isdir 
+------+------+--------+--------------+--------+----------+-------
 (0 rows)
 
 -- try setting and resetting some properties for the new tablespace
-- 
2.17.0


--mPTHnM80CEnHQ2WJ
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v24-0009-pg_ls_-pg_stat_file-to-show-file-type.patch"



^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Fix output of zero privileges in psql
@ 2023-09-17 19:31  Erik Wienhold <[email protected]>
  0 siblings, 2 replies; 5+ messages in thread

From: Erik Wienhold @ 2023-09-17 19:31 UTC (permalink / raw)
  To: PostgreSQL Hackers <[email protected]>

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

--
Erik

Attachments:

  [text/x-patch] v1-0001-Fix-output-of-zero-privileges-in-psql.patch (9.6K, ../../[email protected]/2-v1-0001-Fix-output-of-zero-privileges-in-psql.patch)
  download | inline diff:
From 16af995acb4c0e5fb5981308610a76b7e87c3358 Mon Sep 17 00:00:00 2001
From: Erik Wienhold <[email protected]>
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



^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: Fix output of zero privileges in psql
@ 2023-09-17 19:37  Erik Wienhold <[email protected]>
  parent: Erik Wienhold <[email protected]>
  1 sibling, 0 replies; 5+ messages in thread

From: Erik Wienhold @ 2023-09-17 19:37 UTC (permalink / raw)
  To: PostgreSQL Hackers <[email protected]>

On 17/09/2023 21:31 CEST Erik Wienhold <[email protected]> wrote:

> This affects the following meta commands:
>
>     \db+ \dD+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l

also \des+ and \dew+

--
Erik






^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: Fix output of zero privileges in psql
@ 2023-10-06 20:32  Laurenz Albe <[email protected]>
  parent: Erik Wienhold <[email protected]>
  1 sibling, 1 reply; 5+ messages in thread

From: Laurenz Albe @ 2023-10-06 20:32 UTC (permalink / raw)
  To: Erik Wienhold <[email protected]>; PostgreSQL Hackers <[email protected]>

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] 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)".

The simple attached patch does it like that.  What do you think?

Yours,
Laurenz Albe


Attachments:

  [text/x-patch] 0001-psql-honor-pset-null-in-backslash-commands.patch (10.9K, ../../[email protected]/2-0001-psql-honor-pset-null-in-backslash-commands.patch)
  download | inline diff:
From 6c67f15f011ddf1e309cb7e84580b266d674a1e2 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <[email protected]>
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



^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: Fix output of zero privileges in psql
@ 2023-10-07 03:07  Erik Wienhold <[email protected]>
  parent: Laurenz Albe <[email protected]>
  0 siblings, 0 replies; 5+ messages in thread

From: Erik Wienhold @ 2023-10-07 03:07 UTC (permalink / raw)
  To: Laurenz Albe <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

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] 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)".

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






^ permalink  raw  reply  [nested|flat] 5+ messages in thread


end of thread, other threads:[~2023-10-07 03:07 UTC | newest]

Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-10 02:56 [PATCH v24 08/11] pg_ls_*dir to return all the metadata from pg_stat_file.. Justin Pryzby <[email protected]>
2023-09-17 19:31 Fix output of zero privileges in psql Erik Wienhold <[email protected]>
2023-09-17 19:37 ` Re: Fix output of zero privileges in psql Erik Wienhold <[email protected]>
2023-10-06 20:32 ` Re: Fix output of zero privileges in psql Laurenz Albe <[email protected]>
2023-10-07 03:07   ` Re: Fix output of zero privileges in psql Erik Wienhold <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox