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.gb15...@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
signature.asc
Description: PGP signature
