From: Lars-Erik Bjørk
Date: February 11 2009 5:04pm
Subject: Re: bug#34478
List-Archive: http://lists.mysql.com/falcon/507
Message-Id: <5D185D8D-378A-49F0-A513-8AA568D51642@Sun.COM>
MIME-Version: 1.0
Content-Type: text/plain; delsp=yes; format=flowed; charset=ISO-8859-1
Content-Transfer-Encoding: QUOTED-PRINTABLE

Unless someone objects by tonight (Norwegian time), I will add some =
=20
comments and push :)
It has been reviewed by Vlad and John.

/Lars-Erik

On Feb 10, 2009, at 10:05 PM, Vladislav Vaintroub wrote:

> Thanks Lars-Eik,
> Other then strange alignment in comment, I'm comfortable with this =
=20
> change.
>
>> -----Original Message-----
>> From: Lars-Erik.Bjork@stripped [mailto:Lars-Erik.Bjork@stripped]
>> Sent: Tuesday, February 10, 2009 9:15 PM
>> To: Vladislav Vaintroub
>> Cc: 'Kevin Lewis'; 'FalconDev'; 'Jim Starkey'
>> Subject: Re: bug#34478
>>
>> Wow, that commit message came out a little strange :)
>>
>>
>> On Feb 10, 2009, at 9:07 PM, Vladislav Vaintroub wrote:
>>
>>> if (len < node1->keyLength)
>>>    =09=09   {
>>>    =09=09=09=09int n =3D checkTail(len, node1);
>>>
>>>    =09=09=09=09=09=09if (n)
>>>
> return n;
>>>
> }
>>>
>>> else if (len < node2->keyLength)
>>>
>>> {
>>>
>>> int n =3D checkTail(len, node2);
>>>
>>>
>>>
>>> if (n)
>>>
>>> return -n;
>>>
>>> }
>>>
>>> People think "use decent editor";) Was not Visual Studio availabl=
e =20
>>> on
>>> Macs???
>>>
>>>
>>>> -----Original Message-----
>>>> From: Lars-Erik.Bjork@stripped [mailto:Lars-Erik.Bjork@stripped]
>>>> Sent: Tuesday, February 10, 2009 9:05 PM
>>>> To: Vladislav Vaintroub
>>>> Cc: Kevin Lewis; FalconDev; Jim Starkey
>>>> Subject: Re: bug#34478
>>>>
>>>> I have committed a patch based on this, what does people think?
>>>>
>>>> http://lists.mysql.com/commits/65802
>>>>
>>>> /Lars-Erik
>>>>
>>>>
>>>> On Feb 10, 2009, at 1:20 PM, Vladislav Vaintroub wrote:
>>>>
>>>>> checkTail()/padding do not make sense to me. I would skip it or
>>>>> better yet,
>>>>> delete and see what happens.
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Lars-Erik.Bjork@stripped [mailto:Lars-Erik.Bjork@stripped]
>>>>>> Sent: Tuesday, February 10, 2009 1:16 PM
>>>>>> To: Vladislav Vaintroub
>>>>>> Cc: Kevin.Lewis@stripped; 'FalconDev'
>>>>>> Subject: Re: bug#34478
>>>>>>
>>>>>> Looking at the compare methods for the IndexKey class (used in
>>>>>> IndexPage::indexMerge), none of them pad the shorter key to th=
e
>>>>>> length
>>>>>> of the longer. They only compare on record number if the keys =
are
>>>>>> equal
>>>>>> up to the length of the shortest, and it is partial. Otherwise=
 it
