On Tue, Dec 17, 2019 at 1:31 AM Kyotaro Horiguchi <horikyota....@gmail.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
