allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

On Thu, 2022-12-08 at 10:37 -0800, Nathan Bossart wrote:

0001 makes it possible to
grant CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX. 0002 adds
predefined roles that allow performing these commands on all
relations.

Regarding the pg_refresh_all_matview predefined role, I don't think
it's a good idea. Refreshing a materialized view doesn't seem like an
administrative action to me.

First, it's unbounded in time, so the admin would need to be careful to
have a timeout. Second, the freshness of a materialized view seems very
specific to the application, rather than something that an admin would
have a blanket policy about. Thirdly, there's not a lot of information
the admin could use to make decisions about when to refresh (as opposed
to VACUUM/CLUSTER/REINDEX, where the stats are helpful).

But I'm fine with having a grantable privilege to refresh a
materialized view.

It seems like the discussion on VACUUM/CLUSTER/REINDEX privileges is
happening in the other thread. What would you like to accomplish in
this thread?

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Sat, Dec 10, 2022 at 12:07:12PM -0800, Jeff Davis wrote:

It seems like the discussion on VACUUM/CLUSTER/REINDEX privileges is
happening in the other thread. What would you like to accomplish in
this thread?

Given the feedback in the other thread [0]/messages/by-id/20221206193606.GB3078082@nathanxps13, I was planning to rewrite this
patch to create a MAINTAIN privilege and a pg_maintain_all_tables
predefined role that allowed VACUUM, ANALYZE, CLUSTER, REFRESH MATERIALIZED
VIEW, and REINDEX.

[0]: /messages/by-id/20221206193606.GB3078082@nathanxps13

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Sat, Dec 10, 2022 at 12:41:09PM -0800, Nathan Bossart wrote:

On Sat, Dec 10, 2022 at 12:07:12PM -0800, Jeff Davis wrote:

It seems like the discussion on VACUUM/CLUSTER/REINDEX privileges is
happening in the other thread. What would you like to accomplish in
this thread?

Given the feedback in the other thread [0], I was planning to rewrite this
patch to create a MAINTAIN privilege and a pg_maintain_all_tables
predefined role that allowed VACUUM, ANALYZE, CLUSTER, REFRESH MATERIALIZED
VIEW, and REINDEX.

Patch attached. I ended up reverting some parts of the VACUUM/ANALYZE
patch that were no longer needed (i.e., if the user doesn't have permission
to VACUUM, we don't need to separately check whether the user has
permission to ANALYZE). Otherwise, I don't think there's anything
tremendously different between v1 and v2 besides the fact that all the
privileges are grouped together.

Since there are only 15 privilege bits used after this patch is applied,
presumably we could revert widening AclMode to 64 bits. However, I imagine
that will still be necessary at some point in the near future, so I don't
see a strong reason to revert it.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, Dec 12, 2022 at 12:04:27PM -0800, Nathan Bossart wrote:

Patch attached. I ended up reverting some parts of the VACUUM/ANALYZE
patch that were no longer needed (i.e., if the user doesn't have permission
to VACUUM, we don't need to separately check whether the user has
permission to ANALYZE). Otherwise, I don't think there's anything
tremendously different between v1 and v2 besides the fact that all the
privileges are grouped together.

Here is a v3 of the patch that fixes a typo in the docs.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, 2022-12-12 at 13:01 -0800, Nathan Bossart wrote:

On Mon, Dec 12, 2022 at 12:04:27PM -0800, Nathan Bossart wrote:

Patch attached.  I ended up reverting some parts of the
VACUUM/ANALYZE
patch that were no longer needed (i.e., if the user doesn't have
permission
to VACUUM, we don't need to separately check whether the user has
permission to ANALYZE).  Otherwise, I don't think there's anything
tremendously different between v1 and v2 besides the fact that all
the
privileges are grouped together.

Here is a v3 of the patch that fixes a typo in the docs.

Committed.

The only significant change is that it also allows LOCK TABLE if you
have the MAINTAIN privilege.

