On Thu, Jan 18, 2024 at 1:30 PM Andy Fan <zhihuifan1...@163.com> wrote: > > You added an #include to dynahash.c despite making no other changes to > > the file. > > That's mainly because I put the code into miscadmin.h and spin.h depends > on miscadmin.h with MACROs.
That's not a good reason. Headers need to include headers on which they depend; a .c file shouldn't be required to include one header because it includes another. > The LockBufHdr also used init_local_spin_delay / perform_spin_delay > infrastruce and then it has the same issue like ${subject}, it is pretty > like the code in s_lock; Based on my current knowledge, I think we > should add the check there. I'd like to hear from Andres, if possible. @Andres: Should these sanity checks apply only to spin locks per se, or also to buffer header locks? > they were put into spin.h in v1 but later move to miscadmin.h at [1]. > [1] > https://www.postgresql.org/message-id/CAEze2WggP-2Dhocmdhp-LxBzic%3DMXRgGA_tmv1G_9n-PDt2MQg%40mail.gmail.com I'm not entirely sure what the right thing to do is here, and the answer may depend on the previous question. But I disagree with Matthias -- I don't think miscadmin.h can be the right answer regardless. > START_SPIN_LOCK need to be macro since it use __FILE__ and __LINE__ to > note where the SpinLock is held. for others, just for consistent > purpose. I think they can be changed to inline function, at least for > VerifyNoSpinLocksHeld. Good point about __FILE__ and __LINE__. -- Robert Haas EDB: http://www.enterprisedb.com