On Fri, Aug 30, 2019 at 07:45:57PM +0900, Masahiko Sawada wrote: > > I tried 1. and it shown index_rebuild_count, but it also shown > > "initializing" phase again and other columns were empty. So, it is > > not suitable to fix the problem. :( > > I'm going to try 2. and 3., but, unfortunately, I can't get enough > > time to do that after PGConf.Asia 2019. > > If we selected 3., it affects following these progress reporting > > features: VACUUM, CLUSTER, CREATE_INDEX and ANALYZE. But it's okay, > > I suppose. Any comments welcome! :) > > I looked at this open item. I prefer #3 but I think it would be enough > to have a small stack using a fixed length array to track nested > progress information and the current executing command (maybe 4 or 8 > would be enough for maximum nested level for now?). That way, we don't > need any change the interfaces. AFAICS there is no case where we call > only either pgstat_progress_start_command or > pgstat_progress_end_command without each other (although > pgstat_progress_update_param is called without start). So I think that > having a small stack for tracking multiple reports would work. > > Attached the draft patch that fixes this issue. Please review it.
Do we actually want to show to the user information about CREATE INDEX which is different than CLUSTER? It could be confusing for the user to see a progress report from a command different than the one actually launched. There could be a method 4 here: do not start a new command progress when there is another one already started, and do not try to end it in the code path where it could not be started as it did not stack. So while I see the advantages of stacking the progress records as you do when doing cascading calls of the progress reporting, I am not sure that: 1) We should bloat more PgBackendStatus for that. 2) We want to add more complication in this area close to the release of 12. Another solution as mentioned by Yamada-san is just to start again the progress for the cluster command in reindex_relation() however you got an issue here as reindex_relation() is also called by REINDEX TABLE. I find actually very weird that we have a cluster-related field added for REINDEX, so it seems to me that all the interactions between the code paths of CLUSTER and REINDEX have not been completely thought through. This part has been added by 6f97457 :( -- Michael
signature.asc
Description: PGP signature