List:Falcon Storage Engine« Previous MessageNext Message »
From:Lars-Erik Bjørk Date:February 11 2009 5:04pm
Subject:Re: bug#34478
View as plain text  
Unless someone objects by tonight (Norwegian time), I will add some  
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  
> 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)
>>>    		   {
>>>    				int n = checkTail(len, node1);
>>>
>>>    						if (n)
>>>
> return n;
>>>
> }
>>>
>>> else if (len < node2->keyLength)
>>>
>>> {
>>>
>>> int n = checkTail(len, node2);
>>>
>>>
>>>
>>> if (n)
>>>
>>> return -n;
>>>
>>> }
>>>
>>> People think "use decent editor";) Was not Visual Studio available  
>>> 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 the
>>>>>> 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 as
>>>> 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  
>>>>>> line
>>>>>> leads
>>>>>> me to believe that wen only use 0x00 for the mentioned padding
>>>>>> anyway:
>>>>>>
>>>>>> void Field::setCollation(Collation *newCollation)
>>>>>> {
>>>>>> collation = newCollation;
>>>>>> //indexPadByte = collation->getPadChar();
>>>>>> indexPadByte = 0;
>>>>>> }
>>>>>>
>>>>>>
>>>>>> I am not so familiar with what is the correct padding character
>> 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 fix
>> 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
> today.
>>>>>>>
>>>>>>> It could look like
>>>>>>>
>>>>>>> diff = memcmp(a, b, min(length_a,length_b));
>>>>>>> if (diff == 0)
>>>>>>> diff = (length_a > length_b)?1: ((length_a <
> length_b):-1:0);
>>>>>>>
>>>>>>>
>>>>>>> What makes me wonder in this bug is that IndexPage is not  
>>>>>>> healthy
>>>>>> either.
>>>>>>>
>>>>>>> This could be effect of optimization that has been in index
> code
>>>>>> 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ørk
>>>>>>>> Cc: FalconDev
>>>>>>>> Subject: Re: bug#34478
>>>>>>>>
>>>>>>>> What about making the DeferredIndex class sort things
> exactly
>>>>>>>> like
>>>>>> the
>>>>>>>> IndexKeys read from index pages?  Can it use the same
> compare
>>>>>> function?
>>>>>>>>
>>>>>>>> Lars-Erik Bjørk 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=34478)
>>>>>>>>
>>>>>>>>> which also narrows down to the use of 0x00 in index
> keys (this
>>>>>>>>> time
>>>>>>>>>
>>>>>>>> as
>>>>>>>>
>>>>>>>>> pad bytes in the deferred index). Does anyone have an
> opinion
>> 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 the
>>>>>> 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=1
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Falcon Storage Engine Mailing List
>>>>>>> For list archives: http://lists.mysql.com/falcon
>>>>>>> To unsubscribe:    http://lists.mysql.com/falcon?unsub=1
>>>>>> erik.bjork@stripped
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Falcon Storage Engine Mailing List
>>>>>> For list archives: http://lists.mysql.com/falcon
>>>>>> To unsubscribe:
>> http://lists.mysql.com/falcon?unsub=1
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Falcon Storage Engine Mailing List
>>>>> For list archives: http://lists.mysql.com/falcon
>>>>> To unsubscribe:    http://lists.mysql.com/falcon?unsub=1
>>>> erik.bjork@stripped
>>>>>
>>>>
>>>>
>>>> --
>>>> Falcon Storage Engine Mailing List
>>>> For list archives: http://lists.mysql.com/falcon
>>>> To unsubscribe:    http://lists.mysql.com/falcon?unsub=1
>>>
>>>
>
>
>
> --
> Falcon Storage Engine Mailing List
> For list archives: http://lists.mysql.com/falcon
> To unsubscribe:    http://lists.mysql.com/falcon?unsub=1
>

Thread
bug#34478Lars-Erik Bjørk9 Feb
  • Re: bug#34478Kevin Lewis9 Feb
    • RE: bug#34478Vladislav Vaintroub9 Feb
      • Re: bug#34478Lars-Erik Bjørk10 Feb
        • RE: bug#34478Vladislav Vaintroub10 Feb
          • Re: bug#34478Lars-Erik Bjørk10 Feb
            • RE: bug#34478Vladislav Vaintroub10 Feb
              • Re: bug#34478Lars-Erik Bjørk10 Feb
                • RE: bug#34478Vladislav Vaintroub10 Feb
                  • Re: bug#34478Lars-Erik Bjørk11 Feb