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(<ime,
> > const_cast<char*>(str_value.ptr())));
> > str_value_cached= TRUE;
> > + null_value= FALSE;
> > }
> > else if (!cache_value())
> > return NULL;
> >
> >
> >
> >
> >
>
>
