Re: CLUSTER on partitioned index - Mailing list pgsql-hackers

On Mon, Apr 11, 2022 at 09:06:09AM -0500, Justin Pryzby wrote:
> On Sat, Apr 02, 2022 at 07:21:11PM +0200, Alvaro Herrera wrote:
>> 1. in VACUUM FULL we only process partitions that are owned by the
>> invoking user.  We don't have this test in the new code.  I'm not sure
>> why do we do that there; is it worth doing the same here?

I think that adding a test is a good idea for such things.  Perhaps we
could have an isolation test, but what Justin is proposing seems good
enough to me for this goal.

> 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.

>> 2. We should silently skip a partition that's a foreign table, I
>> suppose.
>
> I think it's not needed, since the loop over index children doesn't see a child
> index on the foreign table?

Hmm.  That may be a sign to add an assertion, at least, or something
based on RELKIND_HAS_STORAGE().

I was wondering what 0001 was doing here as that's a separate issue,
but it looked fine so I have applied it.

+       /* Use a permanent memory context for the result list */
+       old_context = MemoryContextSwitchTo(cluster_context);
+
        rtc = (RelToCluster *) palloc(sizeof(RelToCluster));

Independently of the extra ownership check, the memory context
manipulation has to be fixed and the code shoudl switch to
RelToCluster only when saving an item.

+CREATE ROLE ptnowner;
Roles that are created in the regression tests need to be prefixed
with "regress_", or some buildfarm members will complain.  FWIW, I
enforce -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS in all my dev
builds.

I have added an open item for now, but the whole looks
straight-forward to me.
--
Michael