On Mon, Sep 14, 2020 at 09:31:03AM -0500, Justin Pryzby wrote:
> Anyway, for now this is rebased on 83158f74d.
I have not thought yet about all the details of CIC and DIC and what
you said upthread, but I have gone through CLUSTER for now, as a
start. So, the goal of the patch, as I would define it, is to give a
way to CLUSTER to work on a partitioned table using a given
partitioned index. Hence, we would perform CLUSTER automatically from
the top-most parent for any partitions that have an index on the same
partition tree as the partitioned index defined in USING, switching
indisclustered automatically depending on the index used.
+CREATE TABLE clstrpart3 PARTITION OF clstrpart DEFAULT PARTITION BY RANGE(a);
+CREATE TABLE clstrpart33 PARTITION OF clstrpart3 DEFAULT;
CREATE INDEX clstrpart_idx ON clstrpart (a);
-ALTER TABLE clstrpart CLUSTER ON clstrpart_idx
So.. For any testing of partitioned trees, we should be careful to
check if relfilenodes have been changed or not as part of an
operation, to see if the operation has actually done something.
From what I can see, attempting to use a CLUSTER on a top-most
partitioned table fails to work on child tables, but isn't the goal of
the patch to make sure that if we attempt to do the operation on a
partitioned table using a partitioned index, then the operation should
be done as well on the partitions with the partition index part of the
same partition tree as the parent partitioned index? If using CLUSTER
on a new partitioned index with USING, it seems to me that we may want
to check that indisclustered is correctly set for all the partitions
concerned in the regression tests. It would be good also to check if
we have a partition index tree that maps partially with a partition
table tree (aka no all table partitions have a partition index), where
these don't get clustered because there is no index to work on.
+ MemoryContext old_context = MemoryContextSwitchTo(cluster_context);
+ inhoids = find_all_inheritors(indexOid, NoLock, NULL);
+ MemoryContextSwitchTo(old_context);
Er, isn't that incorrect? I would have assumed that what should be
saved in the context of cluster is the list of RelToCluster items.
And we should not do any lookup of the partitions in a different
context, because this may do allocations of things we don't really
need to keep around for the context dedicated to CLUSTER. Isn't
NoLock unsafe here, even knowing that an exclusive lock is taken on
the parent? It seems to me that at least schema changes should be
prevented with a ShareLock in the first transaction building the list
of elements to cluster.
+ /*
+ * We have a full list of direct and indirect children, so skip
+ * partitioned tables and just handle their children.
+ */
+ if (get_rel_relkind(relid) == RELKIND_PARTITIONED_TABLE)
+ continue;
It would be better to use RELKIND_HAS_STORAGE here.
All this stuff needs to be documented clearly.
--
Michael
