On Thu, Mar 16, 2023 at 07:04:16PM +0400, Ilya Gladyshev wrote:
> > 16 марта 2023 г., в 04:07, Justin Pryzby <pryzby@telsasoft.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
forDefineIndex (although, I don’t know whether all of them need progress tracking), for instance, there is ALTER TABLE
thatcalls DefineIndex to create index for constraints. So I feel like rearranging progress reporting will result in
unnecessarycode duplication in those call sites, so passing in an optional parameter seems to be easier here, if we are
goingto 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
partitionsseem to be locked on the first entry to DefineIndex there. All other call sites pass in the parentIndexId,
whichmeans 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
variablerelkind.
> 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
functionis 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