I noticed a couple issues unrelated to your patch, and started separate
patch threads[1]/messages/by-id/c0a85c2e83158560314b576b6241c8ed0aea1745.camel@j-davis.com[2]/messages/by-id/9550c76535404a83156252b25a11babb4792ea1e.camel@j-davis.com.

[1]: /messages/by-id/c0a85c2e83158560314b576b6241c8ed0aea1745.camel@j-davis.com
/messages/by-id/c0a85c2e83158560314b576b6241c8ed0aea1745.camel@j-davis.com
[2]: /messages/by-id/9550c76535404a83156252b25a11babb4792ea1e.camel@j-davis.com
/messages/by-id/9550c76535404a83156252b25a11babb4792ea1e.camel@j-davis.com

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Tue, Dec 13, 2022 at 07:05:10PM -0800, Jeff Davis wrote:

Committed.

The only significant change is that it also allows LOCK TABLE if you
have the MAINTAIN privilege.

Thanks!

I noticed a couple issues unrelated to your patch, and started separate
patch threads[1][2].

Will take a look.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

After a fresh install, including the patch for \dpS [1]/messages/by-id/20221206193606.GB3078082@nathanxps13,
I found that granting MAINTAIN privilege does not allow the TOAST table
to be vacuumed.

postgres@postgres(16.0)=# GRANT MAINTAIN ON pg_type TO alice;
GRANT
postgres@postgres(16.0)=# \c - alice
You are now connected to database "postgres" as user "alice".
alice@postgres(16.0)=> \dpS pg_type
                                    Access privileges
   Schema   |  Name   | Type  |     Access privileges      | Column
privileges | Policies
------------+---------+-------+----------------------------+-------------------+----------
 pg_catalog | pg_type | table |
postgres=arwdDxtm/postgres+|                   |
            |         |       | =r/postgres +|                   |
            |         |       | alice=m/postgres |                   |
(1 row)

So, the patch for \dpS works as expected and can be committed.

alice@postgres(16.0)=> VACUUM pg_type;
WARNING:  permission denied to vacuum "pg_toast_1247", skipping it
VACUUM

[1]: /messages/by-id/20221206193606.GB3078082@nathanxps13
/messages/by-id/20221206193606.GB3078082@nathanxps13

--
Pavel Luzanov
Postgres Professional: https://postgrespro.com

On Wed, Dec 14, 2022 at 12:07:13PM +0300, Pavel Luzanov wrote:

I found that granting MAINTAIN privilege does not allow the TOAST table to
be vacuumed.

Hm. My first thought is that this is the appropriate behavior. WDYT?

So, the patch for \dpS works as expected and can be committed.

Thanks for reviewing.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 2022-Dec-14, Nathan Bossart wrote:

On Wed, Dec 14, 2022 at 12:07:13PM +0300, Pavel Luzanov wrote:

I found that granting MAINTAIN privilege does not allow the TOAST table to
be vacuumed.

Hm. My first thought is that this is the appropriate behavior. WDYT?

It seems wrong to me. If you can vacuum a table, surely you can also
vacuum its toast table. If you can vacuum all normal tables, you should
be able to vacuum all toast tables too.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)

On Wed, Dec 14, 2022 at 07:05:34PM +0100, Alvaro Herrera wrote:

On 2022-Dec-14, Nathan Bossart wrote:

On Wed, Dec 14, 2022 at 12:07:13PM +0300, Pavel Luzanov wrote:

I found that granting MAINTAIN privilege does not allow the TOAST table to
be vacuumed.

Hm. My first thought is that this is the appropriate behavior. WDYT?

It seems wrong to me. If you can vacuum a table, surely you can also
vacuum its toast table. If you can vacuum all normal tables, you should
be able to vacuum all toast tables too.

Okay. Should all the privileges governed by MAINTAIN apply to a relation's
TOAST table as well? I would think so, but I don't want to assume too much
before writing the patch.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, 2022-12-14 at 10:16 -0800, Nathan Bossart wrote:

Okay.  Should all the privileges governed by MAINTAIN apply to a
relation's
TOAST table as well?

Yes, I agree.

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Wed, 2022-12-14 at 12:07 +0300, Pavel Luzanov wrote:

