On Thu, Jan 18, 2024 at 7:56 AM Andy Fan <zhihuifan1213@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
