On Mon, Jan 22, 2024 at 2:22 AM Andy Fan <zhihuifan1...@163.com> wrote: > I used sigismember(&BlockSig, SIGQUIT) to detect if a process is doing a > quickdie, however this is bad not only because it doesn't work on > Windows, but also it has too poor performance even it impacts on > USE_ASSERT_CHECKING build only. In v8, I introduced a new global > variable quickDieInProgress to handle this.
OK, I like the split between 0001 and 0002. I still think 0001 has cosmetic problems, but if some committer wants to take it forward, they can decide what to do about that; you and I going back and forth doesn't seem like the right approach to sorting that out. Whether or not 0002 is adopted might affect what we do about the cosmetics in 0001, too. 0003 seems ... unfortunate. It seems like an admission that 0001 is wrong. Surely it *isn't* right to ignore the spinlock restrictions in quickdie() in general. For example, we could self-deadlock if we try to acquire a spinlock we already hold. If the problem here is merely the call in errstart() then maybe we need to rethink that particular call. If it goes any deeper than that, maybe we've got actual bugs we need to fix. + * It's likely to check the BlockSig to know if it is doing a quickdie + * with sigismember, but it is too expensive in test, so introduce + * quickDieInProgress to avoid that. This isn't very good English -- I realize that can sometimes be hard -- but also -- I don't think it likely that a future hacker would wonder why this isn't done that way. A static variable is normal for PostgreSQL; checking the signal mask would be a completely novel approach. So I think this comment is missing the mark topically. If this patch is right at all, the comment here should focus on why disabling these checks in quickdie() is necessary and appropriate, not why it's coded to match everything else in the system. -- Robert Haas EDB: http://www.enterprisedb.com