> Hello,
> welcome to hackers with your first patch)
Thank you.
> Though, I got interested in the comment inconsistency you have found.
> I added debug message into this code branch of the patch and was able to
> see it in regression.diffs after 'make check':
> Speaking of your patch, it seems that the buffer was unpinned and pinned
> again between two reads,
> and the condition of holding it continuously has not been met.
May I ask what makes you conclude that the condition of holding the pin continuously has not been met?
Your reply encouraged me to dig a little bit more into this today. First, I wanted to check if indeed the pin was
continuouslyheld by the backend or not. I added some debug info to ReleaseBuffer for this: it turned out that the pin
onthe buffer was definitely never released by the backend between the calls to _bt_first and _bt_next. So the buffer
gotcompacted while the backend held a pin on it.
After some more searching I found the following code: _bt_vacuum_one_page in nbtinsert.c
This function compacts one single page without taking a super-exclusive lock. It is used during inserts to make room on
apage. I verified that if I comment out the calls to this function, the compacting never happens while I have a pin on
thebuffer.
So I guess that answers my own question: cleaning up garbage during inserts is one of the cases where compacting may
happeneven while other backends hold a pin to the buffer. Perhaps this should also be more clearly phrased in the
commentsin eg. _bt_killitems? Because currently those comments make it look like this case never occurs.
> While reading the code to answer your question, I noticed that
> BTScanPosIsPinned macro name is misleading.
> It calls BufferIsValid(), not BufferIsPinned() as one could expect.
> And BufferIsValid in bufmgr.h comment explicitly states that it
> shouldn't be confused with BufferIsPinned.
> The same goes for BTScanPosUnpinIfPinned().
I agree the name is misleading. It clearly does something else than how it's named. However, I don't believe this
introducesproblems in these particular pieces of code, as long as the macro's are always used. BTScanPosIsPinned
actuallychecks whether it's valid and not necessarily whether it's pinned, as you mentioned. However, any time the
buffergets unpinned using the macro BTScanPosUnpin, the buffer gets set to Invalid by the macro as well. Therefore, any
consecutivecall to BTScanPosIsPinned should indeed return false. It'd definitely be nice if this gets clarified in
commentsthough.
-Floris
