> 16 марта 2023 г., в 04:07, Justin Pryzby <pry...@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 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.
