Re: Progress report of CREATE INDEX for nested partitioned tables

> 1 февр. 2023 г., в 08:29, Justin Pryzby <pryzby(at)telsasoft(dot)com> написал(а):
>
> On Tue, Jan 31, 2023 at 07:32:20PM +0400, Ilya Gladyshev wrote:
>>> 17 янв. 2023 г., в 23:44, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> написал(а):
>>> Do we actually need the new parts_done field? I mean, we already do
>>> track the value - at PROGRESS_CREATEIDX_PARTITIONS_DONE index in the
>>> st_progress_param array. Can't we simply read it from there? Then we
>>> would not have ABI issues with the new field added to IndexStmt.
>>
>> I think it’s a good approach and it could be useful outside of scope of this patch too. So I have attached a patch, that introduces pgstat_progress_incr_param function for this purpose. There’s one thing I am not sure about, IIUC, we can assume that the only process that can write into MyBEEntry of the current backend is the current backend itself, therefore looping to get consistent reads from this array is not required. Please correct me, if I am wrong here.
>
> Thanks for the updated patch.
>
> I think you're right - pgstat_begin_read_activity() is used for cases
> involving other backends. It ought to be safe for a process to read its
> own status bits, since we know they're not also being written.
>
> You changed DefineIndex() to update progress for the leaf indexes' when
> called recursively. The caller updates the progress for "attached"
> indexes, but not created ones. That allows providing fine-granularity
> progress updates when using intermediate partitions, right ? (Rather
> than updating the progress by more than one at a time in the case of
> intermediate partitioning).
>
> If my understanding is right, that's subtle, and adds a bit of
> complexity to the current code, so could use careful commentary. I
> suggest:
>
> * If the index was attached, update progress for all its direct and
> * indirect leaf indexes all at once. If the index was built by calling
> * DefineIndex() recursively, the called function is responsible for
> * updating the progress report for built indexes.
>
> ...
>
> * If this is the top-level index, we're done. When called recursively
> * for child tables, the done partition counter is incremented now,
> * rather than in the caller.

Yes, you are correct about the intended behavior, I added your comments to the patch.

> I guess you know that there were compiler warnings (related to your
> question).
> https://cirrus-ci.com/task/6571212386598912
>
> pgstat_progress_incr_param() could call pgstat_progress_update_param()
> rather than using its own Assert() and WRITE_ACTIVITY calls. I'm not
> sure which I prefer, though.
>
> Also, there are whitespace/tab/style issues in
> pgstat_progress_incr_param().
>
> --
> Justin

Thank you for the review, I fixed the aforementioned issues in the v2.