Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Nathan Bossart <nathandbossart(at)gmail(dot)com>
Cc: Jeff Davis <pgsql(at)j-davis(dot)com>, Ted Yu <yuzhihong(at)gmail(dot)com>, Pavel Luzanov <p(dot)luzanov(at)postgrespro(dot)ru>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
Date: 2023-06-21 01:21:04
Message-ID: ZJJQgBkTEvL8RpwP@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2023-06-21 01:28:58 Re: Adding further hardening to nbtree page deletion
Previous Message James Coleman 2023-06-21 00:55:00 Re: path->param_info only set for lateral?