From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: 李杰(慎追) <adger.lj@alibaba-inc.com>
Cc: pgsql-hackers@lists.postgresql.org, 曾文旌(义从) <wenjing.zwj@alibaba-inc.com>
Cc: Zhihong Yu <zyu@yugabyte.com>
Subject: Re: CLUSTER on partitioned index
Date: Wed, 13 Apr 2022 05:52:14 -0500
Message-ID: <20220413105214.GL26620@telsasoft.com> (raw)
In-Reply-To: <YlZyp26LVVfmwfgW@paquier.xyz>
References: <202204021711.zf24w66t6o6w@alvherre.pgsql>
<202204021721.myr2alroeunz@alvherre.pgsql>
<20220411140609.GF26620@telsasoft.com>
<YlZyp26LVVfmwfgW@paquier.xyz>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, michael@paquier.xyz, alvherre@alvh.no-ip.org, adger.lj@alibaba-inc.com, wenjing.zwj@alibaba-inc.com, zyu@yugabyte.com
Subject: Re: CLUSTER on partitioned index
In-Reply-To: <20220413105214.GL26620@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox