[Maria-developers] MDEV-5450 Assertion ... mysql_type_to_time_type(cached_field_type) == ltime.time_type' fails ...
Hi, Alexander! Ah, so at the end you've decided to fix the type, not the assert. Fine. Ok to push. On Jan 31, Alexander Barkov wrote:
Hi Sergei,
Please review a patch for mdev-5450.
Thanks.
=== modified file 'sql/item_timefunc.cc' --- sql/item_timefunc.cc 2013-12-16 12:02:21 +0000 +++ sql/item_timefunc.cc 2014-01-31 12:35:17 +0000 @@ -1480,12 +1480,42 @@ String *Item_temporal_func::val_str(Stri }
+bool Item_temporal_hybrid_func::fix_temporal_type(MYSQL_TIME *ltime) +{ + if (ltime->time_type < 0) /* MYSQL_TIMESTAMP_NONE, MYSQL_TIMESTAMP_ERROR */ + return false; + switch (field_type()) + { + case MYSQL_TYPE_TIME: + ltime->year= ltime->month= ltime->day= 0; + ltime->time_type= MYSQL_TIMESTAMP_TIME; + return false; + case MYSQL_TYPE_DATETIME: + case MYSQL_TYPE_TIMESTAMP: + ltime->neg= 0; + ltime->time_type= MYSQL_TIMESTAMP_DATETIME; + return false; + case MYSQL_TYPE_DATE: + ltime->neg= 0; + ltime->hour= ltime->minute= ltime->second= ltime->second_part= 0; + ltime->time_type= MYSQL_TIMESTAMP_DATE; + return false; + case MYSQL_TYPE_STRING: /* DATE_ADD, ADDTIME can return VARCHAR */ + return false; + default: + DBUG_ASSERT(0); + return true; + } + return false; +} + + String *Item_temporal_hybrid_func::val_str_ascii(String *str) { DBUG_ASSERT(fixed == 1); MYSQL_TIME ltime;
- if (get_date(<ime, 0) || + if (get_date(<ime, 0) || fix_temporal_type(<ime) || (null_value= my_TIME_to_str(<ime, str, decimals))) return (String *) 0;
Regards, Sergei
Hi Sergei, On 02/03/2014 07:44 PM, Sergei Golubchik wrote:
Hi, Alexander!
Ah, so at the end you've decided to fix the type, not the assert. Fine. Ok to push.
There were two problems actually: - the type - the assert I fixed both. Thanks for review! Btw, the problem with type can be back ported to 5.5 or 5.3.
On Jan 31, Alexander Barkov wrote:
Hi Sergei,
Please review a patch for mdev-5450.
Thanks.
=== modified file 'sql/item_timefunc.cc' --- sql/item_timefunc.cc 2013-12-16 12:02:21 +0000 +++ sql/item_timefunc.cc 2014-01-31 12:35:17 +0000 @@ -1480,12 +1480,42 @@ String *Item_temporal_func::val_str(Stri }
+bool Item_temporal_hybrid_func::fix_temporal_type(MYSQL_TIME *ltime) +{ + if (ltime->time_type < 0) /* MYSQL_TIMESTAMP_NONE, MYSQL_TIMESTAMP_ERROR */ + return false; + switch (field_type()) + { + case MYSQL_TYPE_TIME: + ltime->year= ltime->month= ltime->day= 0; + ltime->time_type= MYSQL_TIMESTAMP_TIME; + return false; + case MYSQL_TYPE_DATETIME: + case MYSQL_TYPE_TIMESTAMP: + ltime->neg= 0; + ltime->time_type= MYSQL_TIMESTAMP_DATETIME; + return false; + case MYSQL_TYPE_DATE: + ltime->neg= 0; + ltime->hour= ltime->minute= ltime->second= ltime->second_part= 0; + ltime->time_type= MYSQL_TIMESTAMP_DATE; + return false; + case MYSQL_TYPE_STRING: /* DATE_ADD, ADDTIME can return VARCHAR */ + return false; + default: + DBUG_ASSERT(0); + return true; + } + return false; +} + + String *Item_temporal_hybrid_func::val_str_ascii(String *str) { DBUG_ASSERT(fixed == 1); MYSQL_TIME ltime;
- if (get_date(<ime, 0) || + if (get_date(<ime, 0) || fix_temporal_type(<ime) || (null_value= my_TIME_to_str(<ime, str, decimals))) return (String *) 0;
Regards, Sergei
On 02/03/2014 07:51 PM, Alexander Barkov wrote:
Hi Sergei,
On 02/03/2014 07:44 PM, Sergei Golubchik wrote:
Hi, Alexander!
Ah, so at the end you've decided to fix the type, not the assert. Fine. Ok to push.
There were two problems actually:
- the type - the assert
I fixed both.
Sorry, I misunderstood something when replying to the previous letter. Let me clarify. There were two problems. 1. Item_func_add_time::get_date() might erroneously return MYSQL_TIMESTAMP_TIME in cases when field_type() is actually DATETIME. This is fixed by this change: - if (is_date) // TIMESTAMP function + if (cached_field_type == MYSQL_TYPE_DATETIME) 2. The problem with a crash on DBUG_ASSERT, which was causes by a wrong time_type returned from Item_temporal_hybrid_func::get_date(). This is fixed by adding a new method Item_temporal_hybrid_func::fix_temporal_type(). But the condition itself inside the DBUG_ASSERT() was not changed.
Thanks for review!
Btw, the problem with type can be back ported to 5.5 or 5.3.
On Jan 31, Alexander Barkov wrote:
Hi Sergei,
Please review a patch for mdev-5450.
Thanks.
=== modified file 'sql/item_timefunc.cc' --- sql/item_timefunc.cc 2013-12-16 12:02:21 +0000 +++ sql/item_timefunc.cc 2014-01-31 12:35:17 +0000 @@ -1480,12 +1480,42 @@ String *Item_temporal_func::val_str(Stri }
+bool Item_temporal_hybrid_func::fix_temporal_type(MYSQL_TIME *ltime) +{ + if (ltime->time_type < 0) /* MYSQL_TIMESTAMP_NONE, MYSQL_TIMESTAMP_ERROR */ + return false; + switch (field_type()) + { + case MYSQL_TYPE_TIME: + ltime->year= ltime->month= ltime->day= 0; + ltime->time_type= MYSQL_TIMESTAMP_TIME; + return false; + case MYSQL_TYPE_DATETIME: + case MYSQL_TYPE_TIMESTAMP: + ltime->neg= 0; + ltime->time_type= MYSQL_TIMESTAMP_DATETIME; + return false; + case MYSQL_TYPE_DATE: + ltime->neg= 0; + ltime->hour= ltime->minute= ltime->second= ltime->second_part= 0; + ltime->time_type= MYSQL_TIMESTAMP_DATE; + return false; + case MYSQL_TYPE_STRING: /* DATE_ADD, ADDTIME can return VARCHAR */ + return false; + default: + DBUG_ASSERT(0); + return true; + } + return false; +} + + String *Item_temporal_hybrid_func::val_str_ascii(String *str) { DBUG_ASSERT(fixed == 1); MYSQL_TIME ltime;
- if (get_date(<ime, 0) || + if (get_date(<ime, 0) || fix_temporal_type(<ime) || (null_value= my_TIME_to_str(<ime, str, decimals))) return (String *) 0;
Regards, Sergei
participants (2)
-
Alexander Barkov -
Sergei Golubchik