public inbox for [email protected]
help / color / mirror / Atom feedallow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
39+ messages / 8 participants
[nested] [flat]
* allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-08 18:37 Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2022-12-08 18:37 UTC (permalink / raw)
To: pgsql-hackers
Hi hackers,
This is meant as a continuation of the work to make VACUUM and ANALYZE
grantable privileges [0]. As noted there, the primary motivation for this
is to continue chipping away at things that require special privileges or
even superuser. I've attached two patches. 0001 makes it possible to
grant CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX. 0002 adds
predefined roles that allow performing these commands on all relations.
After applying these patches, there are 13 privilege bits remaining for
future use.
There is an ongoing discussion in another thread [1] about how these
privileges should be divvied up. Should each command get it's own
privilege bit (as I've done in the attached patches), or should the
privileges be grouped in some fashion (e.g., adding a MAINTAIN bit that
governs all of them, splitting out exclusive-lock operations from
non-exclusive-lock ones)?
Most of the changes in the attached patches are rather mechanical, and like
VACUUM/ANALYZE, there is room for future enhancement, such as granting the
privileges on databases/schemas instead of just tables.
[0] https://postgr.es/m/20220722203735.GB3996698%40nathanxps13
[1] https://postgr.es/m/20221206193606.GB3078082%40nathanxps13
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-10 20:07 Jeff Davis <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Jeff Davis @ 2022-12-10 20:07 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-10 20:41 Nathan Bossart <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2022-12-10 20:41 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers
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.
[0] https://postgr.es/m/20221206193606.GB3078082%40nathanxps13
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-12 20:04 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2022-12-12 20:04 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-12 21:01 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2022-12-12 21:01 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 03:05 Jeff Davis <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Jeff Davis @ 2022-12-14 03:05 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers
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][2].
[1]
https://www.postgresql.org/message-id/[email protected]
[2]
https://www.postgresql.org/message-id/[email protected]
--
Jeff Davis
PostgreSQL Contributor Team - AWS
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 03:23 Nathan Bossart <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2022-12-14 03:23 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 09:07 Pavel Luzanov <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 2 replies; 39+ messages in thread
From: Pavel Luzanov @ 2022-12-14 09:07 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; Jeff Davis <[email protected]>; +Cc: pgsql-hackers
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.
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]
https://www.postgresql.org/message-id/20221206193606.GB3078082%40nathanxps13
--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 17:13 Nathan Bossart <[email protected]>
parent: Pavel Luzanov <[email protected]>
1 sibling, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2022-12-14 17:13 UTC (permalink / raw)
To: Pavel Luzanov <[email protected]>; +Cc: Jeff Davis <[email protected]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 18:05 Alvaro Herrera <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Alvaro Herrera @ 2022-12-14 18:05 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Pavel Luzanov <[email protected]>; Jeff Davis <[email protected]>; pgsql-hackers
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)
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 18:16 Nathan Bossart <[email protected]>
parent: Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2022-12-14 18:16 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Pavel Luzanov <[email protected]>; Jeff Davis <[email protected]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 19:05 Jeff Davis <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Jeff Davis @ 2022-12-14 19:05 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; Alvaro Herrera <[email protected]>; +Cc: Pavel Luzanov <[email protected]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 19:46 Jeff Davis <[email protected]>
parent: Pavel Luzanov <[email protected]>
1 sibling, 2 replies; 39+ messages in thread
From: Jeff Davis @ 2022-12-14 19:46 UTC (permalink / raw)
To: Pavel Luzanov <[email protected]>; Nathan Bossart <[email protected]>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 20:32 Isaac Morland <[email protected]>
parent: Jeff Davis <[email protected]>
1 sibling, 1 reply; 39+ messages in thread
From: Isaac Morland @ 2022-12-14 20:32 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Pavel Luzanov <[email protected]>; Nathan Bossart <[email protected]>; pgsql-hackers
On Wed, 14 Dec 2022 at 14:47, Jeff Davis <[email protected]> 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.
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 20:56 Jeff Davis <[email protected]>
parent: Isaac Morland <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Jeff Davis @ 2022-12-14 20:56 UTC (permalink / raw)
To: Isaac Morland <[email protected]>; +Cc: Pavel Luzanov <[email protected]>; Nathan Bossart <[email protected]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 22:02 Pavel Luzanov <[email protected]>
parent: Jeff Davis <[email protected]>
1 sibling, 1 reply; 39+ messages in thread
From: Pavel Luzanov @ 2022-12-14 22:02 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; Nathan Bossart <[email protected]>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 22:11 Justin Pryzby <[email protected]>
parent: Pavel Luzanov <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Justin Pryzby @ 2022-12-14 22:11 UTC (permalink / raw)
To: Pavel Luzanov <[email protected]>; +Cc: Jeff Davis <[email protected]>; Nathan Bossart <[email protected]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 22:40 Isaac Morland <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 0 replies; 39+ messages in thread
From: Isaac Morland @ 2022-12-14 22:40 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Pavel Luzanov <[email protected]>; Nathan Bossart <[email protected]>; pgsql-hackers
On Wed, 14 Dec 2022 at 15:57, Jeff Davis <[email protected]> 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.
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-14 23:29 Nathan Bossart <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2022-12-14 23:29 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Pavel Luzanov <[email protected]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-15 00:12 Michael Paquier <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Michael Paquier @ 2022-12-15 00:12 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Jeff Davis <[email protected]>; Alvaro Herrera <[email protected]>; Pavel Luzanov <[email protected]>; pgsql-hackers
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:
https://www.postgresql.org/message-id/[email protected]
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-15 00:18 Jeff Davis <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Jeff Davis @ 2022-12-15 00:18 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; Pavel Luzanov <[email protected]>; +Cc: Nathan Bossart <[email protected]>; pgsql-hackers
On Wed, 2022-12-14 at 16:11 -0600, Justin Pryzby wrote:
> Yeah, but:
>
> regression=> insert into p1 values (1);
> ERROR: permission denied for table p1
> regression=> select * from p1;
> ERROR: permission denied for table p1
Right, that's what I had in mind: a user is only granted operations on
the partitioned table, not the partitions.
It happens that an INSERT or SELECT on the partitioned table flows
through to the partitions, whereas the VACUUM ends up skipping them, so
I guess the analogy could be interpreted either way. Hmmm...
Thinking about it another way: logical partitioning is about making the
table logically one table, but physically many tables. That would imply
that the privileges should apply per-partition. But then that doesn't
make a lot of sense, because what maintenance can you do on the
partitioned table (which itself has no data)?
There's definitely a problem with this patch and partitioning, because
REINDEX affects the partitions, CLUSTER is a no-op, and VACUUM/ANALYZE
skip them.
--
Jeff Davis
PostgreSQL Contributor Team - AWS
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-15 00:27 Nathan Bossart <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 2 replies; 39+ messages in thread
From: Nathan Bossart @ 2022-12-15 00:27 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Jeff Davis <[email protected]>; Alvaro Herrera <[email protected]>; Pavel Luzanov <[email protected]>; pgsql-hackers
On Thu, Dec 15, 2022 at 09:12:26AM +0900, Michael Paquier wrote:
> 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?
Another option I'm looking at is skipping the privilege checks when VACUUM
recurses to a TOAST table. This won't allow you to VACUUM the TOAST table
directly, but it would at least address the originally-reported issue [0].
Since you can't ANALYZE, REFRESH, or LOCK TOAST tables, this isn't a
problem for those commands. CLUSTER and REINDEX seem to process relations'
TOAST tables without extra privilege checks already. So with the attached
patch applied, you wouldn't be able to VACUUM, CLUSTER, and REINDEX TOAST
tableѕ directly (unless you were given MAINTAIN or pg_maintain), but you
could indirectly process them by specifying the main relation.
I don't know if this is good enough. It seems like ideally you should be
able to VACUUM a TOAST table directly if you have MAINTAIN on its main
relation.
[0] https://postgr.es/m/b572d238-0de2-9cad-5f34-4741dc627834%40postgrespro.ru
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] allow_vacuuming_toast.patch (5.4K, ../../20221215002705.GA889413@nathanxps13/2-allow_vacuuming_toast.patch)
download | inline diff:
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 293b84bbca..8f80e5d8c0 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -91,7 +91,8 @@ static void vac_truncate_clog(TransactionId frozenXID,
MultiXactId minMulti,
TransactionId lastSaneFrozenXid,
MultiXactId lastSaneMinMulti);
-static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params);
+static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
+ bool skip_privs);
static double compute_parallel_delay(void);
static VacOptValue get_vacoptval_from_boolean(DefElem *def);
static bool vac_tid_reaped(ItemPointer itemptr, void *state);
@@ -470,7 +471,7 @@ vacuum(List *relations, VacuumParams *params,
if (params->options & VACOPT_VACUUM)
{
- if (!vacuum_rel(vrel->oid, vrel->relation, params))
+ if (!vacuum_rel(vrel->oid, vrel->relation, params, false))
continue;
}
@@ -1806,7 +1807,7 @@ vac_truncate_clog(TransactionId frozenXID,
* At entry and exit, we are not inside a transaction.
*/
static bool
-vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
+vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs)
{
LOCKMODE lmode;
Relation rel;
@@ -1893,7 +1894,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
* happen across multiple transactions where privileges could have changed
* in-between. Make sure to only generate logs for VACUUM in this case.
*/
- if (!vacuum_is_permitted_for_relation(RelationGetRelid(rel),
+ if (!skip_privs &&
+ !vacuum_is_permitted_for_relation(RelationGetRelid(rel),
rel->rd_rel,
params->options & VACOPT_VACUUM))
{
@@ -2067,7 +2069,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
* totally unimportant for toast relations.
*/
if (toast_relid != InvalidOid)
- vacuum_rel(toast_relid, NULL, params);
+ vacuum_rel(toast_relid, NULL, params, true);
/*
* Now release the session-level lock on the main table.
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 34c26a804a..5335e45434 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -2901,6 +2901,7 @@ CREATE INDEX ON maintain_test (a);
GRANT MAINTAIN ON maintain_test TO regress_maintain;
CREATE MATERIALIZED VIEW refresh_test AS SELECT 1;
GRANT MAINTAIN ON refresh_test TO regress_maintain;
+GRANT MAINTAIN ON pg_type TO regress_maintain;
CREATE SCHEMA reindex_test;
-- negative tests; should fail
SET ROLE regress_no_maintain;
@@ -2910,6 +2911,8 @@ ANALYZE maintain_test;
WARNING: permission denied to analyze "maintain_test", skipping it
VACUUM (ANALYZE) maintain_test;
WARNING: permission denied to vacuum "maintain_test", skipping it
+VACUUM pg_type;
+WARNING: permission denied to vacuum "pg_type", skipping it
CLUSTER maintain_test USING maintain_test_a_idx;
ERROR: must be owner of table maintain_test
REFRESH MATERIALIZED VIEW refresh_test;
@@ -2933,6 +2936,7 @@ SET ROLE regress_maintain;
VACUUM maintain_test;
ANALYZE maintain_test;
VACUUM (ANALYZE) maintain_test;
+VACUUM pg_type;
CLUSTER maintain_test USING maintain_test_a_idx;
REFRESH MATERIALIZED VIEW refresh_test;
REINDEX TABLE maintain_test;
@@ -2950,6 +2954,7 @@ SET ROLE regress_maintain_all;
VACUUM maintain_test;
ANALYZE maintain_test;
VACUUM (ANALYZE) maintain_test;
+VACUUM pg_type;
CLUSTER maintain_test USING maintain_test_a_idx;
REFRESH MATERIALIZED VIEW refresh_test;
REINDEX TABLE maintain_test;
@@ -2965,6 +2970,7 @@ RESET ROLE;
DROP TABLE maintain_test;
DROP MATERIALIZED VIEW refresh_test;
DROP SCHEMA reindex_test;
+REVOKE MAINTAIN ON pg_type FROM regress_maintain;
DROP ROLE regress_no_maintain;
DROP ROLE regress_maintain;
DROP ROLE regress_maintain_all;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 39aa0b4ecf..5f36026eaa 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1872,6 +1872,7 @@ CREATE INDEX ON maintain_test (a);
GRANT MAINTAIN ON maintain_test TO regress_maintain;
CREATE MATERIALIZED VIEW refresh_test AS SELECT 1;
GRANT MAINTAIN ON refresh_test TO regress_maintain;
+GRANT MAINTAIN ON pg_type TO regress_maintain;
CREATE SCHEMA reindex_test;
-- negative tests; should fail
@@ -1879,6 +1880,7 @@ SET ROLE regress_no_maintain;
VACUUM maintain_test;
ANALYZE maintain_test;
VACUUM (ANALYZE) maintain_test;
+VACUUM pg_type;
CLUSTER maintain_test USING maintain_test_a_idx;
REFRESH MATERIALIZED VIEW refresh_test;
REINDEX TABLE maintain_test;
@@ -1896,6 +1898,7 @@ SET ROLE regress_maintain;
VACUUM maintain_test;
ANALYZE maintain_test;
VACUUM (ANALYZE) maintain_test;
+VACUUM pg_type;
CLUSTER maintain_test USING maintain_test_a_idx;
REFRESH MATERIALIZED VIEW refresh_test;
REINDEX TABLE maintain_test;
@@ -1913,6 +1916,7 @@ SET ROLE regress_maintain_all;
VACUUM maintain_test;
ANALYZE maintain_test;
VACUUM (ANALYZE) maintain_test;
+VACUUM pg_type;
CLUSTER maintain_test USING maintain_test_a_idx;
REFRESH MATERIALIZED VIEW refresh_test;
REINDEX TABLE maintain_test;
@@ -1929,6 +1933,7 @@ RESET ROLE;
DROP TABLE maintain_test;
DROP MATERIALIZED VIEW refresh_test;
DROP SCHEMA reindex_test;
+REVOKE MAINTAIN ON pg_type FROM regress_maintain;
DROP ROLE regress_no_maintain;
DROP ROLE regress_maintain;
DROP ROLE regress_maintain_all;
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-15 09:31 Pavel Luzanov <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Pavel Luzanov @ 2022-12-15 09:31 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; Justin Pryzby <[email protected]>; +Cc: Nathan Bossart <[email protected]>; pgsql-hackers
On 15.12.2022 03:18, Jeff Davis wrote:
> Right, that's what I had in mind: a user is only granted operations on
> the partitioned table, not the partitions.
It's all clear now.
> There's definitely a problem with this patch and partitioning, because
> REINDEX affects the partitions, CLUSTER is a no-op, and VACUUM/ANALYZE
> skip them.
I think the approach that Nathan implemented [1] for TOAST tables
in the latest version can be used for partitioned tables as well.
Skipping the privilege check for partitions while working with
a partitioned table. In that case we would get exactly the same behavior
as for INSERT, SELECT, etc privileges - the MAINTAIN privilege would
work for
the whole partitioned table, but not for individual partitions.
[1]
https://www.postgresql.org/message-id/20221215002705.GA889413%40nathanxps13
--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-15 09:42 Pavel Luzanov <[email protected]>
parent: Nathan Bossart <[email protected]>
1 sibling, 0 replies; 39+ messages in thread
From: Pavel Luzanov @ 2022-12-15 09:42 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; +Cc: Jeff Davis <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers
On 15.12.2022 03:27, Nathan Bossart wrote:
> Another option I'm looking at is skipping the privilege checks when VACUUM
> recurses to a TOAST table. This won't allow you to VACUUM the TOAST table
> directly, but it would at least address the originally-reported issue
This approach can be implemented for partitioned tables too. Skipping
the privilege checks when VACUUM/ANALYZE recurses to partitions.
> I don't know if this is good enough.
At least it's better than before.
> It seems like ideally you should be
> able to VACUUM a TOAST table directly if you have MAINTAIN on its main
> relation.
I agree, that would be ideally.
--
Pavel Luzanov
Postgres Professional: https://postgrespro.com
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-15 18:10 Jeff Davis <[email protected]>
parent: Pavel Luzanov <[email protected]>
0 siblings, 2 replies; 39+ messages in thread
From: Jeff Davis @ 2022-12-15 18:10 UTC (permalink / raw)
To: Pavel Luzanov <[email protected]>; Justin Pryzby <[email protected]>; +Cc: Nathan Bossart <[email protected]>; pgsql-hackers
On Thu, 2022-12-15 at 12:31 +0300, Pavel Luzanov wrote:
> I think the approach that Nathan implemented [1] for TOAST tables
> in the latest version can be used for partitioned tables as well.
> Skipping the privilege check for partitions while working with
> a partitioned table. In that case we would get exactly the same
> behavior
> as for INSERT, SELECT, etc privileges - the MAINTAIN privilege would
> work for
> the whole partitioned table, but not for individual partitions.
There is some weirdness in 15, too:
create user foo;
create user su superuser;
grant all privileges on schema public to foo;
\c - foo
create table p(i int) partition by range (i);
create index p_idx on p (i);
create table p0 partition of p for values from (0) to (10);
\c - su
create table p1 partition of p for values from (10) to (20);
\c - foo
-- possibly weird because the 15 inserts into p1 (owned by su)
insert into p values (5), (15);
-- all these are as expected:
select * from p; -- returns 5 & 15
insert into p1 values(16); -- permission denied
select * from p1; -- permission denied
-- the following commands seem inconsistent to me:
vacuum p; -- skips p1 with warning
analyze p; -- skips p1 with warning
cluster p using p_idx; -- silently skips p1
reindex table p; -- reindexes p0 and p1 (owned by su)
-- RLS is also bypassed
\c - su
grant select, insert on p1 to foo;
alter table p1 enable row level security;
create policy odd on p1 using (i % 2 = 1) with check (i % 2 = 1);
\c - foo
insert into p1 values (16); -- RLS error
insert into p values (16); -- succeeds
select * from p1; -- returns only 15
select * from p; -- returns 5, 15, 16
The proposal to skip privilege checks for partitions would be
consistent with INSERT, SELECT, REINDEX that flow through to the
underlying partitions regardless of permissions/ownership (and even
RLS). It would be very minor behavior change on 15 for this weird case
of superuser-owned partitions, but I doubt anyone would be relying on
that.
+1.
I do have some lingering doubts about whether we should even allow
inconsistent ownership/permissions. But I don't think we need to settle
that question now.
--
Jeff Davis
PostgreSQL Contributor Team - AWS
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-15 18:42 Jeff Davis <[email protected]>
parent: Nathan Bossart <[email protected]>
1 sibling, 1 reply; 39+ messages in thread
From: Jeff Davis @ 2022-12-15 18:42 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; Michael Paquier <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Pavel Luzanov <[email protected]>; pgsql-hackers
On Wed, 2022-12-14 at 16:27 -0800, Nathan Bossart wrote:
> I don't know if this is good enough. It seems like ideally you
> should be
> able to VACUUM a TOAST table directly if you have MAINTAIN on its
> main
> relation.
Right now, targetting the toast table directly requires the USAGE
privilege on the toast schema, and you have to look up the name first,
right? As it is, that's not a great UI.
How about if we add a VACUUM option like TOAST_ONLY (or combine it with
the PROCESS_TOAST option)? Then, you're always looking at the parent
table first so there's no deadlock, do the permission checks on the
parent, and then expand to the toast table with no check. This can be a
follow-up patch; for now, the idea of skipping the privilege checks
when expanding looks like an improvement.
--
Jeff Davis
PostgreSQL Contributor Team - AWS
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-15 19:12 Nathan Bossart <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 0 replies; 39+ messages in thread
From: Nathan Bossart @ 2022-12-15 19:12 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Michael Paquier <[email protected]>; Alvaro Herrera <[email protected]>; Pavel Luzanov <[email protected]>; pgsql-hackers
On Thu, Dec 15, 2022 at 10:42:15AM -0800, Jeff Davis wrote:
> Right now, targetting the toast table directly requires the USAGE
> privilege on the toast schema, and you have to look up the name first,
> right? As it is, that's not a great UI.
>
> How about if we add a VACUUM option like TOAST_ONLY (or combine it with
> the PROCESS_TOAST option)? Then, you're always looking at the parent
> table first so there's no deadlock, do the permission checks on the
> parent, and then expand to the toast table with no check. This can be a
> follow-up patch; for now, the idea of skipping the privilege checks
> when expanding looks like an improvement.
I originally suggested an option to allow specifying whether to process the
main relation, but we ended up only adding PROCESS_TOAST [0]. FWIW I still
think that such an option would be useful for the reasons you describe.
[0] https://postgr.es/m/BA8951E9-1524-48C5-94AF-73B1F0D7857F%40amazon.com
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-15 19:48 Justin Pryzby <[email protected]>
parent: Jeff Davis <[email protected]>
1 sibling, 1 reply; 39+ messages in thread
From: Justin Pryzby @ 2022-12-15 19:48 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Pavel Luzanov <[email protected]>; Nathan Bossart <[email protected]>; pgsql-hackers
On Thu, Dec 15, 2022 at 10:10:43AM -0800, Jeff Davis wrote:
> On Thu, 2022-12-15 at 12:31 +0300, Pavel Luzanov wrote:
> > I think the approach that Nathan implemented [1] for TOAST tables
> > in the latest version can be used for partitioned tables as well.
> > Skipping the privilege check for partitions while working with
> > a partitioned table. In that case we would get exactly the same
> > behavior
> > as for INSERT, SELECT, etc privileges - the MAINTAIN privilege would
> > work for
> > the whole partitioned table, but not for individual partitions.
>
> There is some weirdness in 15, too:
I gather you mean postgresql v15.1 and master ?
> -- the following commands seem inconsistent to me:
> vacuum p; -- skips p1 with warning
> analyze p; -- skips p1 with warning
> cluster p using p_idx; -- silently skips p1
> reindex table p; -- reindexes p0 and p1 (owned by su)
Clustering on a partitioned table is new in v15, and this behavior is
from 3f19e176ae0 and cfdd03f45e6, which added
get_tables_to_cluster_partitioned(), borrowing from expand_vacuum_rel()
and get_tables_to_cluster().
vacuum initially calls vacuum_is_permitted_for_relation() only for the
partitioned table, and *later* locks the partition and then checks its
permissions, which is when the message is output.
Since v15, cluster calls get_tables_to_cluster_partitioned(), which
silently discards partitions failing ACL.
We could change it to emit a message, which would seem to behave like
vacuum, except that the check is happening earlier, and (unlike vacuum)
partitions skipped later during CLUOPT_RECHECK wouldn't have any message
output.
Or we could change cluster_rel() to output a message when skipping. But
these patches hardly touched that function at all. I suppose we could
change to emit a message during RECHECK (maybe only in master branch).
If need be, that could be controlled by a new CLUOPT_*.
--
Justin
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-16 01:19 Nathan Bossart <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2022-12-16 01:19 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Jeff Davis <[email protected]>; Pavel Luzanov <[email protected]>; pgsql-hackers
On Thu, Dec 15, 2022 at 01:48:13PM -0600, Justin Pryzby wrote:
> vacuum initially calls vacuum_is_permitted_for_relation() only for the
> partitioned table, and *later* locks the partition and then checks its
> permissions, which is when the message is output.
>
> Since v15, cluster calls get_tables_to_cluster_partitioned(), which
> silently discards partitions failing ACL.
>
> We could change it to emit a message, which would seem to behave like
> vacuum, except that the check is happening earlier, and (unlike vacuum)
> partitions skipped later during CLUOPT_RECHECK wouldn't have any message
> output.
>
> Or we could change cluster_rel() to output a message when skipping. But
> these patches hardly touched that function at all. I suppose we could
> change to emit a message during RECHECK (maybe only in master branch).
> If need be, that could be controlled by a new CLUOPT_*.
Yeah, VACUUM/ANALYZE works differently. For one, you can specify multiple
relations in the command. Also, VACUUM/ANALYZE only takes an
AccessShareLock when first assessing permissions (which actually skips
partitions). CLUSTER immediately takes an AccessExclusiveLock, so the
permissions are checked up front. This is done "to avoid lock-upgrade
hazard in the single-transaction case," which IIUC is what allows CLUSTER
on a single table to run within a transaction block (unlike VACUUM). I
don't know if running CLUSTER in a transaction block is important. IMO we
should consider making CLUSTER look a lot more like VACUUM/ANALYZE in this
regard. The attached patch adds WARNING messages, but we're still far from
consistency with VACUUM/ANALYZE.
Side note: I see that CLUSTER on a partitioned table is disallowed in a
transaction block, which should probably be added to my documentation patch
[0].
[0] https://commitfest.postgresql.org/41/4054/
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] add_cluster_skip_messages.patch (4.4K, ../../20221216011926.GA771496@nathanxps13/2-add_cluster_skip_messages.patch)
download | inline diff:
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 8966b75bd1..6d09d1c639 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -80,6 +80,7 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
static List *get_tables_to_cluster(MemoryContext cluster_context);
static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
Oid indexOid);
+static bool cluster_is_permitted_for_relation(Oid relid, Oid userid);
/*---------------------------------------------------------------------------
@@ -366,8 +367,7 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
if (recheck)
{
/* Check that the user still has privileges for the relation */
- if (!object_ownercheck(RelationRelationId, tableOid, save_userid) &&
- pg_class_aclcheck(tableOid, save_userid, ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(tableOid, save_userid))
{
relation_close(OldHeap, AccessExclusiveLock);
goto out;
@@ -1646,8 +1646,7 @@ get_tables_to_cluster(MemoryContext cluster_context)
index = (Form_pg_index) GETSTRUCT(indexTuple);
- if (!object_ownercheck(RelationRelationId, index->indrelid, GetUserId()) &&
- pg_class_aclcheck(index->indrelid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(index->indrelid, GetUserId()))
continue;
/* Use a permanent memory context for the result list */
@@ -1695,11 +1694,8 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
if (get_rel_relkind(indexrelid) != RELKIND_INDEX)
continue;
- /* Silently skip partitions which the user has no access to. */
- if (!object_ownercheck(RelationRelationId, relid, GetUserId()) &&
- pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
- (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) ||
- IsSharedRelation(relid)))
+ /* skip partitions which the user has no access to */
+ if (!cluster_is_permitted_for_relation(relid, GetUserId()))
continue;
/* Use a permanent memory context for the result list */
@@ -1715,3 +1711,21 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
return rtcs;
}
+
+/*
+ * Check if the current user has privileges to cluster the relation. If not,
+ * issue a WARNING log message and return false to let the caller decide what
+ * to do with this relation.
+ */
+static bool
+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+ if (object_ownercheck(RelationRelationId, relid, userid) ||
+ pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK)
+ return true;
+
+ ereport(WARNING,
+ (errmsg("permission denied to cluster \"%s\", skipping it",
+ get_rel_name(relid))));
+ return false;
+}
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 542c2e098c..830b306907 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -352,7 +352,11 @@ INSERT INTO clstr_3 VALUES (1);
-- this user can only cluster clstr_1 and clstr_3, but the latter
-- has not been clustered
SET SESSION AUTHORIZATION regress_clstr_user;
-CLUSTER;
+CLUSTER clstr_1;
+CLUSTER clstr_2;
+ERROR: must be owner of table clstr_2
+CLUSTER clstr_3;
+ERROR: there is no previously clustered index for table "clstr_3"
SELECT * FROM clstr_1 UNION ALL
SELECT * FROM clstr_2 UNION ALL
SELECT * FROM clstr_3;
@@ -506,6 +510,7 @@ CREATE TEMP TABLE ptnowner_oldnodes AS
JOIN pg_class AS c ON c.oid=tree.relid;
SET SESSION AUTHORIZATION regress_ptnowner;
CLUSTER ptnowner USING ptnowner_i_idx;
+WARNING: permission denied to cluster "ptnowner2", skipping it
RESET SESSION AUTHORIZATION;
SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a
JOIN ptnowner_oldnodes b USING (oid) ORDER BY a.relname COLLATE "C";
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 6cb9c926c0..43d44238ac 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -145,7 +145,9 @@ INSERT INTO clstr_3 VALUES (1);
-- this user can only cluster clstr_1 and clstr_3, but the latter
-- has not been clustered
SET SESSION AUTHORIZATION regress_clstr_user;
-CLUSTER;
+CLUSTER clstr_1;
+CLUSTER clstr_2;
+CLUSTER clstr_3;
SELECT * FROM clstr_1 UNION ALL
SELECT * FROM clstr_2 UNION ALL
SELECT * FROM clstr_3;
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-16 04:35 Jeff Davis <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Jeff Davis @ 2022-12-16 04:35 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; Justin Pryzby <[email protected]>; +Cc: Pavel Luzanov <[email protected]>; pgsql-hackers
On Thu, 2022-12-15 at 17:19 -0800, Nathan Bossart wrote:
> The attached patch adds WARNING messages, but we're still far from
> consistency with VACUUM/ANALYZE.
But why make CLUSTER more like VACUUM? Shouldn't we make
VACUUM/CLUSTER/ANALYZE more like INSERT/SELECT/REINDEX?
As far as I can tell, the only way you can get in this situation in 15
is by having a superuser create partitions in a non-superuser's table.
I don't think that was an intended use case, so it's probably just
historical reasons that VACUUM/CLUSTER/ANALYZE ended up that way, not
precedent we should necessarily follow.
--
Jeff Davis
PostgreSQL Contributor Team - AWS
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-16 05:13 Nathan Bossart <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2022-12-16 05:13 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Pavel Luzanov <[email protected]>; pgsql-hackers
On Thu, Dec 15, 2022 at 08:35:53PM -0800, Jeff Davis wrote:
> But why make CLUSTER more like VACUUM? Shouldn't we make
> VACUUM/CLUSTER/ANALYZE more like INSERT/SELECT/REINDEX?
Hm. Since VACUUM may happen across multiple transactions, it is careful to
re-check the privileges for each relation. For example, vacuum_rel()
contains this comment:
/*
* Check if relation needs to be skipped based on privileges. This check
* happens also when building the relation list to vacuum for a manual
* operation, and needs to be done additionally here as VACUUM could
* happen across multiple transactions where privileges could have changed
* in-between. Make sure to only generate logs for VACUUM in this case.
*/
I do wonder whether this is something we really need to be concerned about.
In the worst case, you might be able to VACUUM a table with a concurrent
owner change.
The logic for gathering all relations to process (i.e.,
get_all_vacuum_rels() and get_tables_to_cluster()) would also need to be
adjusted to handle partitioned tables. Right now, we gather all the
partitions and checks privileges on each. I think this would be easy to
change.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-16 05:20 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 0 replies; 39+ messages in thread
From: Nathan Bossart @ 2022-12-16 05:20 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Pavel Luzanov <[email protected]>; pgsql-hackers
On Thu, Dec 15, 2022 at 09:13:54PM -0800, Nathan Bossart wrote:
> I do wonder whether this is something we really need to be concerned about.
> In the worst case, you might be able to VACUUM a table with a concurrent
> owner change.
I suppose we might want to avoid running the index functions as the new
owner.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-17 06:04 Nathan Bossart <[email protected]>
parent: Jeff Davis <[email protected]>
1 sibling, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2022-12-17 06:04 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Pavel Luzanov <[email protected]>; Justin Pryzby <[email protected]>; pgsql-hackers
On Thu, Dec 15, 2022 at 10:10:43AM -0800, Jeff Davis wrote:
> The proposal to skip privilege checks for partitions would be
> consistent with INSERT, SELECT, REINDEX that flow through to the
> underlying partitions regardless of permissions/ownership (and even
> RLS). It would be very minor behavior change on 15 for this weird case
> of superuser-owned partitions, but I doubt anyone would be relying on
> that.
I've attached a work-in-progress patch that aims to accomplish this.
Instead of skipping the privilege checks, I added logic to trawl through
pg_inherits and pg_class to check whether the user has privileges for the
partitioned table or for the main relation of a TOAST table. This means
that MAINTAIN on a partitioned table is enough to execute maintenance
commands on all the partitions, and MAINTAIN on a main relation is enough
to execute maintenance commands on its TOAST table. Also, the maintenance
commands that flow through to the partitions or the TOAST table should no
longer error due to permissions when the user only has MAINTAIN on the
paritioned table or main relation.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] fix_partition_and_toast_privs.patch (14.8K, ../../20221217060408.GA1256247@nathanxps13/2-fix_partition_and_toast_privs.patch)
download | inline diff:
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 79ccddce55..022b5e9bd3 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -119,6 +119,20 @@ get_partition_parent_worker(Relation inhRel, Oid relid, bool *detach_pending)
return result;
}
+Oid
+get_partition_root(Oid relid)
+{
+ List *ancestors = get_partition_ancestors(relid);
+ Oid root = InvalidOid;
+
+ if (list_length(ancestors) > 0)
+ root = llast_oid(ancestors);
+
+ list_free(ancestors);
+
+ return root;
+}
+
/*
* get_partition_ancestors
* Obtain ancestors of given relation
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 9bc10729b0..1ca2a65405 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -14,6 +14,7 @@
*/
#include "postgres.h"
+#include "access/genam.h"
#include "access/heapam.h"
#include "access/toast_compression.h"
#include "access/xact.h"
@@ -32,6 +33,7 @@
#include "nodes/makefuncs.h"
#include "storage/lock.h"
#include "utils/builtins.h"
+#include "utils/fmgroids.h"
#include "utils/rel.h"
#include "utils/syscache.h"
@@ -413,3 +415,30 @@ needs_toast_table(Relation rel)
/* Otherwise, let the AM decide. */
return table_relation_needs_toast_table(rel);
}
+
+Oid
+get_toast_parent(Oid relid)
+{
+ Relation rel;
+ SysScanDesc scan;
+ ScanKeyData key[1];
+ Oid result = InvalidOid;
+ HeapTuple tuple;
+
+ rel = table_open(RelationRelationId, AccessShareLock);
+
+ ScanKeyInit(&key[0],
+ Anum_pg_class_reltoastrelid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(relid));
+
+ scan = systable_beginscan(rel, InvalidOid, false, NULL, 1, key);
+ tuple = systable_getnext(scan);
+ if (HeapTupleIsValid(tuple))
+ result = ((Form_pg_class) GETSTRUCT(tuple))->oid;
+ systable_endscan(scan);
+
+ table_close(rel, AccessShareLock);
+
+ return result;
+}
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 8966b75bd1..04681ce494 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -80,6 +80,7 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
static List *get_tables_to_cluster(MemoryContext cluster_context);
static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
Oid indexOid);
+static bool cluster_is_permitted_for_relation(Oid relid, Oid userid);
/*---------------------------------------------------------------------------
@@ -366,8 +367,7 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
if (recheck)
{
/* Check that the user still has privileges for the relation */
- if (!object_ownercheck(RelationRelationId, tableOid, save_userid) &&
- pg_class_aclcheck(tableOid, save_userid, ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(tableOid, save_userid))
{
relation_close(OldHeap, AccessExclusiveLock);
goto out;
@@ -1646,8 +1646,7 @@ get_tables_to_cluster(MemoryContext cluster_context)
index = (Form_pg_index) GETSTRUCT(indexTuple);
- if (!object_ownercheck(RelationRelationId, index->indrelid, GetUserId()) &&
- pg_class_aclcheck(index->indrelid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(index->indrelid, GetUserId()))
continue;
/* Use a permanent memory context for the result list */
@@ -1695,12 +1694,11 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
if (get_rel_relkind(indexrelid) != RELKIND_INDEX)
continue;
- /* Silently skip partitions which the user has no access to. */
- if (!object_ownercheck(RelationRelationId, relid, GetUserId()) &&
- pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
- (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) ||
- IsSharedRelation(relid)))
- continue;
+ /*
+ * We already checked that the user has privileges to CLUSTER the
+ * partitioned table when we locked it earlier, so there's no need to
+ * check the privileges again here.
+ */
/* Use a permanent memory context for the result list */
old_context = MemoryContextSwitchTo(cluster_context);
@@ -1715,3 +1713,10 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
return rtcs;
}
+
+static bool
+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+ return pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK ||
+ has_parent_privs(relid, userid, ACL_MAINTAIN);
+}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 7dc1aca8fe..d83e0158dd 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2796,11 +2796,10 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
/* Check permissions */
table_oid = IndexGetRelation(relId, true);
- if (!object_ownercheck(RelationRelationId, relId, GetUserId()) &&
- OidIsValid(table_oid) &&
- pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
- aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX,
- relation->relname);
+ if (OidIsValid(table_oid) &&
+ pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
+ !has_parent_privs(table_oid, GetUserId(), ACL_MAINTAIN))
+ aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX, relation->relname);
/* Lock heap before index to avoid deadlock. */
if (relId != oldRelId)
@@ -3017,7 +3016,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
*/
if (classtuple->relisshared &&
!object_ownercheck(RelationRelationId, relid, GetUserId()) &&
- pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
+ !has_parent_privs(relid, GetUserId(), ACL_MAINTAIN))
continue;
/*
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index e294efc67c..425e94f27c 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -17,6 +17,7 @@
#include "access/table.h"
#include "access/xact.h"
#include "catalog/namespace.h"
+#include "catalog/partition.h"
#include "catalog/pg_inherits.h"
#include "commands/lockcmds.h"
#include "miscadmin.h"
@@ -305,5 +306,17 @@ LockTableAclCheck(Oid reloid, LOCKMODE lockmode, Oid userid)
aclresult = pg_class_aclcheck(reloid, userid, aclmask);
+ /*
+ * If this is a partition, check the permissions on its partitioned table,
+ * too.
+ */
+ if (aclresult != ACLCHECK_OK && get_rel_relispartition(reloid))
+ {
+ Oid root = get_partition_root(reloid);
+
+ if (OidIsValid(root))
+ aclresult = pg_class_aclcheck(root, userid, ACL_MAINTAIN);
+ }
+
return aclresult;
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 56dc995713..a2763b43df 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16918,12 +16918,42 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation,
errmsg("\"%s\" is not a table or materialized view", relation->relname)));
/* Check permissions */
- if (!object_ownercheck(RelationRelationId, relId, GetUserId()) &&
- pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
+ !has_parent_privs(relId, GetUserId(), ACL_MAINTAIN))
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE,
relation->relname);
}
+bool
+has_parent_privs(Oid relid, Oid userid, AclMode acl)
+{
+ /*
+ * If this is a partition, check the permissions on its partitioned table.
+ */
+ if (get_rel_relispartition(relid))
+ {
+ Oid root = get_partition_root(relid);
+
+ if (OidIsValid(root) &&
+ pg_class_aclcheck(root, userid, acl) == ACLCHECK_OK)
+ return true;
+ }
+
+ /*
+ * If this is a TOAST table, check the permissions on the main relation.
+ */
+ if (get_rel_relkind(relid) == RELKIND_TOASTVALUE)
+ {
+ Oid parent = get_toast_parent(relid);
+
+ if (OidIsValid(parent) &&
+ pg_class_aclcheck(parent, userid, acl) == ACLCHECK_OK)
+ return true;
+ }
+
+ return false;
+}
+
/*
* Callback to RangeVarGetRelidExtended() for TRUNCATE processing.
*/
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 293b84bbca..9360564945 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -41,6 +41,7 @@
#include "catalog/pg_namespace.h"
#include "commands/cluster.h"
#include "commands/defrem.h"
+#include "commands/tablecmds.h"
#include "commands/vacuum.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
@@ -568,9 +569,9 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,
* - the role owns the current database and the relation is not shared
* - the role has been granted the MAINTAIN privilege on the relation
*/
- if (object_ownercheck(RelationRelationId, relid, GetUserId()) ||
- (object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) ||
- pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK)
+ if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) ||
+ pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK ||
+ has_parent_privs(relid, GetUserId(), ACL_MAINTAIN))
return true;
relname = NameStr(reltuple->relname);
diff --git a/src/include/catalog/partition.h b/src/include/catalog/partition.h
index b29df6690c..41144abf3f 100644
--- a/src/include/catalog/partition.h
+++ b/src/include/catalog/partition.h
@@ -20,6 +20,7 @@
#define HASH_PARTITION_SEED UINT64CONST(0x7A5B22367996DCFD)
extern Oid get_partition_parent(Oid relid, bool even_if_detached);
+extern Oid get_partition_root(Oid relid);
extern List *get_partition_ancestors(Oid relid);
extern Oid index_get_partition(Relation partition, Oid indexId);
extern List *map_partition_varattnos(List *expr, int fromrel_varno,
diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h
index 51e3ba7b69..2afdec5c4a 100644
--- a/src/include/catalog/toasting.h
+++ b/src/include/catalog/toasting.h
@@ -26,5 +26,6 @@ extern void AlterTableCreateToastTable(Oid relOid, Datum reloptions,
LOCKMODE lockmode);
extern void BootstrapToastTable(char *relName,
Oid toastOid, Oid toastIndexOid);
+extern Oid get_toast_parent(Oid relid);
#endif /* TOASTING_H */
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 07eac9a26c..959ae53aa8 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -98,6 +98,7 @@ extern void AtEOSubXact_on_commit_actions(bool isCommit,
extern void RangeVarCallbackMaintainsTable(const RangeVar *relation,
Oid relId, Oid oldRelId,
void *arg);
+extern bool has_parent_privs(Oid relid, Oid userid, AclMode acl);
extern void RangeVarCallbackOwnsRelation(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out
index 7acb675c97..8d21276996 100644
--- a/src/test/isolation/expected/cluster-conflict-partition.out
+++ b/src/test/isolation/expected/cluster-conflict-partition.out
@@ -22,14 +22,16 @@ starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_res
step s1_begin: BEGIN;
step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE;
step s2_auth: SET ROLE regress_cluster_part;
-step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind;
+step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...>
step s1_commit: COMMIT;
+step s2_cluster: <... completed>
step s2_reset: RESET ROLE;
starting permutation: s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset
step s1_begin: BEGIN;
step s2_auth: SET ROLE regress_cluster_part;
step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE;
-step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind;
+step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...>
step s1_commit: COMMIT;
+step s2_cluster: <... completed>
step s2_reset: RESET ROLE;
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 542c2e098c..332210e81f 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -513,7 +513,7 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a
-----------+----------
ptnowner | t
ptnowner1 | f
- ptnowner2 | t
+ ptnowner2 | f
(3 rows)
DROP TABLE ptnowner;
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 0035d158b7..41ff4ba9cf 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -347,20 +347,14 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum;
ALTER TABLE vacowned_part1 OWNER TO regress_vacuum;
SET ROLE regress_vacuum;
VACUUM vacowned_parted;
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
VACUUM vacowned_part1;
VACUUM vacowned_part2;
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
ANALYZE vacowned_parted;
-WARNING: permission denied to analyze "vacowned_part2", skipping it
ANALYZE vacowned_part1;
ANALYZE vacowned_part2;
-WARNING: permission denied to analyze "vacowned_part2", skipping it
VACUUM (ANALYZE) vacowned_parted;
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
VACUUM (ANALYZE) vacowned_part1;
VACUUM (ANALYZE) vacowned_part2;
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
RESET ROLE;
-- Only one partition owned by other user.
ALTER TABLE vacowned_parted OWNER TO CURRENT_USER;
@@ -389,26 +383,14 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum;
ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER;
SET ROLE regress_vacuum;
VACUUM vacowned_parted;
-WARNING: permission denied to vacuum "vacowned_part1", skipping it
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
VACUUM vacowned_part1;
-WARNING: permission denied to vacuum "vacowned_part1", skipping it
VACUUM vacowned_part2;
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
ANALYZE vacowned_parted;
-WARNING: permission denied to analyze "vacowned_part1", skipping it
-WARNING: permission denied to analyze "vacowned_part2", skipping it
ANALYZE vacowned_part1;
-WARNING: permission denied to analyze "vacowned_part1", skipping it
ANALYZE vacowned_part2;
-WARNING: permission denied to analyze "vacowned_part2", skipping it
VACUUM (ANALYZE) vacowned_parted;
-WARNING: permission denied to vacuum "vacowned_part1", skipping it
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
VACUUM (ANALYZE) vacowned_part1;
-WARNING: permission denied to vacuum "vacowned_part1", skipping it
VACUUM (ANALYZE) vacowned_part2;
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
RESET ROLE;
DROP TABLE vacowned;
DROP TABLE vacowned_parted;
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-17 12:39 Ted Yu <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 2 replies; 39+ messages in thread
From: Ted Yu @ 2022-12-17 12:39 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Jeff Davis <[email protected]>; Pavel Luzanov <[email protected]>; Justin Pryzby <[email protected]>; pgsql-hackers
On Fri, Dec 16, 2022 at 10:04 PM Nathan Bossart <[email protected]>
wrote:
> On Thu, Dec 15, 2022 at 10:10:43AM -0800, Jeff Davis wrote:
> > The proposal to skip privilege checks for partitions would be
> > consistent with INSERT, SELECT, REINDEX that flow through to the
> > underlying partitions regardless of permissions/ownership (and even
> > RLS). It would be very minor behavior change on 15 for this weird case
> > of superuser-owned partitions, but I doubt anyone would be relying on
> > that.
>
> I've attached a work-in-progress patch that aims to accomplish this.
> Instead of skipping the privilege checks, I added logic to trawl through
> pg_inherits and pg_class to check whether the user has privileges for the
> partitioned table or for the main relation of a TOAST table. This means
> that MAINTAIN on a partitioned table is enough to execute maintenance
> commands on all the partitions, and MAINTAIN on a main relation is enough
> to execute maintenance commands on its TOAST table. Also, the maintenance
> commands that flow through to the partitions or the TOAST table should no
> longer error due to permissions when the user only has MAINTAIN on the
> paritioned table or main relation.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
Hi,
+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+ return pg_class_aclcheck(relid, userid, ACL_MAINTAIN) ==
ACLCHECK_OK ||
+ has_parent_privs(relid, userid, ACL_MAINTAIN);
Since the func only contains one statement, it seems this can be defined as
a macro instead.
+ List *ancestors = get_partition_ancestors(relid);
+ Oid root = InvalidOid;
nit: it would be better if the variable `root` can be aligned with variable
`ancestors`.
Cheers
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-18 23:30 Nathan Bossart <[email protected]>
parent: Ted Yu <[email protected]>
1 sibling, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2022-12-18 23:30 UTC (permalink / raw)
To: Ted Yu <[email protected]>; +Cc: Jeff Davis <[email protected]>; Pavel Luzanov <[email protected]>; Justin Pryzby <[email protected]>; pgsql-hackers
Here is a new version of the patch. Besides some cleanup, I added an index
on reltoastrelid for the toast-to-main-relation lookup. Before I bother
adjusting the tests and documentation, I'm curious to hear thoughts on
whether this seems like a viable approach.
On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote:
> +cluster_is_permitted_for_relation(Oid relid, Oid userid)
> +{
> + return pg_class_aclcheck(relid, userid, ACL_MAINTAIN) ==
> ACLCHECK_OK ||
> + has_parent_privs(relid, userid, ACL_MAINTAIN);
>
> Since the func only contains one statement, it seems this can be defined as
> a macro instead.
In the new version, there is a bit more to this function, so I didn't
convert it to a macro.
> + List *ancestors = get_partition_ancestors(relid);
> + Oid root = InvalidOid;
>
> nit: it would be better if the variable `root` can be aligned with variable
> `ancestors`.
Hm. It looked alright on my machine. In any case, I'll be sure to run
pgindent at some point. This patch is still in early stages.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-18 23:38 Justin Pryzby <[email protected]>
parent: Ted Yu <[email protected]>
1 sibling, 1 reply; 39+ messages in thread
From: Justin Pryzby @ 2022-12-18 23:38 UTC (permalink / raw)
To: Ted Yu <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Jeff Davis <[email protected]>; Pavel Luzanov <[email protected]>; pgsql-hackers
On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote:
> + List *ancestors = get_partition_ancestors(relid);
> + Oid root = InvalidOid;
>
> nit: it would be better if the variable `root` can be aligned with variable
> `ancestors`.
It is aligned, but only after configuring one's editor/pager/mail client
to display tabs in the manner assumed by postgres' coding style.
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-19 00:25 Ted Yu <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Ted Yu @ 2022-12-19 00:25 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Jeff Davis <[email protected]>; Pavel Luzanov <[email protected]>; Justin Pryzby <[email protected]>; pgsql-hackers
On Sun, Dec 18, 2022 at 3:30 PM Nathan Bossart <[email protected]>
wrote:
> Here is a new version of the patch. Besides some cleanup, I added an index
> on reltoastrelid for the toast-to-main-relation lookup. Before I bother
> adjusting the tests and documentation, I'm curious to hear thoughts on
> whether this seems like a viable approach.
>
> On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote:
> > +cluster_is_permitted_for_relation(Oid relid, Oid userid)
> > +{
> > + return pg_class_aclcheck(relid, userid, ACL_MAINTAIN) ==
> > ACLCHECK_OK ||
> > + has_parent_privs(relid, userid, ACL_MAINTAIN);
> >
> > Since the func only contains one statement, it seems this can be defined
> as
> > a macro instead.
>
> In the new version, there is a bit more to this function, so I didn't
> convert it to a macro.
>
> > + List *ancestors = get_partition_ancestors(relid);
> > + Oid root = InvalidOid;
> >
> > nit: it would be better if the variable `root` can be aligned with
> variable
> > `ancestors`.
>
> Hm. It looked alright on my machine. In any case, I'll be sure to run
> pgindent at some point. This patch is still in early stages.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
Hi,
+ * Note: Because this function assumes that the realtion whose OID is
passed as
Typo: realtion -> relation
Cheers
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-19 00:31 Nathan Bossart <[email protected]>
parent: Ted Yu <[email protected]>
0 siblings, 0 replies; 39+ messages in thread
From: Nathan Bossart @ 2022-12-19 00:31 UTC (permalink / raw)
To: Ted Yu <[email protected]>; +Cc: Jeff Davis <[email protected]>; Pavel Luzanov <[email protected]>; Justin Pryzby <[email protected]>; pgsql-hackers
On Sun, Dec 18, 2022 at 04:25:15PM -0800, Ted Yu wrote:
> + * Note: Because this function assumes that the realtion whose OID is
> passed as
>
> Typo: realtion -> relation
Thanks, fixed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
@ 2022-12-19 22:43 Jeff Davis <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 0 replies; 39+ messages in thread
From: Jeff Davis @ 2022-12-19 22:43 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; Ted Yu <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Pavel Luzanov <[email protected]>; pgsql-hackers
On Sun, 2022-12-18 at 17:38 -0600, Justin Pryzby wrote:
> On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote:
> > + List *ancestors = get_partition_ancestors(relid);
> > + Oid root = InvalidOid;
> >
> > nit: it would be better if the variable `root` can be aligned with
> > variable
> > `ancestors`.
>
> It is aligned, but only after configuring one's editor/pager/mail
> client
> to display tabs in the manner assumed by postgres' coding style.
If you use emacs or vim, there are editor config samples in
src/tools/editors/
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 39+ messages in thread
end of thread, other threads:[~2022-12-19 22:43 UTC | newest]
Thread overview: 39+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-12-08 18:37 allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX Nathan Bossart <[email protected]>
2022-12-10 20:07 ` Jeff Davis <[email protected]>
2022-12-10 20:41 ` Nathan Bossart <[email protected]>
2022-12-12 20:04 ` Nathan Bossart <[email protected]>
2022-12-12 21:01 ` Nathan Bossart <[email protected]>
2022-12-14 03:05 ` Jeff Davis <[email protected]>
2022-12-14 03:23 ` Nathan Bossart <[email protected]>
2022-12-14 09:07 ` Pavel Luzanov <[email protected]>
2022-12-14 17:13 ` Nathan Bossart <[email protected]>
2022-12-14 18:05 ` Alvaro Herrera <[email protected]>
2022-12-14 18:16 ` Nathan Bossart <[email protected]>
2022-12-14 19:05 ` Jeff Davis <[email protected]>
2022-12-14 23:29 ` Nathan Bossart <[email protected]>
2022-12-15 00:12 ` Michael Paquier <[email protected]>
2022-12-15 00:27 ` Nathan Bossart <[email protected]>
2022-12-15 09:42 ` Pavel Luzanov <[email protected]>
2022-12-15 18:42 ` Jeff Davis <[email protected]>
2022-12-15 19:12 ` Nathan Bossart <[email protected]>
2022-12-14 19:46 ` Jeff Davis <[email protected]>
2022-12-14 20:32 ` Isaac Morland <[email protected]>
2022-12-14 20:56 ` Jeff Davis <[email protected]>
2022-12-14 22:40 ` Isaac Morland <[email protected]>
2022-12-14 22:02 ` Pavel Luzanov <[email protected]>
2022-12-14 22:11 ` Justin Pryzby <[email protected]>
2022-12-15 00:18 ` Jeff Davis <[email protected]>
2022-12-15 09:31 ` Pavel Luzanov <[email protected]>
2022-12-15 18:10 ` Jeff Davis <[email protected]>
2022-12-15 19:48 ` Justin Pryzby <[email protected]>
2022-12-16 01:19 ` Nathan Bossart <[email protected]>
2022-12-16 04:35 ` Jeff Davis <[email protected]>
2022-12-16 05:13 ` Nathan Bossart <[email protected]>
2022-12-16 05:20 ` Nathan Bossart <[email protected]>
2022-12-17 06:04 ` Nathan Bossart <[email protected]>
2022-12-17 12:39 ` Ted Yu <[email protected]>
2022-12-18 23:30 ` Nathan Bossart <[email protected]>
2022-12-19 00:25 ` Ted Yu <[email protected]>
2022-12-19 00:31 ` Nathan Bossart <[email protected]>
2022-12-18 23:38 ` Justin Pryzby <[email protected]>
2022-12-19 22:43 ` Jeff Davis <[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