On Fri, Dec 13, 2019 at 3:00 AM Michael Paquier <mich...@paquier.xyz> wrote: > Agreed, that's an issue and do_pg_abort_abort should not touch > sessionBackupState, so you should keep cancel_before_shmem_exit after > pg_stop_backup().
I don't understand this comment, because that can't possibly work. It assumes either that nobody else is allowed to use before_shmem_exit() after we do, or that cancel_before_shmem_exit() does something that it doesn't actually do. In general, before_shmem_exit() callbacks are intended to be persistent, and therefore it's the responsibility of the callback to test whether any work needs to be done. This particular callback is an exception, assuming that it can remove itself when there's no longer any work to be done. > Other than that, I have looked in details at how > safe it is to move before_shmem_exit(do_pg_abort_backup) before > do_pg_start_backup() and the cleanups of nonExclusiveBackups happen > safely and consistently in the event of an error during > pg_start_backup(). I came to the same conclusion, but I think it's still better to register the callback first. If the callback is properly written to do nothing when there's nothing to do, then having it registered earlier is harmless. And if, in the future, do_pg_start_backup() should be changed in such a way that, say, it can throw an error at the very end, then registering the handler first would prevent that from being a bug. It is generally more robust to register a cleanup handler in advance and then count on it to do the right thing than to try to write code that isn't allowed to fail in the wrong place. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
