Date: Tue, 20 Jun 2023 21:15:18 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
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
Subject: Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
Message-ID: <20230621041518.GA774124@nathanxps13>
References: <20230613235442.GA222795@nathanxps13>
 <20230614181711.GA488295@nathanxps13>
 <ZIpfack6qJUHaw+g@paquier.xyz>
 <20230615041044.GA736001@nathanxps13>
 <20230615235700.GA877311@nathanxps13>
 <20230616052025.GA1026700@nathanxps13>
 <20230619215534.GA442477@nathanxps13>
 <edf8dc01333f4b3355aa91001cb26029cab1eb2e.camel@j-davis.com>
 <20230620225257.GA771663@nathanxps13>
 <ZJJQgBkTEvL8RpwP@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZJJQgBkTEvL8RpwP@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/20230621041518.GA774124%40nathanxps13>
Precedence: bulk

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