> 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: teodor@sigaev.ru
WWW: http://www.sigaev.ru/