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
