> 1 февр. 2023 г., в 08:29, Justin Pryzby <pry...@telsasoft.com> написал(а): > > On Tue, Jan 31, 2023 at 07:32:20PM +0400, Ilya Gladyshev wrote: >>> 17 янв. 2023 г., в 23:44, Tomas Vondra <tomas.von...@enterprisedb.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.
v2-0001-create-index-progress-increment.patch
Description: Binary data
