Thread: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
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
Attachment
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], 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
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
Attachment
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
Attachment
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/c0a85c2e83158560314b576b6241c8ed0aea1745.camel@j-davis.com [2] https://www.postgresql.org/message-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], 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
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: https://www.postgresql.org/message-id/YZI+aNEnnpBASxNU@paquier.xyz -- Michael
Attachment
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
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
Attachment
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
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
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
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
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
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
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
Attachment
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
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
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
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
Attachment
On Fri, Dec 16, 2022 at 10:04 PM Nathan Bossart <nathandbossart@gmail.com> 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,
+{
+ 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;
+ Oid root = InvalidOid;
nit: it would be better if the variable `root` can be aligned with variable `ancestors`.
Cheers
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
Attachment
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.
On Sun, Dec 18, 2022 at 3:30 PM Nathan Bossart <nathandbossart@gmail.com> 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
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
Attachment
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
On Sun, Dec 18, 2022 at 03:30:18PM -0800, Nathan Bossart 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. I'd like to get this fixed, but I have yet to hear thoughts on the suggested approach. I'll proceed with adjusting the tests and documentation shortly unless someone objects. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Tue, Jan 03, 2023 at 03:45:49PM -0800, Nathan Bossart wrote: > I'd like to get this fixed, but I have yet to hear thoughts on the > suggested approach. I'll proceed with adjusting the tests and > documentation shortly unless someone objects. As promised, here is a new version of the patch with adjusted tests and documentation. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Mon, 2023-01-09 at 14:51 -0800, Nathan Bossart wrote: > On Tue, Jan 03, 2023 at 03:45:49PM -0800, Nathan Bossart wrote: > > I'd like to get this fixed, but I have yet to hear thoughts on the > > suggested approach. I'll proceed with adjusting the tests and > > documentation shortly unless someone objects. > > As promised, here is a new version of the patch with adjusted tests > and > documentation. The current patch doesn't handle the case properly where you have partitions that have toast tables. An easy fix by recursing, but I think we might want to do things differently anyway. I'm hesitant to add an index to pg_class just for the privilege checks on toast tables, and I don't think we need to. Instead, we can just skip the privilege check on a toast table if it's not referenced directly, because we already checked the privileges on the parent, and we still hold the session lock so nothing strange should have happened. I'll look into that, but so far it looks like it should come out cleanly for toast tables. The way you're checking privileges on the partitioned tables is fine. -- Jeff Davis PostgreSQL Contributor Team - AWS
On Fri, Jan 13, 2023 at 11:56:03AM -0800, Jeff Davis wrote: > I'm hesitant to add an index to pg_class just for the privilege checks > on toast tables, and I don't think we need to. I bet this index will be useful for more than just these privilege checks (e.g., autovacuum currently creates a hash table for the toast-to-main-relation mapping), but I do understand the hesitation. > Instead, we can just > skip the privilege check on a toast table if it's not referenced > directly, because we already checked the privileges on the parent, and > we still hold the session lock so nothing strange should have happened. That would fix the problem in the original complaint, but it wouldn't allow for vacuuming toast tables directly if you only have MAINTAIN privileges on the main relation. If you can vacuum the toast table indirectly via the main relation, shouldn't it be possible to vacuum it directly? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Fri, 2023-01-13 at 12:33 -0800, Nathan Bossart wrote: > That would fix the problem in the original complaint, but it wouldn't > allow > for vacuuming toast tables directly if you only have MAINTAIN > privileges on > the main relation. If you can vacuum the toast table indirectly via > the > main relation, shouldn't it be possible to vacuum it directly? Perhaps, but that's barely supported today: you have to awkwardly find the internal toast table name yourself, and you need the admin to grant you USAGE on the pg_toast schema. I don't think we're obligated to also support this hackery for non-owners with a new MAINTAIN privilege. If we care about that use case, let's do it right and have forms of VACUUM/CLUSTER/REINDEX that check permissions on the main table, skip the work on the main table, and descend directly to the toast tables. That doesn't seem hard, but it's a separate patch. Right now, we should simply fix the problem. -- Jeff Davis PostgreSQL Contributor Team - AWS
On Fri, Jan 13, 2023 at 01:30:28PM -0800, Jeff Davis wrote: > If we care about that use case, let's do it right and have forms of > VACUUM/CLUSTER/REINDEX that check permissions on the main table, skip > the work on the main table, and descend directly to the toast tables. > That doesn't seem hard, but it's a separate patch. You may be interested in https://commitfest.postgresql.org/41/4088/. > Right now, we should simply fix the problem. Okay. Here is a new patch set. I've split the partition work out to a separate patch, and I've removed the main relation lookups for TOAST tables in favor of adding a skip_privs flag to vacuum_rel(). The latter patch probably needs some additional commentary and tests, which I'll go ahead and add if we want to proceed with this approach. I'm assuming the session lock should be sufficient for avoiding the case where the TOAST table's OID is reused by the time we get to it, but I'm not sure if it's sufficient to prevent vacuuming if the privileges on the main relation have changed across transactions. Even if it's not, I'm not sure that case is worth worrying about too much. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Fri, Jan 13, 2023 at 02:56:26PM -0800, Nathan Bossart wrote: > Okay. Here is a new patch set. And here is a rebased patch set (c44f633 changed the same LOCK TABLE docs). -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
I've been reviewing ff9618e lately, and I'm wondering whether it has the same problem that 19de0ab solved. Specifically, ff9618e introduces has_partition_ancestor_privs(), which is used to check whether a user has MAINTAIN on any partition ancestors. This involves syscache lookups, and presently this function does not take any relation locks. I did spend some time trying to induce cache lookup errors, but I didn't have any luck. However, unless this can be made safe without too much trouble, I think I'm inclined to partially revert ff9618e, leaving the TOAST-related parts intact. By reverting the partition-related parts of ff9618e, users would need to have MAINTAIN on the partition itself to perform the maintenance command. MAINTAIN on the partitioned table would no longer be sufficient. This is more like how things work on supported versions today. Privileges are checked for each partition, so a command that flows down to all partitions might refuse to process a partition (e.g., if the current user doesn't own the partition). In the future, perhaps we could reevaluate adding these partition ancestor privilege checks, but I'd rather leave it out for now instead of introducing behavior in v16 that is potentially buggy and difficult to remove post-GA. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Tue, Jun 13, 2023 at 02:12:46PM -0700, Nathan Bossart wrote: > I've been reviewing ff9618e lately, and I'm wondering whether it has the > same problem that 19de0ab solved. Specifically, ff9618e introduces > has_partition_ancestor_privs(), which is used to check whether a user has > MAINTAIN on any partition ancestors. This involves syscache lookups, and > presently this function does not take any relation locks. I did spend some > time trying to induce cache lookup errors, but I didn't have any luck. > However, unless this can be made safe without too much trouble, I think I'm > inclined to partially revert ff9618e, leaving the TOAST-related parts > intact. Hmm. get_rel_relispartition() and pg_class_aclcheck() are rather reliable when it comes to that as far as it goes. Still get_partition_ancestors() is your problem, isn't it? Indeed, it seems like a bad idea to do partition tree lookups without at least an AccessShareLock as you may finish with a list that makes pg_class_aclcheck() complain on a missing relation. The race is pretty narrow, but a stop point in get_partition_ancestors() with some partition tree manipulation should be enough to make operations like a schema-wide REINDEX less transparent with missing relations at least. has_partition_ancestor_privs() is used in RangeVarCallbackMaintainsTable(), on top of that. As written, it encourages incorrect use patterns. > By reverting the partition-related parts of ff9618e, users would need to > have MAINTAIN on the partition itself to perform the maintenance command. > MAINTAIN on the partitioned table would no longer be sufficient. This is > more like how things work on supported versions today. Privileges are > checked for each partition, so a command that flows down to all partitions > might refuse to process a partition (e.g., if the current user doesn't own > the partition). > > In the future, perhaps we could reevaluate adding these partition ancestor > privilege checks, but I'd rather leave it out for now instead of > introducing behavior in v16 that is potentially buggy and difficult to > remove post-GA. While on it, this buzzes me: static bool -vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params) +vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs) VacuumParams has been originally introduced to avoid extending vacuum_rel() with a bunch of arguments, no? So, yes, agreed about the removal of has_partition_ancestor_privs(). I am adding an open item assigned to you and Jeff. -- Michael
Attachment
On Wed, Jun 14, 2023 at 08:16:15AM +0900, Michael Paquier wrote: > While on it, this buzzes me: > static bool > -vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params) > +vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs) > > VacuumParams has been originally introduced to avoid extending > vacuum_rel() with a bunch of arguments, no? Yeah, that could probably be moved into VacuumParams. > So, yes, agreed about the removal of has_partition_ancestor_privs(). > I am adding an open item assigned to you and Jeff. Thanks. I suspect there's more discussion incoming, but I'm hoping to close this item one way or another by 16beta2. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Tue, Jun 13, 2023 at 04:54:42PM -0700, Nathan Bossart wrote: > On Wed, Jun 14, 2023 at 08:16:15AM +0900, Michael Paquier wrote: >> So, yes, agreed about the removal of has_partition_ancestor_privs(). >> I am adding an open item assigned to you and Jeff. > > Thanks. I suspect there's more discussion incoming, but I'm hoping to > close this item one way or another by 16beta2. Concretely, I am proposing something like the attached patches. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Tue, 2023-06-13 at 14:12 -0700, Nathan Bossart wrote:
> I've been reviewing ff9618e lately, and I'm wondering whether it has
> the
> same problem that 19de0ab solved. Specifically, ff9618e introduces
> has_partition_ancestor_privs(), which is used to check whether a user
> has
> MAINTAIN on any partition ancestors. This involves syscache lookups,
> and
> presently this function does not take any relation locks. I did
> spend some
> time trying to induce cache lookup errors, but I didn't have any
> luck.
> However, unless this can be made safe without too much trouble, I
> think I'm
> inclined to partially revert ff9618e, leaving the TOAST-related parts
> intact.
Agreed. Having it work on partition hierarchies is a nice-to-have, but
not central to the usability of the feature. If it's causing problems,
best to take that out and reconsider in 17 if worthwhile.
Regards,
Jeff Davis
On Wed, Jun 14, 2023 at 11:17:11AM -0700, Nathan Bossart wrote: > On Tue, Jun 13, 2023 at 04:54:42PM -0700, Nathan Bossart wrote: > > On Wed, Jun 14, 2023 at 08:16:15AM +0900, Michael Paquier wrote: > >> So, yes, agreed about the removal of has_partition_ancestor_privs(). > >> I am adding an open item assigned to you and Jeff. > > > > Thanks. I suspect there's more discussion incoming, but I'm hoping to > > close this item one way or another by 16beta2. > > Concretely, I am proposing something like the attached patches. The result after 0001 is applied is that a couple of object_ownercheck() calls that existed before ff9618e are removed from some ACL checks in the REINDEX, CLUSTER and VACUUM paths. Is that OK for shared relations and shouldn't cluster_is_permitted_for_relation() include that? vacuum_is_permitted_for_relation() is consistent on this side. Here are the paths that now differ: cluster_rel get_tables_to_cluster get_tables_to_cluster_partitioned RangeVarCallbackForReindexIndex ReindexMultipleTables 0002 looks OK to retain the skip check for toast relations in the VACUUM case. -- Michael
Attachment
On Thu, Jun 15, 2023 at 09:46:33AM +0900, Michael Paquier wrote: > The result after 0001 is applied is that a couple of > object_ownercheck() calls that existed before ff9618e are removed from > some ACL checks in the REINDEX, CLUSTER and VACUUM paths. Is that OK > for shared relations and shouldn't cluster_is_permitted_for_relation() > include that? vacuum_is_permitted_for_relation() is consistent on > this side. These object_ownercheck() calls were removed because they were redundant, as owners have all privileges by default. Privileges can be revoked from the owner, so an extra ownership check would effectively bypass the relation's ACL in that case. I looked around and didn't see any other examples of a combined ownership and ACL check like we were doing for MAINTAIN. The only thing that gives me pause is that the docs call out ownership as sufficient for some maintenance commands. With these patches, that's only true as long as no one revokes privileges from the owner. IMO we should update the docs and leave out the ownership checks since MAINTAIN is now a grantable privilege like any other. WDYT? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Wed, Jun 14, 2023 at 09:10:44PM -0700, Nathan Bossart wrote: > IMO > we should update the docs and leave out the ownership checks since MAINTAIN > is now a grantable privilege like any other. WDYT? Here's an attempt at adjusting the documentation as I proposed yesterday. I think this is a good opportunity to simplify the privilege-related sections for these maintenance commands. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Thu, Jun 15, 2023 at 04:57:00PM -0700, Nathan Bossart wrote: > Here's an attempt at adjusting the documentation as I proposed yesterday. > I think this is a good opportunity to simplify the privilege-related > sections for these maintenance commands. I noticed that it was possible to make the documentation changes in 0001 easier to read. Apologies for the noise. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Thu, Jun 15, 2023 at 10:20:25PM -0700, Nathan Bossart wrote: > I noticed that it was possible to make the documentation changes in 0001 > easier to read. Apologies for the noise. Yet more noise... In v4 of the patch set, I moved the skip_privs flag refactoring to 0001. I intend to commit this tomorrow unless there is additional feedback. I split out the documentation simplifications to 0003 since it seemed independent. Also, I adjusted some ACL-related error messages in 0002 to appropriately use "permission denied" messages instead of "must be owner" messages. I'm hoping to commit 0002 and 0003 by the end of the week so that these fixes are available in 16beta2. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Mon, Jun 19, 2023 at 02:55:34PM -0700, Nathan Bossart wrote: > In v4 of the patch set, I moved the skip_privs flag refactoring to 0001. I > intend to commit this tomorrow unless there is additional feedback. Fine by me. 0001 looks OK seen from here. > These object_ownercheck() calls were removed because they were redundant, > as owners have all privileges by default. Privileges can be revoked from > the owner, so an extra ownership check would effectively bypass the > relation's ACL in that case. I looked around and didn't see any other > examples of a combined ownership and ACL check like we were doing for > MAINTAIN. The only thing that gives me pause is that the docs call out > ownership as sufficient for some maintenance commands. With these patches, > that's only true as long as no one revokes privileges from the owner. IMO > we should update the docs and leave out the ownership checks since MAINTAIN > is now a grantable privilege like any other. WDYT? TBH, I have a mixed feeling about this line of reasoning because MAINTAIN is much broader and less specific than TRUNCATE, for instance, being spawned across so much more operations. As you say, owners of a relation have the MAINTAIN right by default, but they would not be able to run any maintenance operations if somebody has revoked their MAINTAIN right to do so, even if they are the owners of the so-said relation. Perhaps that's OK in the long run, still I have mixed feeling about whether that's the best decision we can take here, especially because MAINTAIN impacts VACUUM, ANALYZE, CLUSTER, REFRESH MATVIEW, REINDEX and LOCK. Some users may find that surprising as they used to have more control over these operations as owners of the relations worked on. -- Michael
Attachment
On Tue, 2023-06-20 at 14:26 +0900, Michael Paquier wrote:
> TBH, I have a mixed feeling about this line of reasoning because
> MAINTAIN is much broader and less specific than TRUNCATE, for
> instance, being spawned across so much more operations.
...
> Some users may find that surprising as they
> used to have more control over these operations as owners of the
> relations worked on.
It seems like the user shouldn't be surprised if they can carry out the
action; nor should they be surprised if they can't carry out the
action. Having privileges revoked on a table from the table's owner is
an edge case in behavior and both make sense to me.
In the absense of a use case, I'd be inclined towards just being
consistent with the other privileges.
Regards,
Jeff Davis
On Mon, 2023-06-19 at 14:55 -0700, Nathan Bossart wrote:
> In v4 of the patch set, I moved the skip_privs flag refactoring to
> 0001. I
> intend to commit this tomorrow unless there is additional feedback.
I think v4-0001 broke the handling of toast tables? It looks like you
removed the check for !skip_privs but need to add it to the flags in
vacuum_is_permitted_for_relation().
Regards,
Jeff Davis
On Tue, Jun 20, 2023 at 10:04:37AM -0700, Jeff Davis wrote: > I think v4-0001 broke the handling of toast tables? It looks like you > removed the check for !skip_privs but need to add it to the flags in > vacuum_is_permitted_for_relation(). Good catch. I'm not sure why some of the calls to vacuum_is_permitted_for_relation() are masking the options. AFAICT we can simply remove the masks. I've done so in the attached patch. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Tue, Jun 20, 2023 at 09:16:59AM -0700, Jeff Davis wrote: > On Tue, 2023-06-20 at 14:26 +0900, Michael Paquier wrote: >> TBH, I have a mixed feeling about this line of reasoning because >> MAINTAIN is much broader and less specific than TRUNCATE, for >> instance, being spawned across so much more operations. > > ... > >> Some users may find that surprising as they >> used to have more control over these operations as owners of the >> relations worked on. > > It seems like the user shouldn't be surprised if they can carry out the > action; nor should they be surprised if they can't carry out the > action. Having privileges revoked on a table from the table's owner is > an edge case in behavior and both make sense to me. > > In the absense of a use case, I'd be inclined towards just being > consistent with the other privileges. Agreed, I think we should make MAINTAIN consistent with the other grantable privileges. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Tue, Jun 20, 2023 at 10:40:32AM -0700, Nathan Bossart wrote: > On Tue, Jun 20, 2023 at 10:04:37AM -0700, Jeff Davis wrote: >> I think v4-0001 broke the handling of toast tables? It looks like you >> removed the check for !skip_privs but need to add it to the flags in >> vacuum_is_permitted_for_relation(). > > Good catch. I'm not sure why some of the calls to > vacuum_is_permitted_for_relation() are masking the options. AFAICT we can > simply remove the masks. I've done so in the attached patch. Oh, I think I see why. This appears to be used to control which WARNING message is emitted. If you lose permissions before you get to analyzing in a VACUUM (ANALYZE) command, you'll get a "permission denied to vacuum" message instead of a "permission denied to analyze" message. IMO a better way to do that would be to control only those two bits (VACOPT_VACUUM and VACOPT_ANALYZE) in calls to vacuum_is_permitted_for_relation(), and to leave the rest untouched. Patch incoming... -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Tue, Jun 20, 2023 at 10:49:27AM -0700, Nathan Bossart wrote: > Patch incoming... Attached. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Mon, 2023-06-19 at 14:55 -0700, Nathan Bossart wrote:
> I'm hoping to commit 0002 and 0003 by the end of the week so
> that these fixes are available in 16beta2.
A few observations for the case where a user does have the MAINTAIN
privilege on a partitioned table but not the partitions:
* they can LOCK TABLE on the partitioned table
* ANALYZE works on the inheritance tree but not the individual
partitions
* CLUSTER and VACUUM are useless because they skip all of the
partitions. That's consistent with the purpose of this thread -- to
avoid the locking problems trying to support those operations on
partitioned tables.
* REINDEX TABLE applies to all indexes in all partitions, which seems
a bit inconsistent.
The only behavior I'm worried about is REINDEX. I'm not sure what we
should do about it, or if we even want to do something about it. If we
want REINDEX to fail in this case, we should be sure to check
permissions on everything up-front to avoid doing a lot of work. The
only other option I can think of is to REINDEX only those indexes
declared on the partitioned table (not the individual partitions),
which seems consistent but might be confusing to users.
Regards,
Jeff Davis
On Tue, 2023-06-20 at 10:56 -0700, Nathan Bossart wrote:
> On Tue, Jun 20, 2023 at 10:49:27AM -0700, Nathan Bossart wrote:
> > Patch incoming...
>
> Attached.
Looks good to me.
Regards,
Jeff Davis
On Tue, Jun 20, 2023 at 11:49:36AM -0700, Jeff Davis wrote: > On Tue, 2023-06-20 at 10:56 -0700, Nathan Bossart wrote: >> On Tue, Jun 20, 2023 at 10:49:27AM -0700, Nathan Bossart wrote: >> > Patch incoming... >> >> Attached. > > Looks good to me. Thanks, committed. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
I've attached rebased versions of the remaining two patches. On Tue, Jun 20, 2023 at 11:43:05AM -0700, Jeff Davis wrote: > * REINDEX TABLE applies to all indexes in all partitions, which seems > a bit inconsistent. > > The only behavior I'm worried about is REINDEX. I'm not sure what we > should do about it, or if we even want to do something about it. If we > want REINDEX to fail in this case, we should be sure to check > permissions on everything up-front to avoid doing a lot of work. The > only other option I can think of is to REINDEX only those indexes > declared on the partitioned table (not the individual partitions), > which seems consistent but might be confusing to users. At the moment, I think I'm inclined to call this "existing behavior" since we didn't check privileges for each partition in this case even before MAINTAIN was introduced. IIUC we still process the individual partitions in v15 regardless of whether the calling user owns the partition. However, I do agree that it feels inconsistent. Besides the options you proposed, we might also consider making REINDEX work a bit more like VACUUM and ANALYZE and emit a WARNING for any relations that the user is not permitted to process. But this probably deserves its own thread, and it might even need to wait until v17. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Tue, Jun 20, 2023 at 11:43:05AM -0700, Jeff Davis wrote: > The only behavior I'm worried about is REINDEX. I'm not sure what we > should do about it, or if we even want to do something about it. If we > want REINDEX to fail in this case, we should be sure to check > permissions on everything up-front to avoid doing a lot of work. Yes, that feels a bit inconsistent to only check the partitioned table in RangeVarCallbackForReindexIndex() and let all the partitions process as a user may not have the permissions to work on the partitions themselves. We'd need something close to expand_vacuum_rel() for this work. I am not sure that this level of change is required, TBH, still it could be discussed for v17~. > The > only other option I can think of is to REINDEX only those indexes > declared on the partitioned table (not the individual partitions), > which seems consistent but might be confusing to users. I am not sure to understand this last sentence. REINDEX on a partitioned table builds a list of the indexes to work on in the first transaction processing the command in ReindexPartitions(), and there is no need to process partitioned indexes as these have no storage, so your suggestion is a no-op? -- Michael
Attachment
On Tue, Jun 20, 2023 at 03:52:57PM -0700, Nathan Bossart wrote:
> However, I do agree that it feels inconsistent. Besides the options you
> proposed, we might also consider making REINDEX work a bit more like VACUUM
> and ANALYZE and emit a WARNING for any relations that the user is not
> permitted to process. But this probably deserves its own thread, and it
> might even need to wait until v17.
Looking at 0001..
-step s2_auth { SET ROLE regress_cluster_part; }
+step s2_auth { SET ROLE regress_cluster_part; SET client_min_messages = ERROR; }
Is this change necessary because the ordering of the WARNING messages
generated for denied permissions is not guaranteed?
From the generated vacuum.out:
-- Only one partition owned by other user.
ALTER TABLE vacowned_parted OWNER TO CURRENT_USER;
SET ROLE regress_vacuum;
VACUUM vacowned_parted;
WARNING: permission denied to vacuum "vacowned_parted", skipping it
WARNING: permission denied to vacuum "vacowned_part2", skipping it
This is interesting. In this case, regress_vacuum owns only one
partition, but we would be able to vacuum it even when querying
vacowned_parted. Seeing from [1], this is intentional as per the
argument that VACUUM/ANALYZE can take multiple relations. Am I
getting that right? That's different from CLUSTER or REINDEX, where
not owning the partitioned table fails immediately.
I think that there is a testing gap with the coverage of CLUSTER.
"Ownership of partitions is checked" is a test that looks for the case
where regress_ptnowner owns the partitioned table and one of its
partitions, checking that the leaf not owned is skipped, but we don't
have a test where we attempt a CLUSTER on the partitioned table with
regress_ptnowner *not* owning the partitioned table, only one or more
of its partitions owned by regress_ptnowner. In this case, the
command would fail.
- privilege on the catalog. If a role has permission to
- <command>REINDEX</command> a partitioned table, it is also permitted to
- <command>REINDEX</command> each of its partitions, regardless of whether the
- role has the aforementioned privileges on the partition. Of course,
- superusers can always reindex anything.
+ privilege on the catalog. Of course, superusers can always reindex anything.
With 0001 applied, if a user is marked as an owner of a partitioned
table, all the partitions are reindexed even if this user does not own
a portion of them, making this change incorrect while the former is
more correct?
[1]: https://www.postgresql.org/message-id/20221216011926.GA771496@nathanxps13
--
Michael
Attachment
On Wed, Jun 21, 2023 at 10:21:04AM +0900, Michael Paquier wrote:
> Looking at 0001..
Thanks for taking a look.
> -step s2_auth { SET ROLE regress_cluster_part; }
> +step s2_auth { SET ROLE regress_cluster_part; SET client_min_messages = ERROR; }
>
> Is this change necessary because the ordering of the WARNING messages
> generated for denied permissions is not guaranteed?
Yes.
> From the generated vacuum.out:
> -- Only one partition owned by other user.
> ALTER TABLE vacowned_parted OWNER TO CURRENT_USER;
> SET ROLE regress_vacuum;
> VACUUM vacowned_parted;
> WARNING: permission denied to vacuum "vacowned_parted", skipping it
> WARNING: permission denied to vacuum "vacowned_part2", skipping it
>
> This is interesting. In this case, regress_vacuum owns only one
> partition, but we would be able to vacuum it even when querying
> vacowned_parted. Seeing from [1], this is intentional as per the
> argument that VACUUM/ANALYZE can take multiple relations. Am I
> getting that right? That's different from CLUSTER or REINDEX, where
> not owning the partitioned table fails immediately.
Yes.
> I think that there is a testing gap with the coverage of CLUSTER.
> "Ownership of partitions is checked" is a test that looks for the case
> where regress_ptnowner owns the partitioned table and one of its
> partitions, checking that the leaf not owned is skipped, but we don't
> have a test where we attempt a CLUSTER on the partitioned table with
> regress_ptnowner *not* owning the partitioned table, only one or more
> of its partitions owned by regress_ptnowner. In this case, the
> command would fail.
We could add something for this, but it'd really just exercise the checks
in RangeVarCallbackMaintainsTable(), which already has a decent amount of
coverage.
> - privilege on the catalog. If a role has permission to
> - <command>REINDEX</command> a partitioned table, it is also permitted to
> - <command>REINDEX</command> each of its partitions, regardless of whether the
> - role has the aforementioned privileges on the partition. Of course,
> - superusers can always reindex anything.
> + privilege on the catalog. Of course, superusers can always reindex anything.
>
> With 0001 applied, if a user is marked as an owner of a partitioned
> table, all the partitions are reindexed even if this user does not own
> a portion of them, making this change incorrect while the former is
> more correct?
The former wording would be true from the perspective that REINDEX on a
partitioned table will flow down to its partitions and skip privilege
checks on them, but it's incomplete because REINDEX on the individual
partitions might still fail due to privileges (even if the user has
privileges to REINDEX the partitioned table). After both patches are
applied, the privilege documentation is distilled down to
Reindexing a single index or table requires having the MAINTAIN
privilege on the table.
plus some assorted notes about REINDEX DATABASE/SCHEMA/SYSTEM. I think the
proposed wording is accurate, but I can see the argument that it leaves
some ambiguity for the partitioned table case. Perhaps we should add
something like
Note that while REINDEX on a partitioned index or table requires
MAINTAIN on the partitioned table, such commands skip the privilege
checks when processing the individual partitions.
Thoughts? I'm trying to keep the privilege documentation for maintenance
commands as simple as possible, so I'm hoping to avoid adding too much text
dedicated to these special cases.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, 2023-06-21 at 07:53 +0900, Michael Paquier wrote:
> I am not sure to understand this last sentence. REINDEX on a
> partitioned table builds a list of the indexes to work on in the
> first
> transaction processing the command in ReindexPartitions(), and there
> is no need to process partitioned indexes as these have no storage,
> so
> your suggestion is a no-op?
What I meant is that if you do:
CREATE TABLE p(i INT, j INT) PARTITION BY RANGE (i);
CREATE TABLE p0 PARTITION OF p FOR VALUES FROM (00) TO (10);
CREATE TABLE p1 PARTITION OF p FOR VALUES FROM (10) TO (20);
CREATE INDEX p_idx ON p (i);
CREATE INDEX special_idx ON p0 (j);
GRANT MAINTAIN ON p TO foo;
\c - foo
REINDEX TABLE p;
That would reindex p0_i_idx and p1_i_idx, but skip special_idx. That
might be too confusing, but feels a bit more consistent permissions-
wise.
Regards,
Jeff Davis
On Tue, 2023-06-20 at 15:52 -0700, Nathan Bossart wrote:
> At the moment, I think I'm inclined to call this "existing behavior"
> since
> we didn't check privileges for each partition in this case even
> before
> MAINTAIN was introduced. IIUC we still process the individual
> partitions
> in v15 regardless of whether the calling user owns the partition.
That's fine with me. I just wanted to bring it up in case someone else
thought it was a problem.
> However, I do agree that it feels inconsistent. Besides the options
> you
> proposed, we might also consider making REINDEX work a bit more like
> VACUUM
> and ANALYZE and emit a WARNING for any relations that the user is not
> permitted to process. But this probably deserves its own thread, and
> it
> might even need to wait until v17.
Yes, we can revisit for 17.
Regards,
Jeff Davis
On Tue, Jun 20, 2023 at 09:15:18PM -0700, Nathan Bossart wrote: > Perhaps we should add something like > > Note that while REINDEX on a partitioned index or table requires > MAINTAIN on the partitioned table, such commands skip the privilege > checks when processing the individual partitions. > > Thoughts? I'm trying to keep the privilege documentation for maintenance > commands as simple as possible, so I'm hoping to avoid adding too much text > dedicated to these special cases. Here is a new patch set that includes this new sentence. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Wed, Jun 21, 2023 at 09:26:09AM -0700, Jeff Davis wrote: > What I meant is that if you do: > > CREATE TABLE p(i INT, j INT) PARTITION BY RANGE (i); > CREATE TABLE p0 PARTITION OF p FOR VALUES FROM (00) TO (10); > CREATE TABLE p1 PARTITION OF p FOR VALUES FROM (10) TO (20); > CREATE INDEX p_idx ON p (i); > CREATE INDEX special_idx ON p0 (j); > GRANT MAINTAIN ON p TO foo; > \c - foo > REINDEX TABLE p; > > That would reindex p0_i_idx and p1_i_idx, but skip special_idx. That > might be too confusing, but feels a bit more consistent permissions- > wise. FWIW, the current behavior to reindex special_idx in this case feels more natural to me, as the user requests a REINDEX at table-level, not at index-level. This would mean to me that all the indexes of all the partitions should be rebuilt on, not just the partitioned indexes that are defined in the partitioned table requested for rebuild. -- Michael
Attachment
On Wed, Jun 21, 2023 at 10:16:24AM -0700, Nathan Bossart wrote: >> I think that there is a testing gap with the coverage of CLUSTER. >> "Ownership of partitions is checked" is a test that looks for the case >> where regress_ptnowner owns the partitioned table and one of its >> partitions, checking that the leaf not owned is skipped, but we don't >> have a test where we attempt a CLUSTER on the partitioned table with >> regress_ptnowner *not* owning the partitioned table, only one or more >> of its partitions owned by regress_ptnowner. In this case, the >> command would fail. > > We could add something for this, but it'd really just exercise the checks > in RangeVarCallbackMaintainsTable(), which already has a decent amount of > coverage. It seems to me that this has some value for the CLUSTER path, so I would add a small thing for it. > On Tue, Jun 20, 2023 at 09:15:18PM -0700, Nathan Bossart wrote: >> Perhaps we should add something like >> >> Note that while REINDEX on a partitioned index or table requires >> MAINTAIN on the partitioned table, such commands skip the privilege >> checks when processing the individual partitions. >> >> Thoughts? I'm trying to keep the privilege documentation for maintenance >> commands as simple as possible, so I'm hoping to avoid adding too much text >> dedicated to these special cases. > > Here is a new patch set that includes this new sentence. - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX, - relation->relname); Interesting that the previous code assumed ACLCHECK_NOT_OWNER all the time in the reindex RangeVar callback. - /* - * 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. - */ + if (!cluster_is_permitted_for_relation(relid, GetUserId())) + continue; I would add a comment here that this ACL recheck for the leaves is an important thing to keep around as it impacts the case where the leaves have a different owner than the parent, and the owner of the parent clusters it. The only place in the tests where this has an influence is the isolation test cluster-conflict-partition. The documentation changes seem in line with the code changes, particularly for VACUUM and REINDEX where we have some special handling for shared catalogs with ownership. -- Michael
Attachment
On Thu, Jun 22, 2023 at 10:46:41AM +0900, Michael Paquier wrote: > On Wed, Jun 21, 2023 at 10:16:24AM -0700, Nathan Bossart wrote: >>> I think that there is a testing gap with the coverage of CLUSTER. >>> "Ownership of partitions is checked" is a test that looks for the case >>> where regress_ptnowner owns the partitioned table and one of its >>> partitions, checking that the leaf not owned is skipped, but we don't >>> have a test where we attempt a CLUSTER on the partitioned table with >>> regress_ptnowner *not* owning the partitioned table, only one or more >>> of its partitions owned by regress_ptnowner. In this case, the >>> command would fail. >> >> We could add something for this, but it'd really just exercise the checks >> in RangeVarCallbackMaintainsTable(), which already has a decent amount of >> coverage. > > It seems to me that this has some value for the CLUSTER path, so I > would add a small thing for it. Done. > - /* > - * 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. > - */ > + if (!cluster_is_permitted_for_relation(relid, GetUserId())) > + continue; > I would add a comment here that this ACL recheck for the leaves is an > important thing to keep around as it impacts the case where the leaves > have a different owner than the parent, and the owner of the parent > clusters it. The only place in the tests where this has an influence > is the isolation test cluster-conflict-partition. Done. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Wed, Jun 21, 2023 at 08:06:06PM -0700, Nathan Bossart wrote: > On Thu, Jun 22, 2023 at 10:46:41AM +0900, Michael Paquier wrote: >> - /* >> - * 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. >> - */ >> + if (!cluster_is_permitted_for_relation(relid, GetUserId())) >> + continue; >> I would add a comment here that this ACL recheck for the leaves is an >> important thing to keep around as it impacts the case where the leaves >> have a different owner than the parent, and the owner of the parent >> clusters it. The only place in the tests where this has an influence >> is the isolation test cluster-conflict-partition. > > Done. + /* + * It's possible that the user does not have privileges to CLUSTER the + * leaf partition despite having such privileges on the partitioned + * table. We skip any partitions which the user is not permitted to + * CLUSTER. + */ Sounds good to me. Thanks. -- Michael
Attachment
On Thu, Jun 22, 2023 at 04:11:08PM +0900, Michael Paquier wrote: > Sounds good to me. Thanks. I plan to commit these patches later today. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Thu, Jun 22, 2023 at 08:43:01AM -0700, Nathan Bossart wrote: > I plan to commit these patches later today. Committed. I've also marked the related open item for v16 as resolved. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com