add \dpS to psql

Started by Nathan Bossartover 3 years ago28 messageshackers
Jump to latest
#1Nathan Bossart
nathandbossart@gmail.com

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+12-13
#2Pavel Luzanov
p.luzanov@postgrespro.ru
In reply to: Nathan Bossart (#1)
Re: add \dpS to psql

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

#3Nathan Bossart
nathandbossart@gmail.com
In reply to: Pavel Luzanov (#2)
Re: add \dpS to psql

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

#4Justin Pryzby
pryzby@telsasoft.com
In reply to: Pavel Luzanov (#2)
Re: add \dpS to psql

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.html

Something 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

#5Nathan Bossart
nathandbossart@gmail.com
In reply to: Justin Pryzby (#4)
Re: add \dpS to psql

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

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Nathan Bossart (#5)
Re: add \dpS to psql

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

#7Nathan Bossart
nathandbossart@gmail.com
In reply to: Tom Lane (#6)
Re: add \dpS to psql

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

#8Isaac Morland
isaac.morland@gmail.com
In reply to: Tom Lane (#6)
Re: add \dpS to psql

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.

#9Nathan Bossart
nathandbossart@gmail.com
In reply to: Isaac Morland (#8)
Re: add \dpS to psql

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

#10Isaac Morland
isaac.morland@gmail.com
In reply to: Nathan Bossart (#9)
Re: add \dpS to psql

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

#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Nathan Bossart (#9)
Re: add \dpS to psql

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

#12Pavel Luzanov
p.luzanov@postgrespro.ru
In reply to: Isaac Morland (#8)
Re: add \dpS to psql

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

#13Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Pavel Luzanov (#12)
Re: add \dpS to psql

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/

#14Nathan Bossart
nathandbossart@gmail.com
In reply to: Tom Lane (#11)
Re: add \dpS to psql

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

#15Nathan Bossart
nathandbossart@gmail.com
In reply to: Nathan Bossart (#14)
Re: add \dpS to psql

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

#16Michael Paquier
michael@paquier.xyz
In reply to: Nathan Bossart (#14)
Re: add \dpS to psql

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

#17Nathan Bossart
nathandbossart@gmail.com
In reply to: Michael Paquier (#16)
Re: add \dpS to psql

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

#18Nathan Bossart
nathandbossart@gmail.com
In reply to: Nathan Bossart (#17)
Re: add \dpS to psql

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

#19Andrew Dunstan
andrew@dunslane.net
In reply to: Nathan Bossart (#18)
Re: add \dpS to psql

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

#20Nathan Bossart
nathandbossart@gmail.com
In reply to: Andrew Dunstan (#19)
Re: add \dpS to psql

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

#21Maxim Orlov
orlovmg@gmail.com
In reply to: Nathan Bossart (#20)
#22Nathan Bossart
nathandbossart@gmail.com
In reply to: Maxim Orlov (#21)
#23Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Nathan Bossart (#22)
#24Nathan Bossart
nathandbossart@gmail.com
In reply to: Dean Rasheed (#23)
#25Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Nathan Bossart (#24)
#26Nathan Bossart
nathandbossart@gmail.com
In reply to: Dean Rasheed (#25)
#27Shinoda, Noriyoshi (PN Japan FSIP)
noriyoshi.shinoda@hpe.com
In reply to: Nathan Bossart (#26)
#28Nathan Bossart
nathandbossart@gmail.com
In reply to: Shinoda, Noriyoshi (PN Japan FSIP) (#27)