After a fresh install, including the patch for \dpS [1],
I found that granting MAINTAIN privilege does not allow the TOAST
table
to be vacuumed.

I wanted to also mention partitioning. The behavior is that MAINTAIN
privileges on the partitioned table does not imply MAINTAIN privileges
on the partitions. I believe that's fine and it's consistent with other
privileges on partitioned tables, such as SELECT and INSERT. In the
case of an admin maintaining users' tables, they'd be a member of
pg_maintain anyway.

Furthermore, MAINTAIN privileges on the partitioned table do not grant
the ability to create new partitions. There's a comment in tablecmds.c
alluding to a possible "UNDER" privilege:

/*
* We should have an UNDER permission flag for this, but for now,
* demand that creator of a child table own the parent.
*/

Perhaps there's something we want to do there, but it's a different use
case than the MAINTAIN privilege, so I don't see a reason it should be
grouped. Also, there's a bit of weirdness to think about in cases where
another user creates (and owns) a partition of your table (currently
this is only possible if the other user is a superuser).

I am not suggesting a change here, just posting in case someone has a
different opinion.

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Wed, 14 Dec 2022 at 14:47, Jeff Davis <pgsql@j-davis.com> wrote:

Furthermore, MAINTAIN privileges on the partitioned table do not grant

the ability to create new partitions. There's a comment in tablecmds.c
alluding to a possible "UNDER" privilege:

/*
* We should have an UNDER permission flag for this, but for now,
* demand that creator of a child table own the parent.
*/

Perhaps there's something we want to do there, but it's a different use
case than the MAINTAIN privilege, so I don't see a reason it should be
grouped. Also, there's a bit of weirdness to think about in cases where
another user creates (and owns) a partition of your table (currently
this is only possible if the other user is a superuser).

I strongly agree. MAINTAIN is for actions that leave the schema the same.
Conceptually, running MAINTAIN shouldn't affect the result of pg_dump. That
may not be strictly true, but adding a table is definitely not something
that MAINTAIN should allow.

Is there a firm decision on the issue of changing the cluster index of a
table? Re-clustering a table on the same index is clearly something that
should be granted by MAINTAIN as I imagine it, but changing the cluster
index, strictly speaking, changes the schema and could be considered
outside of the scope of what should be allowed. On the other hand, I can
see simplicity in having CLUSTER check the same permissions whether or not
the cluster index is being updated.

On Wed, 2022-12-14 at 15:32 -0500, Isaac Morland wrote:

Is there a firm decision on the issue of changing the cluster index
of a table? Re-clustering a table on the same index is clearly
something that should be granted by MAINTAIN as I imagine it, but
changing the cluster index, strictly speaking, changes the schema and
could be considered outside of the scope of what should be allowed.
On the other hand, I can see simplicity in having CLUSTER check the
same permissions whether or not the cluster index is being updated.

In both the case of CLUSTER and REFRESH, I don't have a strong enough
opinion to break them out into separate privileges. There's some
argument that can be made; but at the same time it's hard for me to
imagine someone really making use of the privileges separately.

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On 14.12.2022 22:46, Jeff Davis wrote:

The behavior is that MAINTAIN
privileges on the partitioned table does not imply MAINTAIN privileges
on the partitions. I believe that's fine and it's consistent with other
privileges on partitioned tables, such as SELECT and INSERT.

Sorry, I may have missed something, but here's what I see:

postgres@postgres(16.0)=# create table p (id int) partition by list (id);
postgres@postgres(16.0)=# create table p1 partition of p for values in (1);
postgres@postgres(16.0)=# create table p2 partition of p for values in (2);

postgres@postgres(16.0)=# grant select, insert, maintain on p to alice ;

postgres@postgres(16.0)=# \c - alice
You are now connected to database "postgres" as user "alice".

alice@postgres(16.0)=> insert into p values (1);
INSERT 0 1
alice@postgres(16.0)=> select * from p;
 id
----
  1
(1 row)

