MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Evgeny Potemkin Date:November 2 2010 1:36pm
Subject:Re: bzr commit into mysql-5.5-bugteam branch (epotemkin:3098) Bug#57278
View as plain text  
Hi Oystein,

Thanks for the review. Please, see my answers inline.

Regards, Evgen.

On 11/01/10 17:30, Øystein Grøvlen wrote:
> Hi Evgeny,
>
> I observe that this patch introduce changes in behavior:
>
> Before:
>
>  create table t1 (d  date , dt datetime , ts timestamp);
>  insert into t1 values (1110101,1110101,1110101);
>  Warnings:
>  Warning    1264    Out of range value for column 'd' at row 1
>  Warning    1264    Out of range value for column 'dt' at row 1
>  Warning    1265    Data truncated for column 'ts' at row 1
>  select * from t1;
>  d    dt    ts
>  0000-00-00    0000-00-00 00:00:00    0000-00-00 00:00:00
>
> Now:
>
>  create table t1 (d  date , dt datetime , ts timestamp);
>  insert into t1 values (1110101,1110101,1110101);
>  Warnings:
>  Warning    1265    Data truncated for column 'ts' at row 1
>  select * from t1;
>  d    dt    ts
>  0111-01-01    0111-01-01 00:00:00    0000-00-00 00:00:00
>
> Do you think this change is OK?  The different behavior between
> datetime and timestamp seems a bit inconsistent.
This are results on unmodified 5.5-bugfixing

mysql>  create table t1 (d  date , dt datetime , ts timestamp);
Query OK, 0 rows affected (0.30 sec)

mysql> insert into t1 values (1110101,1110101,1110101);
Query OK, 1 row affected, 3 warnings (0.01 sec)

mysql> insert into t1 values ('111-01-01','111-01-01','111-01-01');
Query OK, 1 row affected, 1 warning (0.01 sec)

mysql> show warnings;
+---------+------+---------------------------------------------+
| Level   | Code | Message                                     |
+---------+------+---------------------------------------------+
| Warning | 1264 | Out of range value for column 'ts' at row 1 |
+---------+------+---------------------------------------------+
1 row in set (0.00 sec)

So, here is inconsistency which actually caused the bug.
Strings are allowed to include '111-01-01', ints aren't.
And, btw, it the strings who violate. IMHO it's much worse
to disallow '111-01-01' since it would break apps.
As you can see, after the fix warnings became consistent for int and string values.

