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

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