Date: Wed, 13 Apr 2022 05:52:14 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
	=?utf-8?B?5p2O5p2wKOaFjui/vSk=?= <adger.lj@alibaba-inc.com>,
	pgsql-hackers@lists.postgresql.org,
	=?utf-8?B?5pu+5paH5peMKOS5ieS7jik=?= <wenjing.zwj@alibaba-inc.com>,
	Zhihong Yu <zyu@yugabyte.com>
Subject: Re: CLUSTER on partitioned index
Message-ID: <20220413105214.GL26620@telsasoft.com>
References: <202204021711.zf24w66t6o6w@alvherre.pgsql>
 <202204021721.myr2alroeunz@alvherre.pgsql>
 <20220411140609.GF26620@telsasoft.com>
 <YlZyp26LVVfmwfgW@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YlZyp26LVVfmwfgW@paquier.xyz>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220413105214.GL26620%40telsasoft.com>
Precedence: bulk

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