I would like to go and implement this check now, and if all goes well
I may propose that you commit it to the master branch for v11. I don't
see this as a new feature. I see it as essential testing
infrastructure. What do you think about adding this new check soon?
Agree. Hope, nobody found a way how to use amcheck module in production
to serve user requests. But, it should be implemented before BETA stage,
in opposite case we will get a lot of objections.
Nevertheless, seems, I found. In _bt_mark_page_halfdead() we use truncated
high key IndexTuple as a storage of blocknumber of top parent to remove. And
sets BTreeTupleSetNAtts(&trunctuple, 0) - it's stored in ip_posid.
But some later, in _bt_unlink_halfdead_page() we check ItemPointer high key
with ItemPointerIsValid macro - and it returns false, because offset is
actually InvalidOffsetNumber - i.e. 0 which was set by BTreeTupleSetNAtts.
And some wrong decisions are follows, I didn't look at that.
I'm not at all surprised. I strongly suspected that it was some simple
issue with the representation, since the INCLUDE patch didn't actually
change the page deletion algorithm in any way.
+1
Trivial and naive fix is attached, but for me it looks a bit annoing that we
store pointer (leafhikey) somewhere inside unlocked page.
Well, it has worked that way for a long time. No reason to change it now.
Agree, but at least this place needs a comment - why it's safe.
I also think that we could have better conventional regression test
coverage here.
Will try to invent not so large test.
--
Teodor Sigaev E-mail: teo...@sigaev.ru
WWW: http://www.sigaev.ru/
