Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
Michael Paquier <michael@paquier.xyz>
From: Michael Paquier <michael@paquier.xyz>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Jeff Davis <pgsql@j-davis.com>, Ted Yu <yuzhihong@gmail.com>, Pavel Luzanov <p.luzanov@postgrespro.ru>, Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@postgresql.org
Date: 2023-06-21T01:21:04Z
Lists: pgsql-hackers
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
Commits
-
Improve privilege documentation for maintenance commands.
- c2122aae636d 16.0 landed
-
Fix cache lookup hazards introduced by ff9618e82a.
- 4dbdb82513b6 16.0 landed
-
Move bool parameter for vacuum_rel() to option bits.
- 5b1a87994312 16.0 landed
-
Fix MAINTAIN privileges for toast tables and partitions.
- ff9618e82a46 16.0 landed
-
Add grantable MAINTAIN privilege and pg_maintain role.
- 60684dd834a2 16.0 landed