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

On Thu, Dec 15, 2022 at 01:48:13PM -0600, Justin Pryzby wrote:
> vacuum initially calls vacuum_is_permitted_for_relation() only for the
> partitioned table, and *later* locks the partition and then checks its
> permissions, which is when the message is output.
> 
> Since v15, cluster calls get_tables_to_cluster_partitioned(), which
> silently discards partitions failing ACL.
> 
> We could change it to emit a message, which would seem to behave like
> vacuum, except that the check is happening earlier, and (unlike vacuum)
> partitions skipped later during CLUOPT_RECHECK wouldn't have any message
> output.
> 
> Or we could change cluster_rel() to output a message when skipping.  But
> these patches hardly touched that function at all.  I suppose we could
> change to emit a message during RECHECK (maybe only in master branch).
> If need be, that could be controlled by a new CLUOPT_*.

Yeah, VACUUM/ANALYZE works differently.  For one, you can specify multiple
relations in the command.  Also, VACUUM/ANALYZE only takes an
AccessShareLock when first assessing permissions (which actually skips
partitions).  CLUSTER immediately takes an AccessExclusiveLock, so the
permissions are checked up front.  This is done "to avoid lock-upgrade
hazard in the single-transaction case," which IIUC is what allows CLUSTER
on a single table to run within a transaction block (unlike VACUUM).  I
don't know if running CLUSTER in a transaction block is important.  IMO we
should consider making CLUSTER look a lot more like VACUUM/ANALYZE in this
regard.  The attached patch adds WARNING messages, but we're still far from
consistency with VACUUM/ANALYZE.

Side note: I see that CLUSTER on a partitioned table is disallowed in a
transaction block, which should probably be added to my documentation patch
[0].

[0] https://commitfest.postgresql.org/41/4054/

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com