>>>>>> compares on key length.
>>>>>>
>>>>>> Would this make sense for the DeferredIndex::compare methods a=
s
>>>> well?
>>>>>> Skip the checkTails() calls? Or will this make types with a
>>>> different
>>>>>> pad character than 0x00 sort incorrectly.
>>>>>>
>>>>>> The following method with the (for some reason) commented out =
=20
>>>>>> line
>>>>>> leads
>>>>>> me to believe that wen only use 0x00 for the mentioned padding
>>>>>> anyway:
>>>>>>
>>>>>> void Field::setCollation(Collation *newCollation)
>>>>>> {
>>>>>> collation =3D newCollation;
>>>>>> //indexPadByte =3D collation->getPadChar();
>>>>>> indexPadByte =3D 0;
>>>>>> }
>>>>>>
>>>>>>
>>>>>> I am not so familiar with what is the correct padding characte=
r
>> and
>>>>>> when
>>>>>> to pad etc. in all cases for every type, although the topic is
>> very
>>>>>> fascinating ;)
>>>>>>
>>>>>> Btw, Kevin, I tried removing the padding, and that seems to fi=
x
>> the
>>>>>> bug
>>>>>> you linked to (bug#33190), when enabling the assert again. So =
the
>>>>>> bugs
>>>>>> are definitively closely related.
>>>>>>
>>>>>> Best,
>>>>>>
>>>>>> Lars-Erik
>>>>>>
>>>>>> Vladislav Vaintroub wrote:
>>>>>>> I guess varbinary does not have any padding and this is the
>>>>>>> clue...
>>>>>>>
>>>>>>> The compare function for index keys does not exist as of toda=
y.
>>>>>>>
>>>>>>> It could look like
>>>>>>>
>>>>>>> diff =3D memcmp(a, b, min(length_a,length_b));
>>>>>>> if (diff =3D=3D 0)
>>>>>>> diff =3D (length_a > length_b)?1: ((length_a < length_b):-1:0=
);
>>>>>>>
>>>>>>>
>>>>>>> What makes me wonder in this bug is that IndexPage is not =
=20
>>>>>>> healthy
>>>>>> either.
>>>>>>>
>>>>>>> This could be effect of optimization that has been in index c=
ode
>>>>>> since ever:
>>>>>>> when merging a new key, use the fact that keys in log record =
are
>>>>>> presorted -
>>>>>>>
>>>>>>> findInsertionPoint does not search from the start of the page=
,
>> but
>>>>>> from the
>>>>>>> position where prior key has been inserted.
>>>>>>> And if keys are wrongly presorted, IndexPage could be easily
>>>>>> corrupted too.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Kevin.Lewis@stripped [mailto:Kevin.Lewis@stripped]
>>>>>>>> Sent: Monday, February 09, 2009 9:46 PM
>>>>>>>> To: Lars-Erik Bj=F8rk
>>>>>>>> Cc: FalconDev
>>>>>>>> Subject: Re: bug#34478
>>>>>>>>
>>>>>>>> What about making the DeferredIndex class sort things exactl=
y
>>>>>>>> like
>>>>>> the
>>>>>>>> IndexKeys read from index pages?  Can it use the same compar=
e
>>>>>> function?
>>>>>>>>
>>>>>>>> Lars-Erik Bj=F8rk wrote:
>>>>>>>>
>>>>>>>>> Hi all!
>>>>>>>>>
>>>>>>>>> I don't have the the battery capacity to write a long mail =
now
>>>>>> (lucky
>>>>>>>>> you!) :) I analyzed bug#34478 (
>>>>>>>>>
>>>>>>>> http://bugs.mysql.com/bug.php?id=3D34478)
>>>>>>>>
>>>>>>>>> which also narrows down to the use of 0x00 in index keys (t=
his
>>>>>>>>> time
>>>>>>>>>
>>>>>>>> as
>>>>>>>>
>>>>>>>>> pad bytes in the deferred index). Does anyone have an opini=
on
>> on
>>>>>> how
>>>>>>>>> this bug can be avoided in a nice and correct fashion? Does=
 it
>>>>>>>>> make
>>>>>>>>>
>>>>>>>> any
>>>>>>>>
>>>>>>>>> sense to pad with 0x00?
>>>>>>>>>
>>>>>>>>> 2 minutes capacity left, sorry the short mail, please see t=
he
>>>>>> report
>>>>>>>>>
>>>>>>>> for
>>>>>>>>
>>>>>>>>> a bit more elaborate version :)
>>>>>>>>>
>>>>>>>>> Best,
>>>>>>>>>
>>>>>>>>> Lars-Erik
>>>>>>>>>
>>>>>>>>>
>>>>>>>> --
>>>>>>>> Falcon Storage Engine Mailing List
>>>>>>>> For list archives: http://lists.mysql.com/falcon
>>>>>>>> To unsubscribe:
>>>> http://lists.mysql.com/falcon?unsub=3Dwlad@stripped
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Falcon Storage Engine Mailing List
>>>>>>> For list archives: http://lists.mysql.com/falcon
>>>>>>> To unsubscribe:    http://lists.mysql.com/falcon?unsub=3Dlars=
-
>>>>>> erik.bjork@stripped
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Falcon Storage Engine Mailing List
>>>>>> For list archives: http://lists.mysql.com/falcon
>>>>>> To unsubscribe:
>> http://lists.mysql.com/falcon?unsub=3Dwlad@stripped
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Falcon Storage Engine Mailing List
>>>>> For list archives: http://lists.mysql.com/falcon
>>>>> To unsubscribe:    http://lists.mysql.com/falcon?unsub=3Dlars-
>>>> erik.bjork@stripped
>>>>>
>>>>
>>>>
>>>> --
>>>> Falcon Storage Engine Mailing List
>>>> For list archives: http://lists.mysql.com/falcon
>>>> To unsubscribe:    http://lists.mysql.com/falcon?unsub=3Dwlad@su=
n.com
>>>
>>>
>
>
>
> --
> Falcon Storage Engine Mailing List
> For list archives: http://lists.mysql.com/falcon
> To unsubscribe:    http://lists.mysql.com/falcon?unsub=3Dlars-erik.=
bjork@stripped
>