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
signature.asc
Description: PGP signature
