Re: non-exclusive backup cleanup is mildly broken

On Fri, Dec 13, 2019 at 3:00 AM Michael Paquier <michael(at)paquier(dot)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