Re: Add index scan progress to pg_stat_progress_vacuum
Sami Imseih <simseih@amazon.com>
From: "Imseih (AWS), Sami" <simseih@amazon.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Robert Haas <robertmhaas@gmail.com>, "Bossart, Nathan" <bossartn@amazon.com>, Peter Geoghegan <pg@bowt.ie>, Justin
Pryzby <pryzby@telsasoft.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2022-11-28T23:57:14Z
Lists: pgsql-hackers
Attachments
- v16-0001-Add-2-new-columns-to-pg_stat_progress_vacuum.-Th.patch (application/octet-stream) patch v16-0001
> I think that indexes_total should be 0 also when INDEX_CLEANUP is off. Patch updated for handling of INDEX_CLEANUP = off, with an update to the documentation as well. > I think we don't need to reset it at the end of index vacuuming. There > is a small window before switching to the next phase. If we reset this > value while showing "index vacuuming" phase, the user might get > confused. Instead, we can reset it at the beginning of the index > vacuuming. No, I think the way it's currently done is correct. We want to reset the number of indexes completed before we increase the num_index_scans ( index vacuum cycle ). This ensures that when we enter a new index cycle, the number of indexes completed are already reset. The 2 fields that matter here is how many indexes are vacuumed in the currently cycle and this is called out in the documentation as such. > We should add CHECK_FOR_INTERRUPTS() at the beginning of the loop to > make the wait interruptible. Done. > I think it would be better to update the counter only when the value > has been increased. Done. Did so by checking the progress value from the beentry directly in the function. We can do a more generalized > I think we should set a timeout, say 1 sec, to WaitLatch so that it > can periodically check the progress. Done. > Probably it's better to have a new wait event for this wait in order > to distinguish wait for workers to complete index vacuum from the wait > for workers to exit. I was trying to avoid introducing a new wait event, but thinking about it, I agree with your suggestion. I created a new ParallelVacuumFinish wait event and documentation for the wait event. > I think this doesn't work, since ivinfo.idx_completed is in the > backend-local memory. Instead, I think we can have a function in > vacuumparallel.c that updates the progress. Then we can have index AM > call this function. Yeah, you're absolutely correct. To make this work correctly, I did something similar to VacuumActiveNWorkers and introduced an extern variable called ParallelVacuumProgress. This variable points to pvs->shared->idx_completed_progress. The index AMs then call parallel_vacuum_update_progress which Is responsible for updating the progress with the current value of ParallelVacuumProgress. ParallelVacuumProgress is also set to NULL at the end of every index cycle. > ivinfo.report_parallel_progress = !IsParallelWorker(); Done Regards, Sami Imseih Amazon Web Services (AWS)
Commits
-
Report index vacuum progress.
- 46ebdfe164c6 17.0 landed
-
Add new parallel message type to progress reporting.
- f1889729dd3a 17.0 landed
-
Remove MaxBackends variable in favor of GetMaxBackends() function.
- aa64f23b0292 15.0 cited