add \dpS to psql
Hi hackers,
As discussed elsewhere [0]/messages/by-id/a2382acd-e465-85b2-9d8e-f9ed1a5a66e9@postgrespro.ru, \dp doesn't show privileges on system objects,
and this behavior is not mentioned in the docs. I've attached a small
patch that adds support for the S modifier (i.e., \dpS) and the adjusts the
docs.
Thoughts?
[0]: /messages/by-id/a2382acd-e465-85b2-9d8e-f9ed1a5a66e9@postgrespro.ru
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
add_s_modifier_to_dp.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index d3dd638b14..406936dd1c 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1825,14 +1825,16 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
<varlistentry>
- <term><literal>\dp [ <link linkend="app-psql-patterns"><replaceable class="parameter">pattern</replaceable></link> ]</literal></term>
+ <term><literal>\dp[S] [ <link linkend="app-psql-patterns"><replaceable class="parameter">pattern</replaceable></link> ]</literal></term>
<listitem>
<para>
Lists tables, views and sequences with their
associated access privileges.
If <replaceable class="parameter">pattern</replaceable> is
specified, only tables, views and sequences whose names match the
- pattern are listed.
+ pattern are listed. By default only user-created objects are shown;
+ supply a pattern or the <literal>S</literal> modifier to include system
+ objects.
</para>
<para>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index de6a3a71f8..3520655dc0 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -875,7 +875,7 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
success = listCollations(pattern, show_verbose, show_system);
break;
case 'p':
- success = permissionsList(pattern);
+ success = permissionsList(pattern, show_system);
break;
case 'P':
{
@@ -2831,7 +2831,7 @@ exec_command_z(PsqlScanState scan_state, bool active_branch)
char *pattern = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
- success = permissionsList(pattern);
+ success = permissionsList(pattern, false);
free(pattern);
}
else
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 2eae519b1d..eb98797d67 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1002,7 +1002,7 @@ listAllDbs(const char *pattern, bool verbose)
* \z (now also \dp -- perhaps more mnemonic)
*/
bool
-permissionsList(const char *pattern)
+permissionsList(const char *pattern, bool showSystem)
{
PQExpBufferData buf;
PGresult *res;
@@ -1121,15 +1121,12 @@ permissionsList(const char *pattern)
CppAsString2(RELKIND_FOREIGN_TABLE) ","
CppAsString2(RELKIND_PARTITIONED_TABLE) ")\n");
- /*
- * Unless a schema pattern is specified, we suppress system and temp
- * tables, since they normally aren't very interesting from a permissions
- * point of view. You can see 'em by explicit request though, eg with \z
- * pg_catalog.*
- */
+ if (!showSystem && !pattern)
+ appendPQExpBufferStr(&buf, "AND n.nspname !~ '^pg_'\n");
+
if (!validateSQLNamePattern(&buf, pattern, true, false,
"n.nspname", "c.relname", NULL,
- "n.nspname !~ '^pg_' AND pg_catalog.pg_table_is_visible(c.oid)",
+ "pg_catalog.pg_table_is_visible(c.oid)",
NULL, 3))
goto error_return;
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index bd051e09cb..58d0cf032b 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -38,7 +38,7 @@ extern bool describeRoles(const char *pattern, bool verbose, bool showSystem);
extern bool listDbRoleSettings(const char *pattern, const char *pattern2);
/* \z (or \dp) */
-extern bool permissionsList(const char *pattern);
+extern bool permissionsList(const char *pattern, bool showSystem);
/* \ddp */
extern bool listDefaultACLs(const char *pattern);
On 06.12.2022 22:36, Nathan Bossart wrote:
As discussed elsewhere [0], \dp doesn't show privileges on system objects,
and this behavior is not mentioned in the docs. I've attached a small
patch that adds support for the S modifier (i.e., \dpS) and the adjusts the
docs.Thoughts?
[0] /messages/by-id/a2382acd-e465-85b2-9d8e-f9ed1a5a66e9@postgrespro.ru
A few words in support of this patch, since I was the initiator of the
discussion.
Before VACUUM, ANALYZE privileges, there was no such question.
Why check privileges on system catalog objects? But now it doesn't.
It is now possible to grant privileges on system tables,
so it should be possible to see privileges with psql commands.
However, the \dp command does not support the S modifier, which is
inconsistent.
Furthermore. The VACUUM privilege allows you to also execute VACUUM FULL.
VACUUM and VACUUM FULL are commands with similar names, but work
completely differently.
It may be worth clarifying on this page:
https://www.postgresql.org/docs/devel/ddl-priv.html
Something like: Allows VACUUM on a relation, including VACUUM FULL.
But that's not all.
There is a very similar command to VACUUM FULL with a different name -
CLUSTER.
The VACUUM privilege does not apply to the CLUSTER command. This is
probably correct.
However, the documentation for the CLUSTER command does not say
who can perform this command. I think it would be correct to add a sentence
to the Notes section
(https://www.postgresql.org/docs/devel/sql-cluster.html)
similar to the one in the VACUUM documentation:
"To cluster a table, one must ordinarily be the table's owner or a
superuser."
Ready to participate, if it seems reasonable.
--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
On Wed, Dec 07, 2022 at 10:48:49AM +0300, Pavel Luzanov wrote:
There is a very similar command to VACUUM FULL with a different name -
CLUSTER.
The VACUUM privilege does not apply to the CLUSTER command. This is probably
correct.
However, the documentation for the CLUSTER command does not say
who can perform this command. I think it would be correct to add a sentence
to the Notes section
(https://www.postgresql.org/docs/devel/sql-cluster.html)
similar to the one in the VACUUM documentation:"To cluster a table, one must ordinarily be the table's owner or a
superuser."
I created a new thread for this:
/messages/by-id/20221207223924.GA4182184@nathanxps13
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, Dec 07, 2022 at 10:48:49AM +0300, Pavel Luzanov wrote:
Furthermore. The VACUUM privilege allows you to also execute VACUUM FULL.
VACUUM and VACUUM FULL are commands with similar names, but work completely
differently.
It may be worth clarifying on this page:
https://www.postgresql.org/docs/devel/ddl-priv.htmlSomething like: Allows VACUUM on a relation, including VACUUM FULL.
Since (as you said) they work completely differently, I think it'd be
more useful if vacuum_full were a separate privilege, rather than being
included in vacuum. And cluster could be allowed whenever vacuum_full
is allowed.
There is a very similar command to VACUUM FULL with a different name -
CLUSTER. The VACUUM privilege does not apply to the CLUSTER command.
This is probably correct.
I think if vacuum privilege allows vacuum full, then it ought to also
allow cluster. But I suggest that it'd be even better if it doesn't
allow either, and there was a separate privilege for those.
Disclaimer: I have not been following these threads.
--
Justin
On Wed, Dec 07, 2022 at 08:39:30PM -0600, Justin Pryzby wrote:
I think if vacuum privilege allows vacuum full, then it ought to also
allow cluster. But I suggest that it'd be even better if it doesn't
allow either, and there was a separate privilege for those.Disclaimer: I have not been following these threads.
I haven't formed an opinion on whether VACUUM FULL should get its own bit,
but FWIW І just finished writing the first draft of a patch set to add bits
for CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX. I plan to post that
tomorrow.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
I haven't formed an opinion on whether VACUUM FULL should get its own bit,
but FWIW І just finished writing the first draft of a patch set to add bits
for CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX. I plan to post that
tomorrow.
The fact that we just doubled the number of available bits doesn't
mean we should immediately strive to use them up. Perhaps it'd
be better to subsume these retail privileges under some generic
"maintenance action" privilege?
regards, tom lane
On Wed, Dec 07, 2022 at 11:25:32PM -0500, Tom Lane wrote:
The fact that we just doubled the number of available bits doesn't
mean we should immediately strive to use them up. Perhaps it'd
be better to subsume these retail privileges under some generic
"maintenance action" privilege?
That's fine with me, but I wouldn't be surprised if there's disagreement on
how to group the commands. I certainly don't want to use up the rest of
the bits right away, but there might not be too many more existing
privileges after these three that deserve them. Perhaps I should take
inventory and offer a plan for all the remaining privileges.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, 7 Dec 2022 at 23:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Nathan Bossart <nathandbossart@gmail.com> writes:
I haven't formed an opinion on whether VACUUM FULL should get its own
bit,
but FWIW І just finished writing the first draft of a patch set to add
bits
for CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX. I plan to post that
tomorrow.The fact that we just doubled the number of available bits doesn't
mean we should immediately strive to use them up. Perhaps it'd
be better to subsume these retail privileges under some generic
"maintenance action" privilege?
That was my original suggestion:
/messages/by-id/CAMsGm5c4DycKBYZCypfV02s-SC8GwF+KeTt==vbWrFn+dz=Keg@mail.gmail.com
In that message I review the history of permission bit growth. A bit later
in the discussion, I did some investigation into the history of demand for
new permission bits and I proposed calling the new privilege MAINTAIN:
/messages/by-id/CAMsGm5d=2gi4kyKONUJyYFwen=bsWm4hz_KxLXkEhMmg5WSWTA@mail.gmail.com
For what it's worth, I wouldn't bother changing the format of the
permission bits to expand the pool of available bits. My previous analysis
shows that there is no vast hidden demand for new privilege bits. If we
implement MAINTAIN to control access to VACUUM, ANALYZE, REFRESH, CLUSTER,
and REINDEX, we will cover everything that I can find that has seriously
discussed on this list, and still leave 3 unused bits for future expansion.
There is even justification for stopping after this expansion: if it is
done, then schema changes (DDL) will only be able to be done by owner; data
changes (insert, update, delete, as well as triggering of automatic data
maintenance actions) will be able to be done by anybody who is granted
permission.
My guess is that if we ever do expand the privilege bit system, it should
be in a way that removes the limit entirely, replacing a bit map model with
something more like a table with one row for each individual grant, with a
field indicating which grant is involved. But that is a hypothetical future.
On Wed, Dec 07, 2022 at 11:48:20PM -0500, Isaac Morland wrote:
For what it's worth, I wouldn't bother changing the format of the
permission bits to expand the pool of available bits.
7b37823 expanded AclMode to 64 bits, so we now have room for 16 additional
privileges (after the addition of VACUUM and ANALYZE in b5d6382).
My previous analysis
shows that there is no vast hidden demand for new privilege bits. If we
implement MAINTAIN to control access to VACUUM, ANALYZE, REFRESH, CLUSTER,
and REINDEX, we will cover everything that I can find that has seriously
discussed on this list, and still leave 3 unused bits for future expansion.
If we added CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX as individual
privilege bits, we'd still have 13 remaining for future use.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Thu, 8 Dec 2022 at 00:07, Nathan Bossart <nathandbossart@gmail.com>
wrote:
On Wed, Dec 07, 2022 at 11:48:20PM -0500, Isaac Morland wrote:
For what it's worth, I wouldn't bother changing the format of the
permission bits to expand the pool of available bits.7b37823 expanded AclMode to 64 bits, so we now have room for 16 additional
privileges (after the addition of VACUUM and ANALYZE in b5d6382).My previous analysis
shows that there is no vast hidden demand for new privilege bits. If we
implement MAINTAIN to control access to VACUUM, ANALYZE, REFRESH,CLUSTER,
and REINDEX, we will cover everything that I can find that has seriously
discussed on this list, and still leave 3 unused bits for futureexpansion.
If we added CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX as individual
privilege bits, we'd still have 13 remaining for future use.
I was a bit imprecise. I was comparing to the state before the recent
changes - so 12 bits used out of 16, with MAINTAIN being the 13th bit. I
think in my mind it's still approximately 2019 on some level.
Nathan Bossart <nathandbossart@gmail.com> writes:
On Wed, Dec 07, 2022 at 11:48:20PM -0500, Isaac Morland wrote:
My previous analysis
shows that there is no vast hidden demand for new privilege bits. If we
implement MAINTAIN to control access to VACUUM, ANALYZE, REFRESH, CLUSTER,
and REINDEX, we will cover everything that I can find that has seriously
discussed on this list, and still leave 3 unused bits for future expansion.
If we added CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX as individual
privilege bits, we'd still have 13 remaining for future use.
I think the appropriate question is not "have we still got bits left?".
It should be more like "under what plausible scenario would it be useful
to grant somebody CLUSTER but not VACUUM privileges on a table?".
I'm really thinking that MAINTAIN is the right level of granularity
here. Or maybe it's worth segregating exclusive-lock from
not-exclusive-lock maintenance. But I really fail to see how it's
useful to distinguish CLUSTER from REINDEX, say.
regards, tom lane
On 08.12.2022 07:48, Isaac Morland wrote:
If we implement MAINTAIN to control access to VACUUM, ANALYZE,
REFRESH, CLUSTER, and REINDEX, we will cover everything that I can
find that has seriously discussed on this list
I like this approach with MAINTAIN privilege. I'm trying to find any
disadvantages ... and I can't.
For the complete picture, I tried to see what other actions with the
table could *potentially* be considered as maintenance.
Here is the list:
- create|alter|drop on extended statistics objects
- alter table|index alter column set statistics
- alter table|index [re]set (storage_parameters)
- alter table|index set tablespace
- alter table alter column set storage|compression
- any actions with the TOAST table that can be performed separately from
the main table
I have to admit that the discussion has moved away from the $subject.
--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
On 2022-Dec-08, Pavel Luzanov wrote:
For the complete picture, I tried to see what other actions with the table
could *potentially* be considered as maintenance.
Here is the list:- create|alter|drop on extended statistics objects
- alter table|index alter column set statistics
- alter table|index [re]set (storage_parameters)
- alter table|index set tablespace
- alter table alter column set storage|compression
- any actions with the TOAST table that can be performed separately from the
main table
Well, I can't see that any of these is valuable to grant separately from
the table's owner. The maintenance ones are the ones that are
interesting to run from a database-owner perspective, but these ones do
not seem to need that treatment.
If you're extremely generous you could think that ALTER .. SET STORAGE
would be reasonable to be run by the db-owner. However, that's not
something you do on an ongoing basis -- you just do it once -- so it
seems pointless.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On Thu, Dec 08, 2022 at 12:15:23AM -0500, Tom Lane wrote:
Nathan Bossart <nathandbossart@gmail.com> writes:
On Wed, Dec 07, 2022 at 11:48:20PM -0500, Isaac Morland wrote:
My previous analysis
shows that there is no vast hidden demand for new privilege bits. If we
implement MAINTAIN to control access to VACUUM, ANALYZE, REFRESH, CLUSTER,
and REINDEX, we will cover everything that I can find that has seriously
discussed on this list, and still leave 3 unused bits for future expansion.If we added CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX as individual
privilege bits, we'd still have 13 remaining for future use.I think the appropriate question is not "have we still got bits left?".
It should be more like "under what plausible scenario would it be useful
to grant somebody CLUSTER but not VACUUM privileges on a table?".I'm really thinking that MAINTAIN is the right level of granularity
here. Or maybe it's worth segregating exclusive-lock from
not-exclusive-lock maintenance. But I really fail to see how it's
useful to distinguish CLUSTER from REINDEX, say.
The main idea behind this work is breaking out privileges into more
granular pieces. If I want to create a role that only runs VACUUM on some
tables on the weekend, why ѕhould I have to also give it the ability to
ANALYZE, REFRESH, CLUSTER, and REINDEX? IMHO we should really let the user
decide what set of privileges makes sense for their use-case. I'm unsure
the grouping all these privileges together serves much purpose besides
preserving ACL bits.
The other reason I'm hesitant to group the privileges together is because I
suspect it will be difficult to reach agreement on how to do so, as
evidenced by past discussion [0]/messages/by-id/67a1d667e8ec228b5e07f232184c80348c5d93f4.camel@j-davis.com. That being said, I'm open to it if we
find a way that folks are happy with. For example, separating
exclusive-lock and non-exclusive-lock maintenance actions seems like a
reasonable idea (which perhaps is an argument for moving VACUUM FULL out of
the VACUUM privilege).
[0]: /messages/by-id/67a1d667e8ec228b5e07f232184c80348c5d93f4.camel@j-davis.com
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
I've created a new thread for making CLUSTER, REFRESH MATERIALIZED VIEW,
and REINDEX grantable:
/messages/by-id/20221208183707.GA55474@nathanxps13
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Thu, Dec 08, 2022 at 09:15:03AM -0800, Nathan Bossart wrote:
The main idea behind this work is breaking out privileges into more
granular pieces. If I want to create a role that only runs VACUUM on some
tables on the weekend, why ѕhould I have to also give it the ability to
ANALYZE, REFRESH, CLUSTER, and REINDEX? IMHO we should really let the user
decide what set of privileges makes sense for their use-case. I'm unsure
the grouping all these privileges together serves much purpose besides
preserving ACL bits.
Hmm. I'd like to think that we should keep a frugal mind here. More
bits are now available, but it does not strike me as a good idea to
force their usage more than necessary, so grouping all these no-quite
DDL commands into the same bag does not sound that bad to me.
--
Michael
On Fri, Dec 09, 2022 at 01:36:03PM +0900, Michael Paquier wrote:
On Thu, Dec 08, 2022 at 09:15:03AM -0800, Nathan Bossart wrote:
The main idea behind this work is breaking out privileges into more
granular pieces. If I want to create a role that only runs VACUUM on some
tables on the weekend, why ѕhould I have to also give it the ability to
ANALYZE, REFRESH, CLUSTER, and REINDEX? IMHO we should really let the user
decide what set of privileges makes sense for their use-case. I'm unsure
the grouping all these privileges together serves much purpose besides
preserving ACL bits.Hmm. I'd like to think that we should keep a frugal mind here. More
bits are now available, but it does not strike me as a good idea to
force their usage more than necessary, so grouping all these no-quite
DDL commands into the same bag does not sound that bad to me.
Okay, it seems I am outnumbered. I will work on updating the patch to add
an ACL_MAINTAIN bit and a pg_maintain_all_tables predefined role.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Fri, Dec 09, 2022 at 10:40:55AM -0800, Nathan Bossart wrote:
On Fri, Dec 09, 2022 at 01:36:03PM +0900, Michael Paquier wrote:
On Thu, Dec 08, 2022 at 09:15:03AM -0800, Nathan Bossart wrote:
The main idea behind this work is breaking out privileges into more
granular pieces. If I want to create a role that only runs VACUUM on some
tables on the weekend, why ѕhould I have to also give it the ability to
ANALYZE, REFRESH, CLUSTER, and REINDEX? IMHO we should really let the user
decide what set of privileges makes sense for their use-case. I'm unsure
the grouping all these privileges together serves much purpose besides
preserving ACL bits.Hmm. I'd like to think that we should keep a frugal mind here. More
bits are now available, but it does not strike me as a good idea to
force their usage more than necessary, so grouping all these no-quite
DDL commands into the same bag does not sound that bad to me.Okay, it seems I am outnumbered. I will work on updating the patch to add
an ACL_MAINTAIN bit and a pg_maintain_all_tables predefined role.
Any thoughts on $SUBJECT?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On 2022-12-09 Fr 13:44, Nathan Bossart wrote:
On Fri, Dec 09, 2022 at 10:40:55AM -0800, Nathan Bossart wrote:
On Fri, Dec 09, 2022 at 01:36:03PM +0900, Michael Paquier wrote:
On Thu, Dec 08, 2022 at 09:15:03AM -0800, Nathan Bossart wrote:
The main idea behind this work is breaking out privileges into more
granular pieces. If I want to create a role that only runs VACUUM on some
tables on the weekend, why ѕhould I have to also give it the ability to
ANALYZE, REFRESH, CLUSTER, and REINDEX? IMHO we should really let the user
decide what set of privileges makes sense for their use-case. I'm unsure
the grouping all these privileges together serves much purpose besides
preserving ACL bits.Hmm. I'd like to think that we should keep a frugal mind here. More
bits are now available, but it does not strike me as a good idea to
force their usage more than necessary, so grouping all these no-quite
DDL commands into the same bag does not sound that bad to me.Okay, it seems I am outnumbered. I will work on updating the patch to add
an ACL_MAINTAIN bit and a pg_maintain_all_tables predefined role.Any thoughts on $SUBJECT?
Yeah, the discussion got way off into the weeds here. I think the
original proposal seems reasonable. Please add it to the next CF if you
haven't already.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Mon, Dec 12, 2022 at 07:01:01AM -0500, Andrew Dunstan wrote:
On 2022-12-09 Fr 13:44, Nathan Bossart wrote:
Any thoughts on $SUBJECT?
Yeah, the discussion got way off into the weeds here. I think the
original proposal seems reasonable. Please add it to the next CF if you
haven't already.
Here it is: https://commitfest.postgresql.org/41/4043/
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Here it is: https://commitfest.postgresql.org/41/4043/
Hi!
The patch applies with no problem, implements what it declared, CF bot is
happy.
Without patch \dpS shows 0 rows, after applying system objects are shown.
Consider this patch useful, hope it will be committed soon.
--
Best regards,
Maxim Orlov.
On Wed, Dec 28, 2022 at 02:46:23PM +0300, Maxim Orlov wrote:
The patch applies with no problem, implements what it declared, CF bot is
happy.
Without patch \dpS shows 0 rows, after applying system objects are shown.
Consider this patch useful, hope it will be committed soon.
Thanks for reviewing.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, 28 Dec 2022 at 21:26, Nathan Bossart <nathandbossart@gmail.com> wrote:
On Wed, Dec 28, 2022 at 02:46:23PM +0300, Maxim Orlov wrote:
The patch applies with no problem, implements what it declared, CF bot is
happy.
Without patch \dpS shows 0 rows, after applying system objects are shown.
Consider this patch useful, hope it will be committed soon.Thanks for reviewing.
Looking this over this, I have a couple of comments:
Firstly, I think it should allow \zS in the same fashion as \dpS,
since \z is an alias for \dp, so the 2 should be kept in sync.
Secondly, I don't think the following is the right SQL clause to use
in the absence of "S":
if (!showSystem && !pattern)
appendPQExpBufferStr(&buf, "AND n.nspname !~ '^pg_'\n");
I know that's the condition it used before, but the problem with using
that now is that it will cause temporary relations to be excluded
unless the "S" modifier is used, which goes against the expectation
that "S" just causes system relations to be included. Also, it fails
to exclude information_schema relations, if that happens to be on the
user's search_path.
So I think we should use the same SQL clauses as every other psql
command that supports "S", namely:
if (!showSystem && !pattern)
appendPQExpBufferStr(&buf, " AND n.nspname <> 'pg_catalog'\n"
" AND n.nspname <> 'information_schema'\n");
Updated patch attached.
Regards,
Dean
Attachments:
add_s_modifier_to_dp-v2.patchtext/x-patch; charset=US-ASCII; name=add_s_modifier_to_dp-v2.patchDownload
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
new file mode 100644
index 8a5285d..3f994a3
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1825,14 +1825,16 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind '
<varlistentry>
- <term><literal>\dp [ <link linkend="app-psql-patterns"><replaceable class="parameter">pattern</replaceable></link> ]</literal></term>
+ <term><literal>\dp[S] [ <link linkend="app-psql-patterns"><replaceable class="parameter">pattern</replaceable></link> ]</literal></term>
<listitem>
<para>
Lists tables, views and sequences with their
associated access privileges.
If <replaceable class="parameter">pattern</replaceable> is
specified, only tables, views and sequences whose names match the
- pattern are listed.
+ pattern are listed. By default only user-created objects are shown;
+ supply a pattern or the <literal>S</literal> modifier to include
+ system objects.
</para>
<para>
@@ -3575,14 +3577,16 @@ testdb=> <userinput>\setenv LESS -imx
<varlistentry>
- <term><literal>\z [ <link linkend="app-psql-patterns"><replaceable class="parameter">pattern</replaceable></link> ]</literal></term>
+ <term><literal>\z[S] [ <link linkend="app-psql-patterns"><replaceable class="parameter">pattern</replaceable></link> ]</literal></term>
<listitem>
<para>
Lists tables, views and sequences with their
associated access privileges.
If a <replaceable class="parameter">pattern</replaceable> is
specified, only tables, views and sequences whose names match the
- pattern are listed.
+ pattern are listed. By default only user-created objects are shown;
+ supply a pattern or the <literal>S</literal> modifier to include
+ system objects.
</para>
<para>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
new file mode 100644
index 00b89d9..b5201ed
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -140,7 +140,8 @@ static backslashResult exec_command_writ
static backslashResult exec_command_watch(PsqlScanState scan_state, bool active_branch,
PQExpBuffer query_buf, PQExpBuffer previous_buf);
static backslashResult exec_command_x(PsqlScanState scan_state, bool active_branch);
-static backslashResult exec_command_z(PsqlScanState scan_state, bool active_branch);
+static backslashResult exec_command_z(PsqlScanState scan_state, bool active_branch,
+ const char *cmd);
static backslashResult exec_command_shell_escape(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_slash_command_help(PsqlScanState scan_state, bool active_branch);
static char *read_connect_arg(PsqlScanState scan_state);
@@ -413,8 +414,8 @@ exec_command(const char *cmd,
query_buf, previous_buf);
else if (strcmp(cmd, "x") == 0)
status = exec_command_x(scan_state, active_branch);
- else if (strcmp(cmd, "z") == 0)
- status = exec_command_z(scan_state, active_branch);
+ else if (strcmp(cmd, "z") == 0 || strcmp(cmd, "zS") == 0)
+ status = exec_command_z(scan_state, active_branch, cmd);
else if (strcmp(cmd, "!") == 0)
status = exec_command_shell_escape(scan_state, active_branch);
else if (strcmp(cmd, "?") == 0)
@@ -875,7 +876,7 @@ exec_command_d(PsqlScanState scan_state,
success = listCollations(pattern, show_verbose, show_system);
break;
case 'p':
- success = permissionsList(pattern);
+ success = permissionsList(pattern, show_system);
break;
case 'P':
{
@@ -2822,16 +2823,22 @@ exec_command_x(PsqlScanState scan_state,
* \z -- list table privileges (equivalent to \dp)
*/
static backslashResult
-exec_command_z(PsqlScanState scan_state, bool active_branch)
+exec_command_z(PsqlScanState scan_state, bool active_branch, const char *cmd)
{
bool success = true;
if (active_branch)
{
- char *pattern = psql_scan_slash_option(scan_state,
- OT_NORMAL, NULL, true);
+ char *pattern;
+ bool show_system;
+
+ pattern = psql_scan_slash_option(scan_state,
+ OT_NORMAL, NULL, true);
+
+ show_system = strchr(cmd, 'S') ? true : false;
+
+ success = permissionsList(pattern, show_system);
- success = permissionsList(pattern);
free(pattern);
}
else
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
new file mode 100644
index 523fab6..e280b6f
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1002,7 +1002,7 @@ listAllDbs(const char *pattern, bool ver
* \z (now also \dp -- perhaps more mnemonic)
*/
bool
-permissionsList(const char *pattern)
+permissionsList(const char *pattern, bool showSystem)
{
PQExpBufferData buf;
PGresult *res;
@@ -1121,15 +1121,13 @@ permissionsList(const char *pattern)
CppAsString2(RELKIND_FOREIGN_TABLE) ","
CppAsString2(RELKIND_PARTITIONED_TABLE) ")\n");
- /*
- * Unless a schema pattern is specified, we suppress system and temp
- * tables, since they normally aren't very interesting from a permissions
- * point of view. You can see 'em by explicit request though, eg with \z
- * pg_catalog.*
- */
+ if (!showSystem && !pattern)
+ appendPQExpBufferStr(&buf, " AND n.nspname <> 'pg_catalog'\n"
+ " AND n.nspname <> 'information_schema'\n");
+
if (!validateSQLNamePattern(&buf, pattern, true, false,
"n.nspname", "c.relname", NULL,
- "n.nspname !~ '^pg_' AND pg_catalog.pg_table_is_visible(c.oid)",
+ "pg_catalog.pg_table_is_visible(c.oid)",
NULL, 3))
goto error_return;
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
new file mode 100644
index 15f62b9..554fe86
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -38,7 +38,7 @@ extern bool describeRoles(const char *pa
extern bool listDbRoleSettings(const char *pattern, const char *pattern2);
/* \z (or \dp) */
-extern bool permissionsList(const char *pattern);
+extern bool permissionsList(const char *pattern, bool showSystem);
/* \ddp */
extern bool listDefaultACLs(const char *pattern);
On Fri, Jan 06, 2023 at 06:52:33PM +0000, Dean Rasheed wrote:
Looking this over this, I have a couple of comments:
Thanks for reviewing.
Firstly, I think it should allow \zS in the same fashion as \dpS,
since \z is an alias for \dp, so the 2 should be kept in sync.
That seems reasonable to me.
Secondly, I don't think the following is the right SQL clause to use
in the absence of "S":if (!showSystem && !pattern)
appendPQExpBufferStr(&buf, "AND n.nspname !~ '^pg_'\n");I know that's the condition it used before, but the problem with using
that now is that it will cause temporary relations to be excluded
unless the "S" modifier is used, which goes against the expectation
that "S" just causes system relations to be included. Also, it fails
to exclude information_schema relations, if that happens to be on the
user's search_path.So I think we should use the same SQL clauses as every other psql
command that supports "S", namely:if (!showSystem && !pattern)
appendPQExpBufferStr(&buf, " AND n.nspname <> 'pg_catalog'\n"
" AND n.nspname <> 'information_schema'\n");
Good catch. I should have noticed this. The deleted comment mentions that
the system/temp tables normally aren't very interesting from a permissions
perspective, so perhaps there is an argument for always excluding temp
tables without a pattern. After all, \dp always excludes indexes and TOAST
tables. However, it looks like \dt includes temp tables, and I didn't see
any other meta-commands that excluded them.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Sat, 7 Jan 2023 at 00:36, Nathan Bossart <nathandbossart@gmail.com> wrote:
On Fri, Jan 06, 2023 at 06:52:33PM +0000, Dean Rasheed wrote:
So I think we should use the same SQL clauses as every other psql
command that supports "S", namely:if (!showSystem && !pattern)
appendPQExpBufferStr(&buf, " AND n.nspname <> 'pg_catalog'\n"
" AND n.nspname <> 'information_schema'\n");Good catch. I should have noticed this. The deleted comment mentions that
the system/temp tables normally aren't very interesting from a permissions
perspective, so perhaps there is an argument for always excluding temp
tables without a pattern. After all, \dp always excludes indexes and TOAST
tables. However, it looks like \dt includes temp tables, and I didn't see
any other meta-commands that excluded them.
It might be true that temp tables aren't usually interesting from a
permissions point of view, but it's not hard to imagine situations
where interesting things do happen. It's also probably the case that
most users won't have many temp tables, so I don't think including
them by default will be particularly intrusive.
Also, from a user perspective, I think it would be something of a POLA
violation for \dp[S] and \dt[S] to include different sets of tables,
though I appreciate that we do that now. There's nothing in the docs
to indicate that that's the case.
Anyway, I've pushed the v2 patch as-is. If anyone feels strongly
enough that we should change its behaviour for temp tables, then we
can still discuss that.
Regards,
Dean
On Sat, Jan 07, 2023 at 11:18:59AM +0000, Dean Rasheed wrote:
It might be true that temp tables aren't usually interesting from a
permissions point of view, but it's not hard to imagine situations
where interesting things do happen. It's also probably the case that
most users won't have many temp tables, so I don't think including
them by default will be particularly intrusive.Also, from a user perspective, I think it would be something of a POLA
violation for \dp[S] and \dt[S] to include different sets of tables,
though I appreciate that we do that now. There's nothing in the docs
to indicate that that's the case.
Agreed.
Anyway, I've pushed the v2 patch as-is. If anyone feels strongly
enough that we should change its behaviour for temp tables, then we
can still discuss that.
Thanks!
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Hi,
Thank you for developing a good feature.
I found while testing PostgreSQL 16 Beta 1 that the output of the \? metacommand did not include \dS, \dpS.
The attached patch changes the output of the \? meta command to:
Current output
psql=# \?
\z [PATTERN] same as \dp
\dp [PATTERN] list table, view, and sequence access privileges
Patched output
psql=# \?
\dp[S] [PATTERN] list table, view, and sequence access privileges
\z[S] [PATTERN] same as \dp
Regards,
Noriyoshi Shinoda
-----Original Message-----
From: Nathan Bossart <nathandbossart@gmail.com>
Sent: Tuesday, January 10, 2023 2:46 AM
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Maxim Orlov <orlovmg@gmail.com>; Andrew Dunstan <andrew@dunslane.net>; Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Isaac Morland <isaac.morland@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; Pavel Luzanov <p.luzanov@postgrespro.ru>; pgsql-hackers@postgresql.org
Subject: Re: add \dpS to psql
On Sat, Jan 07, 2023 at 11:18:59AM +0000, Dean Rasheed wrote:
It might be true that temp tables aren't usually interesting from a
permissions point of view, but it's not hard to imagine situations
where interesting things do happen. It's also probably the case that
most users won't have many temp tables, so I don't think including
them by default will be particularly intrusive.Also, from a user perspective, I think it would be something of a POLA
violation for \dp[S] and \dt[S] to include different sets of tables,
though I appreciate that we do that now. There's nothing in the docs
to indicate that that's the case.
Agreed.
Anyway, I've pushed the v2 patch as-is. If anyone feels strongly
enough that we should change its behaviour for temp tables, then we
can still discuss that.
Thanks!
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
psql_dpS_metacommand_v1.diffapplication/octet-stream; name=psql_dpS_metacommand_v1.diffDownload
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index c6478ed559..0ff595e7ee 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -277,7 +277,7 @@ slashUsage(unsigned short int pager)
HELP0(" \\do[S+] [OPPTRN [TYPEPTRN [TYPEPTRN]]]\n"
" list operators\n");
HELP0(" \\dO[S+] [PATTERN] list collations\n");
- HELP0(" \\dp [PATTERN] list table, view, and sequence access privileges\n");
+ HELP0(" \\dp[S] [PATTERN] list table, view, and sequence access privileges\n");
HELP0(" \\dP[itn+] [PATTERN] list [only index/table] partitioned relations [n=nested]\n");
HELP0(" \\drds [ROLEPTRN [DBPTRN]] list per-database role settings\n");
HELP0(" \\dRp[+] [PATTERN] list replication publications\n");
@@ -293,7 +293,7 @@ slashUsage(unsigned short int pager)
HELP0(" \\l[+] [PATTERN] list databases\n");
HELP0(" \\sf[+] FUNCNAME show a function's definition\n");
HELP0(" \\sv[+] VIEWNAME show a view's definition\n");
- HELP0(" \\z [PATTERN] same as \\dp\n");
+ HELP0(" \\z[S] [PATTERN] same as \\dp\n");
HELP0("\n");
HELP0("Large Objects\n");
On Thu, Jun 29, 2023 at 02:11:43AM +0000, Shinoda, Noriyoshi (PN Japan FSIP) wrote:
I found while testing PostgreSQL 16 Beta 1 that the output of the \? metacommand did not include \dS, \dpS.
The attached patch changes the output of the \? meta command to:
Thanks for the report! I've committed your patch.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com