Re: Progress report of CREATE INDEX for nested partitioned tables - Mailing list pgsql-hackers
From | Ilya Gladyshev |
---|---|
Subject | Re: Progress report of CREATE INDEX for nested partitioned tables |
Date | |
Msg-id | DF2FEE0B-8D65-4926-B2E1-79C84363DBAC@gmail.com Whole thread Raw |
In response to | Re: Progress report of CREATE INDEX for nested partitioned tables (Matthias van de Meent <boekewurm+postgres@gmail.com>) |
Responses |
Re: Progress report of CREATE INDEX for nested partitioned tables
|
List | pgsql-hackers |
1 февр. 2023 г., в 20:27, Matthias van de Meent <boekewurm+postgres@gmail.com> написал(а):On Wed, 1 Feb 2023 at 16:53, Justin Pryzby <pryzby@telsasoft.com> wrote:
On Wed, Feb 01, 2023 at 04:21:35PM +0100, Matthias van de Meent wrote:On Wed, 1 Feb 2023 at 15:21, Ilya Gladyshev <ilya.v.gladyshev@gmail.com> wrote:1 февр. 2023 г., в 16:01, Alvaro Herrera <alvherre@alvh.no-ip.org> написал(а):
Hmm, count_leaf_partitions has to scan pg_inherits and do a syscache
lookup for every single element therein ... this sounds slow.
In one of the callsites, we already have the partition descriptor
available. We could just scan partdesc->is_leaf[] and add one for each
'true' value we see there.
The problem is that partdesc contains only direct children of the table and we need all the children down the inheritance tree to count the total number of leaf partitions in the first callsite.In the other callsite, we had the table open just a few lines before the
place you call count_leaf_partitions. Maybe we can rejigger things by
examining its state before closing it: if relkind is not partitioned we
know leaf_partitions=0, and only if partitioned we count leaf partitions.
I think that would save some work. I also wonder if it's worth writing
a bespoke function for counting leaf partitions rather than relying on
find_all_inheritors.
Sure, added this condition to avoid the extra work here.When creating an index on a partitioned table, this column is set to
- the total number of partitions on which the index is to be created.
+ the total number of leaf partitions on which the index is to be created or attached.
I think we should also add a note about the (now) non-constant nature
of the value, something along the lines of "This value is updated as
we're processing and discovering partitioned tables in the partition
hierarchy".
But the TOTAL is constant, right ? Updating the total when being called
recursively is the problem these patches fix.
If that's the case, then I'm not seeing the 'fix' part of the patch. I
thought this patch was fixing the provably incorrect TOTAL value where
DONE > TOTAL due to the recursive operation overwriting the DONE/TOTAL
values instead of updating them.
In HEAD we set TOTAL to whatever number partitioned table we're
currently processing has - regardless of whether we're the top level
statement.
With the patch we instead add the number of child relations to that
count, for which REL_HAS_STORAGE(child) -- or at least, in the v3
posted by Ilya. Approximately immediately after updating that count we
recurse to the child relations, and that only returns once it is done
creating the indexes, so both TOTAL and DONE go up as we process more
partitions in the hierarchy.
The TOTAL in the patch is set only when processing the top-level parent and it is not updated when we recurse, so yes, it is constant. From v3:
@@ -1219,8 +1243,14 @@ DefineIndex(Oid relationId,
Relation parentIndex;
TupleDesc parentDesc;
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
- nparts);
+ if (!OidIsValid(parentIndexId))
+ {
+ int total_parts;
+
+ total_parts = count_leaf_partitions(relationId);
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
+ total_parts);
+ }
It is set to the total number of children on all levels of the hierarchy, not just the current one, so the total value doesn’t need to be updated later, because it is set to the correct value from the very beginning.
It is the DONE counter that is updated, and when we attach an index of a partition that is itself a partitioned table (like a2 in your example, if it already had an index created), it will be updated by the number of children of the partition.
@@ -1431,9 +1463,25 @@ DefineIndex(Oid relationId,
SetUserIdAndSecContext(child_save_userid,
child_save_sec_context);
}
+ else
+ {
+ int attached_parts = 1;
+
+ if (RELKIND_HAS_PARTITIONS(child_relkind))
+ attached_parts = count_leaf_partitions(childRelid);
+
+ /*
+ * If the index was attached, we need to update progress
+ * here, in its parent. For a partitioned index, we need
+ * to mark all of its children that were included in
+ * PROGRESS_CREATEIDX_PARTITIONS_TOTAL as done. If the
+ * index was built by calling DefineIndex() recursively,
+ * the called function is responsible for updating the
+ * progress report for built indexes.
+ */
+ pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, attached_parts);
+ }
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
- i + 1);
free_attrmap(attmap);
}
pgsql-hackers by date: