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