Re: non-exclusive backup cleanup is mildly broken

On Tue, Dec 17, 2019 at 1:31 AM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
> The patch can cause removal of a wrong cleanup function on non-cassert
> build. That might be unwanted. But I think the assertion is needed
> anyway.

I agree with the first part of this critique, but not necessarily with
the second part. Right now, if you call cancel_before_shmem_exit(),
you might not remove the handler that you intended to remove, but you
won't remove some unrelated handler. With the patch, if assertions are
disabled, you will definitely remove something, but it might not be
the thing you intended to remove. That seems worse.

On the question of whether the assertion is needed, it is currently
the case that you could call cancel_before_shmem_exit() without
knowing whether you'd actually registered a handler or not. With the
proposed change, that would no longer be legal. Maybe that's OK. But
it doesn't seem entirely crazy to have some error-recovery path where
cancel_before_shmem_exit() could get called twice in obscure
circumstances, and right now, you can rest easy, knowing that the
second call just won't do anything. If we make it an assertion failure
to do such things, then you can't. On the other hand, maybe there's no
use for such a construct, and it'd be better to just reduce confusion.
Anyway, I think this is a separate topic from fixing this specific
problem.

Since there doesn't seem to be any opposition to my original fix,
except for the fact that I included a bug in it, I'm going to go see
about getting that committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company