On Wed, Sep 4, 2019 at 9:03 PM Michael Paquier <mich...@paquier.xyz> wrote: > For CLUSTER, the progress starts and ends in cluster_rel(). CLUSTER > uses its code paths at the beginning, but then things get more > complicated, particularly with finish_heap_swap() which calls directly > reindex_table(). 6f97457 includes one progress update at point which > can be a problem per its shared nature in reindex_relation() with > PROGRESS_CLUSTER_INDEX_REBUILD_COUNT. This last part is wrong IMO, > why should cluster reporting take priority in this code path, > enforcing anything else?
Oops. Yeah, that's bogus (as are some of the other things you mention). I think we're going to have to fix this by passing down some flags to these functions to tell them what kind of progress updates to do (or to do none). Or else pass down a callback function and a context object, but that seems like it might be overkill. > On top of those issues, I see some problems with the current state of > affairs, and I am repeating myself: > - It is possible that pgstat_progress_update_param() is called for a > given command for a code path taken by a completely different > command, and that we have a real risk of showing a status completely > buggy as the progress phases share the same numbers. > - We don't consider wisely end and start progress handling for > cascading calls, leading to a risk where we start command A, but > for shared code paths where we assume that only command B can run then > the processing abruptly ends for command A. Those are just weaknesses of the infrastructure. Perhaps there is a better solution, but there's no intrinsic reason that we can't avoid them by careful coding. > - Is it actually fine to report information about a command completely > different than the one provided by the client? It is for example > possible to call CLUSTER, but show up to the user progress report > about PROGRESS_COMMAND_CREATE_INDEX (see reindex_index). This could > actually make sense if we are able to handle cascading progress > reports. Well, it might be OK to do that if we're clear that this is the index progress-reporting view and the command is CLUSTER but it happens to be building an index now so we're showing it here. But I don't see how it would work anyway: you can't reported cascading progress reports in shared memory because you've only got a fixed amount of space. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
