On Thu, Jan 18, 2024 at 7:56 AM Andy Fan <zhihuifan1...@163.com> wrote: > Do you still have interest with making some progress on this topic?
Some, but it's definitely not my top priority. I wish I could give as much attention as everyone would like to everyone's patches, but I can't even come close. I think that the stack that you set up in START_SPIN_LOCK() is crazy. That would only be necessary if it were legal to acquire multiple spinlocks at the same time, which it definitely isn't. Also, doing memory allocation and deallocation here appears highly undesirable - even if we did need to support multiple spinlocks, it would be better to handle this using something like the approach we already use for lwlocks, where there is a fixed size array and we blow up if it overflows. ASSERT_NO_SPIN_LOCK() looks odd, because I would expect it to be spelled Assert(!AnySpinLockHeld()). But looking deeper, I see that it doesn't internally Assert() but rather does something else. Maybe the name needs some workshopping. SpinLockMustNotBeHeldHere()? VerifyNoSpinLocksHeld()? I think we should check that no spinlock is held in a few additional places: the start of SpinLockAcquire(), and the start of errstart(). You added an #include to dynahash.c despite making no other changes to the file. I don't know whether the choice to treat buffer header locks as spinlocks is correct. It seems like it at least deserves a comment, and possibly some discussion on this mailing list about whether that's the right idea. I'm not sure that we have all the same restrictions for buffer header locks as we do for spinlocks in general, but I'm also not sure that we don't. On a related note, the patch overall has 0 comments. I don't know that it needs a lot, but 0 isn't many at all. miscadmin.h doesn't seem like a good place for this. It's a widely-included header file and these checks should be needed in relatively few places; also, they're not really related to most of what's in that file, IIRC. I also wonder why we're using macros instead of static inline functions. -- Robert Haas EDB: http://www.enterprisedb.com