alice@postgres(16.0)=> vacuum p;
WARNING:  permission denied to vacuum "p1", skipping it
WARNING:  permission denied to vacuum "p2", skipping it
VACUUM

--
Pavel Luzanov
Postgres Professional: https://postgrespro.com

On Thu, Dec 15, 2022 at 01:02:39AM +0300, Pavel Luzanov wrote:

On 14.12.2022 22:46, Jeff Davis wrote:

The behavior is that MAINTAIN
privileges on the partitioned table does not imply MAINTAIN privileges
on the partitions. I believe that's fine and it's consistent with other
privileges on partitioned tables, such as SELECT and INSERT.

Sorry, I may have missed something, but here's what I see:

postgres@postgres(16.0)=# create table p (id int) partition by list (id);
postgres@postgres(16.0)=# create table p1 partition of p for values in (1);
postgres@postgres(16.0)=# create table p2 partition of p for values in (2);

postgres@postgres(16.0)=# grant select, insert, maintain on p to alice ;

postgres@postgres(16.0)=# \c - alice
You are now connected to database "postgres" as user "alice".

alice@postgres(16.0)=> insert into p values (1);
INSERT 0 1
alice@postgres(16.0)=> select * from p;
�id
----
� 1
(1 row)

alice@postgres(16.0)=> vacuum p;
WARNING:� permission denied to vacuum "p1", skipping it
WARNING:� permission denied to vacuum "p2", skipping it
VACUUM

Yeah, but:

regression=> insert into p1 values (1);
ERROR: permission denied for table p1
regression=> select * from p1;
ERROR: permission denied for table p1

On Wed, 14 Dec 2022 at 15:57, Jeff Davis <pgsql@j-davis.com> wrote:

On Wed, 2022-12-14 at 15:32 -0500, Isaac Morland wrote:

Is there a firm decision on the issue of changing the cluster index
of a table? Re-clustering a table on the same index is clearly
something that should be granted by MAINTAIN as I imagine it, but
changing the cluster index, strictly speaking, changes the schema and
could be considered outside of the scope of what should be allowed.
On the other hand, I can see simplicity in having CLUSTER check the
same permissions whether or not the cluster index is being updated.

In both the case of CLUSTER and REFRESH, I don't have a strong enough
opinion to break them out into separate privileges. There's some
argument that can be made; but at the same time it's hard for me to
imagine someone really making use of the privileges separately.

Thanks, that makes a lot of sense. I wanted to make sure the question was
considered. I'm very pleased this is happening and appreciate all the work
you're doing. I have a few places where I want to be able to grant MAINTAIN
so I'll be using this as soon as it's available on our production database.

On Wed, Dec 14, 2022 at 11:05:13AM -0800, Jeff Davis wrote:

On Wed, 2022-12-14 at 10:16 -0800, Nathan Bossart wrote:

Okay.� Should all the privileges governed by MAINTAIN apply to a
relation's
TOAST table as well?

Yes, I agree.

This might be tricky, because AFAICT you have to scan pg_class to find a
TOAST table's main relation.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Dec 14, 2022 at 03:29:39PM -0800, Nathan Bossart wrote:

On Wed, Dec 14, 2022 at 11:05:13AM -0800, Jeff Davis wrote:

On Wed, 2022-12-14 at 10:16 -0800, Nathan Bossart wrote:

Okay.  Should all the privileges governed by MAINTAIN apply to a
relation's
TOAST table as well?

Yes, I agree.

This might be tricky, because AFAICT you have to scan pg_class to find a
TOAST table's main relation.

Ugh, yeah. Are we talking about a case where we know the toast
information but need to look back at some information of its parent to
do a decision? I don't recall a case where we do that. CLUSTER,
REINDEX and VACUUM lock first the parent when working on it, and no
AEL is taken on the parent if doing directly a VACUUM or a REINDEX on
the toast table, so that could lead to deadlock scenarios. Shouldn't
MAINTAIN be sent down to the toast table as well if that's not done
this way?

FWIW, I have briefly poked at that here:
/messages/by-id/YZI+aNEnnpBASxNU@paquier.xyz
--
Michael