On 2022-12-07 We 23:13, Nathan Bossart wrote:
> On Wed, Dec 07, 2022 at 08:25:59PM -0600, Justin Pryzby wrote:
>> Your patch makes it inconsistent with vacuum full, which is strange
>> because vacuum full calls cluster.
>>
>> postgres=> VACUUM FULL t;
>> VACUUM
>> postgres=> CLUSTER t;
>> ERROR: must be owner of table t
> This is the existing behavior on HEAD. I think it has been this way for a
> while. Granted, that doesn't mean it's ideal, but AFAICT it's intentional.
>
> Looking closer, the database ownership check in
> get_tables_to_cluster_partitioned() appears to have no meaningful effect.
> In this code path, cluster_rel() will always be called with CLUOPT_RECHECK,
> and that function only checks for table ownership. Anything gathered in
> get_tables_to_cluster_partitioned() that the user doesn't own will be
> skipped. So, I don't think my patch changes the behavior in any meaningful
> way, but I still think it's worthwhile to make the checks consistent.
We should probably talk about what the privileges should be, though. I
think there's a case to be made that CLUSTER should be governed by the
VACUUM privileges, given how VACUUM FULL is now implemented.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com