On Mon, Jan 22, 2024 at 2:22 AM Andy Fan <zhihuifan1213@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