Re: Progress report of CREATE INDEX for nested partitioned tables

On Thu, Mar 16, 2023 at 07:04:16PM +0400, Ilya Gladyshev wrote:
> > 16 марта 2023 г., в 04:07, Justin Pryzby <pryzby(at)telsasoft(dot)com> написал(а):
> >
> > On Tue, Mar 14, 2023 at 06:58:14PM +0400, Ilya Gladyshev wrote:
> >>> The only change from the current patch is (3). (1) still calls
> >>> count_leaf_partitions(), but only once. I'd prefer that to rearranging
> >>> the progress reporting to set the TOTAL in ProcessUtilitySlow().
> >>
> >> As for reusing TOTAL calculated outside of DefineIndex, as I can see, ProcessUtilitySlow is not the only call site for DefineIndex (although, I don’t know whether all of them need progress tracking), for instance, there is ALTER TABLE that calls DefineIndex to create index for constraints. So I feel like rearranging progress reporting will result in unnecessary code duplication in those call sites, so passing in an optional parameter seems to be easier here, if we are going to optimize it, after all. Especially if back-patching is a non-issue.
> >
> > Yeah. See attached. I don't like duplicating the loop. Is this really
> > the right direction to go ?
> >
> > I haven't verified if the child tables are locked in all the paths which
> > would call count_leaf_partitions(). But why is it important to lock
> > them for this? If they weren't locked before, that'd be a pre-existing
> > problem...
> > <0001-fix-CREATE-INDEX-progress-report-with-nested-partiti.patch>
>
> I’m not sure what the general policy on locking is, but I have checked ALTER TABLE ADD INDEX, and the all the partitions seem to be locked on the first entry to DefineIndex there. All other call sites pass in the parentIndexId, which means the progress tracking machinery will not be initialized, so I think, we don’t need to do locking in count_leaf_partitions().

> The approach in the patch looks good to me. Some nitpicks on the patch:
> 1. There’s an unnecessary second call to get_rel_relkind in ProcessUtilitySlow, we can just use what’s in the variable relkind.
> 2. We can also combine else and if to have one less nested level like that:
> + else if (!RELKIND_HAS_PARTITIONS(child_relkind))
>
> 3. There was a part of the comment saying "If the index was built by calling DefineIndex() recursively, the called function is responsible for updating the progress report for built indexes.", I think it is still useful to have it there.

Thanks, I addressed (1) and (3). (2) is deliberate, to allow a place to
put the comment which is not specific to the !HAS_PARTITIONS case.

--
Justin