Re: [HACKERS] CLUSTER command progress monitor

On Mon, Sep 16, 2019 at 03:26:10PM +0900, Tattsu Yama wrote:
> I should have explained the API changes more. I added cmdtype as a given
> parameter for all functions (See below).
> Therefore, I suppose that my patch is similar to the following fix as you
> mentioned on -hackers.

Yes, that's an option I mentioned here, but it has drawbacks:
https://www.postgresql.org/message-id/20190914024547.GB15406@paquier.xyz

I have just looked at it again after a small rebase and there are
issues with the design of your patch:
- When aborting a transaction, we need to enforce a reset of the
command ID used in st_progress_command to be PROGRESS_COMMAND_INVALID.
Unfortunately, your patch does not consider the case where an error
happens while a command ID is set, causing a command to still be
tracked with the next transactions of the session. Even worse, it
prevents pgstat_progress_start_command() to be called again in this
case for another command.
- CLUSTER can rebuild indexes, and we'd likely want to be able to
track some of the information from CREATE INDEX for CLUSTER.

The second issue is perhaps fine as it is not really straight-forward
to share the same progress phases across multiple commands, and we
could live without it for now, or require a follow-up patch to make
the information of CREATE INDEX available to CLUSTER.

Now, the first issue is of another caliber and a no-go :(

On HEAD, pgstat_progress_end_command() has the limitation to not be
able to stack multiple commands, so calling it in cascade has the
disadvantage to perhaps erase the progress state of a command (and it
is not designed for that anyway), which is what happens with CLUSTER
when reindex_index() starts a new progress report, but the simplicity
of the current infrastructure is very safe when it comes to failure
handling, to make sure that an reset happens as long as the command ID
is not invalid. Your patch makes that part unpredictable.
--
Michael