List:Falcon Storage Engine« Previous MessageNext Message »
From:Vladislav Vaintroub Date:December 19 2008 1:25pm
Subject:RE: Fix for bug#41582
View as plain text  
>and I don't want to work under the assumption that everything is broken by
default :)

Based on 12 year experience : sooner or later you will work under that
assumption. Unless of course you personally write the code  from scratch. In
this case, programmer *after* will assume that everything is broken.


> -----Original Message-----
> From: Lars-Erik.Bjork@stripped [mailto:Lars-Erik.Bjork@stripped]
> Sent: Friday, December 19, 2008 2:11 PM
> To: Vladislav Vaintroub
> Cc: falcon@stripped
> Subject: Re: Fix for bug#41582
> 
> 
> On Dec 19, 2008, at 12:53 PM, Vladislav Vaintroub wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Lars-Erik.Bjork@stripped [mailto:Lars-Erik.Bjork@stripped]
> >> Sent: Friday, December 19, 2008 11:32 AM
> >> To: falcon@stripped
> >> Subject: Fix for bug#41582
> >>
> >> Hi all!
> >>
> >> I have a fix for the decimal/numeric bug 41582.
> >>
> >> As explained in the reference manual [1], decimals are stored using
> a
> >> binary format that packs nine decimal digits into four bytes, where
> >> any leftover digits require some fraction of four bytes.
> >>
> >> In StorageDatabase::getSegmentValue (yes, this method again(!)) we
> do
> >> a check on the precision, to see if the value is big enough to need
> >> special treatment:
> >>
> >>        else if (field->precision < 19 && field->scale ==
> 0)
> >>             {
> >>             int64 number = (signed char) (*ptr++ ^ 0x80);
> >>
> >>             for (int n = 1; n < length; ++n)
> >>                 number = (number << 8) | *ptr++;
> >>
> >>             if (number < 0)
> >>                 ++number;
> >>
> >>             value->setValue(number);
> >>             }
> >>         else
> >>             {
> >>             BigInt bigInt;
> >>             ScaledBinary::getBigIntFromBinaryDecimal((const char*)
> >> ptr, field->precision, field->scale, &bigInt);
> >>             value->setValue(&bigInt);
> >>             }
> >>
> >>         break;
> >>
> >> This test is obviously wrong., what we want to test for is:
> >>
> >> else if (field->precision <= 9 && field->scale == 0)
> >>
> >> This will fix the problem.
> >>
> >> Even better, we can avoid the test all together and treat all
> decimal
> >> keys as if they *may* have a precision greater than 9. This way
> there
> >> will be less code and we can avoid a cluttering 'if'. However, this
> >> will result in an additional function call or more (see the else
> >> clause above).
> >> What do you think? Am I allowed to remove the entire 'else if'?   :)
> >
> > If think additional function call is not a problem and should be
> > preferred
> > for reasons of simplicity and avoiding cluttering. Provided
> > ScaledBinary::getBigIntFromBinaryDecimal works correctly, which
> > might be a
> > big "if".
> >
> >
> 
> Given what happens further down the stack, it will probably be more
> like 5-6 extra function calls, or so. Testing and code reading
> suggests that ScaledBinary::getBigintFromBinaryDecimal works fine, and
> I don't want to work under the assumption that everything is broken by
> default :)
> 
> /Lars-Erik
> 
> >> Btw, when I am done with the fix for bug#40607, there will be some
> >> more rewrite in this area as well, but that is a different bug ...
> >>
> >>
> >> /Lars-Erik
> >>
> >> [1]  -  http://dev.mysql.com/doc/refman/6.0/en/precision-math-
> >> decimal-
> >> changes.html
> >>
> >>
> >> --
> >> 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
Fix for bug#41582Lars-Erik Bjørk19 Dec
  • Re: Fix for bug#41582Hakan Kuecuekyilmaz19 Dec
    • Re: Fix for bug#41582Lars-Erik Bjørk19 Dec
  • RE: Fix for bug#41582Vladislav Vaintroub19 Dec
    • Re: Fix for bug#41582Lars-Erik Bjørk19 Dec
      • RE: Fix for bug#41582Vladislav Vaintroub19 Dec