On Wed, Apr 13, 2022 at 03:50:15PM +0900, Michael Paquier wrote: > > > That dates to a556549d7 (see also cbe24a6dd8 for an earlier commit in > > CLUSTER > > itself). The reason was to avoid blocking if an unprivileged user runs > > VACUUM > > FULL which would try to lock things (including shared catalogs) before > > checking > > if they have permission to vacuum them. That commit also initially checks > > the > > owner of the partitioned table, and then re-checking owner of partitions > > later > > on. > > > > A similar issue exists here. But 1) catalog tables are not partitioned, > > and, > > 2) ownership of a partitioned table is checked immediately. So the problem > > can > > only occur if a user who owns a partitioned table but doesn't own all its > > partitions tries to cluster it, and it blocks behind another session. > > Fixing > > this is probably a good idea, but seems improbable that it would avoid a > > DOS. > > Catalogs are out of the picture as you say and I would not worry about > them becoming somewhat partitioned even in the far future. Are you > saying that it is possible for a user kicking a CLUSTER command on a > partitioned table who has no ownership on some of the partitions to > do some blocking table_open() calls if the permission check is not > done in get_tables_to_cluster_partitioned()? Hence, this user could > block the access to such partitions? I am not sure that we need to > add any new ownership checks here as CLUOPT_RECHECK gets added to the > parameters in cluster() before calling cluster_multiple_rels(), then > we do a mix of try_relation_open() with a skip when we are not the > owner anymore. So this logic looks sound to me. In short, you don't > need this extra check, and the test proposed in 0002 keeps the same > behavior.
Are you sure? The ownership re-check in cluster_rel() occurs after acquiring locks. s1: postgres=# CREATE TABLE p(i int) PARTITION BY LIST (i); postgres=# CREATE TABLE p1 PARTITION OF p FOR VALUES IN (1); postgres=# CREATE TABLE p2 PARTITION OF p FOR VALUES IN (2); postgres=# CREATE INDEX ON p (i); postgres=# CREATE ROLE po WITH LOGIN; postgres=# ALTER TABLE p OWNER TO po; postgres=# begin; SELECT FROM p1; s2: postgres=> SET client_min_messages =debug; postgres=> CLUSTER VERBOSE p USING p_i_idx ; LOG: process 26058 still waiting for AccessExclusiveLock on relation 39577 of database 5 after 1000.105 ms postgres=> SELECT 39577::regclass; regclass | p1