>
> AFAICT, there are currently no test case that tests your changes to
> Item_cache_datetime::val_str().  Test suite does not fail without
> these changes, and no test case triggers the new "if (null_value)"
> statement.  I think such a test case should be added.
Ok.
>
> I also think that it would be better if the check for an already
> cached null-value, was done at the top of the function like for other
> similar functions.  I think something like this should work:
>
>   if ((value_cached || str_value_cached) && null_value)
>     return NULL;
Done.
>
> It would be even better if cache_value() was written in a way that it
> would be possible to use the has_value() check here, too.  However, it
> probably requires a too big change for this bug fix.
has_value can't be used here. It will only cache value if it's not cached yet,
while val_str needs to convert already cached int value to a string.
>
> I really do not see the need for the extra complexity added by
> initializing null_value.  In case of an OOM error, I would think the
> contents of null_value would be the least concern.  I am more
> concerned that there seem to be no way for a caller to detect that an
> OOM error has occurred.
See, not all callers checks thd->error after calling item->val_xxx, but
all of them check item->null_value. If in case of OOM we will return NULL
but not set null_value caller will try to dereference NULL and crash.
Thus null_value have to be kept relevant.
>
> -- 
> Øystein
>
>
> On 10/27/10 05:07 PM, Evgeny Potemkin wrote:
> > #At file:///work/bzrroot/57278-bug-5.5-bugfixing/ based on 
> revid:marc.alff@stripped
> >
> >   3098 Evgeny Potemkin    2010-10-27
> >        Bug#57278: Crash on min/max + with date out of range.
> >        MySQL officially supports DATE values starting from 1000-01-01. This is
> >        enforced for int values, but not for string values, thus one
> >        could easily insert '0001-01-01' value. Int values are checked by
> >        number_to_datetime function and Item_cache_datetime::val_str uses it
> >        to fill MYSQL_TIME struct out of cached int value. This leads to the
> >        scenario where Item_cache_datetime caches a non-null datetime value 
> and when
> >        it tries to convert it from int to string number_to_datetime function
> >        treats the value as out-of-range and returns an error and
> >        Item_cache_datetime::val_str returns NULL for a non-null value. Due 
> to this
> >        inconsistency server crashes.
> >
> >        Now number_to_datetime allows DATE values below 1000-01-01 if the
> >        TIME_FUZZY_DATE flag is set. Item_cache_datetime::val_str now better 
> handles
> >        null_value.
> >       @ mysql-test/r/type_date.result
> >          Added a test case for the bug#57278.
> >       @ mysql-test/t/type_date.test
> >          Added a test case for the bug#57278.
> >       @ sql-common/my_time.c
> >          Bug#57278: Crash on min/max + with date out of range.
> >          Now number_to_datetime allows DATE values below 1000-01-01 if the
> >          TIME_FUZZY_DATE flag is set.
> >       @ sql/item.cc
> >          Bug#57278: Crash on min/max + with date out of range.
> >          Item_cache_datetime::val_str now better handles
> >          null_value.
> >
> >      modified:
> >        mysql-test/r/type_date.result
> >        mysql-test/t/type_date.test
> >        sql-common/my_time.c
> >        sql/item.cc
> > === modified file 'mysql-test/r/type_date.result'
> > --- a/mysql-test/r/type_date.result    2010-03-20 20:23:42 +0000
> > +++ b/mysql-test/r/type_date.result    2010-10-27 15:07:37 +0000
> > @@ -318,4 +318,12 @@ f1    date    YES        NULL
> >   f2    date    YES        NULL
> >   DROP TABLE t1;
> >   #
> > +#
> > +# Bug#57278: Crash on min/max + with date out of range.
> > +#
> > +set @a=(select min(makedate('111','1'))) ;
> > +select @a;
> > +@a
> > +0111-01-01
> > +#
> >   End of 6.0 tests
> >
> > === modified file 'mysql-test/t/type_date.test'
> > --- a/mysql-test/t/type_date.test    2010-03-20 20:23:42 +0000
> > +++ b/mysql-test/t/type_date.test    2010-10-27 15:07:37 +0000
> > @@ -284,4 +284,12 @@ DROP TABLE t1;
> >
> >   --echo #
> >
> > +--echo #
> > +--echo # Bug#57278: Crash on min/max + with date out of range.
> > +--echo #
> > +set @a=(select min(makedate('111','1'))) ;
> > +select @a;
> > +--echo #
> > +
> > +
> >   --echo End of 6.0 tests
> >
> > === modified file 'sql-common/my_time.c'
> > --- a/sql-common/my_time.c    2010-07-09 12:28:51 +0000
> > +++ b/sql-common/my_time.c    2010-10-27 15:07:37 +0000
> > @@ -1127,7 +1127,12 @@ longlong number_to_datetime(longlong nr,
> >       nr= (nr+19000000L)*1000000L;                 /* YYMMDD, year: 
> 1970-1999 */
> >       goto ok;
> >     }
> > -  if (nr<  10000101L)
> > +  /*
> > +    Though officially we support DATE values from 1000-01-01 only, one can
> > +    easily insert a value like 1-1-1. So, for consistency reasons such dates
> > +    are allowed when TIME_FUZZY_DATE is set.
> > +  */
> > +  if (nr<  10000101L&&  !(flags&  TIME_FUZZY_DATE))
> >       goto err;
> >     if (nr<= 99991231L)
> >     {
> >
> > === modified file 'sql/item.cc'
> > --- a/sql/item.cc    2010-10-08 14:06:31 +0000
> > +++ b/sql/item.cc    2010-10-27 15:07:37 +0000
> > @@ -7509,6 +7509,11 @@ String *Item_cache_datetime::val_str(Str
> >       if (value_cached)
> >       {
> >         MYSQL_TIME ltime;
> > +      /* No need to convert cached NULL. */
> > +      if (null_value)
> > +        return NULL;
> > +      /* Return NULL in case of OOM/conversion error. */
> > +      null_value= TRUE;
> >         if (str_value.alloc(MAX_DATE_STRING_REP_LENGTH))
> >           return NULL;
> >         if (cached_field_type == MYSQL_TYPE_TIME)
> > @@ -7538,6 +7543,7 @@ String *Item_cache_datetime::val_str(Str
> >         str_value.length(my_TIME_to_str(&ltime,
> > const_cast<char*>(str_value.ptr())));
> >         str_value_cached= TRUE;
> > +      null_value= FALSE;
> >       }
> >       else if (!cache_value())
> >         return NULL;
> >
> >
> >
> >
> >
>
>

Thread
bzr commit into mysql-5.5-bugteam branch (epotemkin:3098) Bug#57278Evgeny Potemkin27 Oct
  • Re: bzr commit into mysql-5.5-bugteam branch (epotemkin:3098) Bug#57278Øystein Grøvlen1 Nov
    • Re: bzr commit into mysql-5.5-bugteam branch (epotemkin:3098) Bug#57278Evgeny Potemkin2 Nov
      • Re: bzr commit into mysql-5.5-bugteam branch (epotemkin:3098) Bug#57278Øystein Grøvlen3 Nov