April 2011 - developers - lists.mariadb.org

[Maria-developers] Review of microsecond patch
by Michael Widenius 19 May '11

19 May '11

Hi! Here is finally a review of the microsecond patch. (Took me 5 days to complete! Lots of code!!!) I got this by doing bzr diff -r tag:mysql-5.1.54..-1 in the 5.1-micro tree. > === modified file 'client/mysqltest.cc' > --- client/mysqltest.cc 2010-11-22 09:21:10 +0000 > +++ client/mysqltest.cc 2011-02-23 17:26:02 +0000 > @@ -7342,7 +7342,8 @@ int util_query(MYSQL* org_mysql, const c > cur_con->util_mysql= mysql; > } > > - return mysql_query(mysql, query); > + int ret= mysql_query(mysql, query); > + DBUG_RETURN(ret); > } Not necessary change, but not important enough to revert. > === modified file 'include/my_sys.h' <cut> > +#define my_micro_time() (my_getsystime()/10) I am not happy with the above change. The old my_micro_time() was defined to be significantly faster than getsystime() for some systems (especially windows and solaris) and we should consider keeping the old implementation.... <long phone conversation> Conclusion: my_getsystime() is not depending on the clock and only to be used when calculating intervals. > +#define hrtime_to_time(X) ((X).val/1000000) > +#define hrtime_from_time(X) ((ulonglong)((X)*1000000ULL)) > +#define hrtime_to_double(X) ((X).val/1e6) I think you need to add a cast to double avoid compiler warnings on all platforms. (check buildbot!) <cut> > > === modified file 'include/my_time.h' > --- include/my_time.h 2008-04-03 17:14:57 +0000 > +++ include/my_time.h 2011-03-26 10:59:34 +0000 > @@ -55,6 +55,7 @@ typedef long my_time_t; > /* Flags to str_to_datetime */ > #define TIME_FUZZY_DATE 1 > #define TIME_DATETIME_ONLY 2 > +#define TIME_TIME_ONLY 4 > /* Must be same as MODE_NO_ZERO_IN_DATE */ > #define TIME_NO_ZERO_IN_DATE (65536L*2*2*2*2*2*2*2) > /* Must be same as MODE_NO_ZERO_DATE */ > @@ -68,8 +69,9 @@ typedef long my_time_t; > #define TIME_MAX_HOUR 838 > #define TIME_MAX_MINUTE 59 > #define TIME_MAX_SECOND 59 > +#define TIME_MAX_SECOND_PART 999999 > #define TIME_MAX_VALUE (TIME_MAX_HOUR*10000 + TIME_MAX_MINUTE*100 + \ > - TIME_MAX_SECOND) > + TIME_MAX_SECOND + TIME_MAX_SECOND_PART/1e6) Make 1e6 a constant and use this in my_sys.h and here. > #define TIME_MAX_VALUE_SECONDS (TIME_MAX_HOUR * 3600L + \ > TIME_MAX_MINUTE * 60L + TIME_MAX_SECOND) > > @@ -80,16 +82,17 @@ str_to_datetime(const char *str, uint le > uint flags, int *was_cut); > longlong number_to_datetime(longlong nr, MYSQL_TIME *time_res, > uint flags, int *was_cut); > +int number_to_time(double nr, MYSQL_TIME *ltime, int *was_cut); > ulonglong TIME_to_ulonglong_datetime(const MYSQL_TIME *); > ulonglong TIME_to_ulonglong_date(const MYSQL_TIME *); > ulonglong TIME_to_ulonglong_time(const MYSQL_TIME *); > ulonglong TIME_to_ulonglong(const MYSQL_TIME *); > +double TIME_to_double(const MYSQL_TIME *my_time); > > +longlong pack_time(MYSQL_TIME *my_time); > +MYSQL_TIME *unpack_time(longlong packed, MYSQL_TIME *my_time); > > -my_bool str_to_time(const char *str,uint length, MYSQL_TIME *l_time, > - int *warning); > - > -int check_time_range(struct st_mysql_time *, int *warning); > +int check_time_range(struct st_mysql_time *my_time, uint dec, int *warning); > > long calc_daynr(uint year,uint month,uint day); > uint calc_days_in_year(uint year); > @@ -136,11 +139,30 @@ void set_zero_time(MYSQL_TIME *tm, enum > sent using binary protocol fit in this buffer. > */ > #define MAX_DATE_STRING_REP_LENGTH 30 > +#define MAX_SEC_PART_VALUE 999999 > +#define MAX_SEC_PART_DIGITS 6 > +#define AUTO_SEC_PART_DIGITS 31 /* same as NOT_FIXED_DEC */ Do we really need to have MAX_SEC_PART_VALUE and TIME_MAX_SECOND_PART ? Shouldn't these always be the same for mysys functions? Also MAX_SEC_PART_DIGITS should be changed to TIME_MAX_SECOND_PART_DIGITS <cut> > === modified file 'mysql-test/include/ps_conv.inc' > @@ -1153,19 +1146,19 @@ select '-- select .. where date/time col > set @arg00= '1991-01-01 01:01:01' ; > select 'true' as found from t9 > where c1= 20 and c13= CAST('1991-01-01 01:01:01' AS DATE) and c14= '1991-01-01 01:01:01' and > - c15= '1991-01-01 01:01:01' and c16= '1991-01-01 01:01:01' and > + c15= '1991-01-01 01:01:01' and You need to add at least one test that shows the failure of doing a comparison between datetime and time. > === added file 'mysql-test/include/type_hrtime.inc' > --- mysql-test/include/type_hrtime.inc 1970-01-01 00:00:00 +0000 > +++ mysql-test/include/type_hrtime.inc 2011-03-01 12:24:36 +0000 > @@ -0,0 +1,94 @@ > + > +--source include/have_innodb.inc > + > +--disable_warnings > +drop table if exists t1, t2, t3; > +--enable_warnings > + > +--error ER_TOO_BIG_PRECISION > +eval create table t1 (a $type(7)); > + > +eval create table t1 (a $type(3)); > +insert t1 values ('2010-12-11 01:02:03.4567'); > +insert t1 values (20101211010203.45678); > +insert t1 values (20101211030405.789e0); > +insert t1 values (99991231235959e1); > +select * from t1; > +select truncate(a, 6) from t1; # Field::val_real() > +select a DIV 1 from t1; # Field::val_int() > +select group_concat(distinct a) from t1; # Field::cmp() > +alter table t1 engine=innodb; > +select * from t1; > +drop table t1; > +eval create table t1 (a $type(4)) engine=innodb; > +insert t1 values ('2010-12-11 01:02:03.456789'); > +select * from t1; > +select extract(microsecond from a + interval 100 microsecond) from t1 where a>'2010-11-12 01:02:03.456'; > +select a from t1 where a>'2010-11-12 01:02:03.456' group by a; Shouldn't we do the above for all engines ? If you can, please add at test of this in suite/engines/time.test It would be good to add a combinations file: [xtradb] --default-engine=innodb [aira] --default-engine=aria [myisam] --default-engine=myisam [heap] --default-engine=heap The problem I see with this is: - How to ignore running tests if an engine doesn't exist? - How to handle different result files ? - Adding the combination name to the result file may be a good temporary solution until we can do something based on 'diff' <cut> > +# > +# Views > +# > + > +create view v1 as select * from t1 group by a,b; > +select * from v1; > +show columns from v1; > +create table t2 select * from v1; > +show create table t2; Please also select some data from t2. > + > +drop view v1; > +drop table t1, t2; Good set of tests. The things that was not tested, as far as I can see: - INSERT SELECT between tables that has different column types. It would be good to test all common combinations. date -> datetime date -> string time -> datetime time -> string datetime -> time datetime -> date datetime -> timestamp datetime -> string timestamp -> datetime timestamp -> date timestamp -> time timestamp -> string string -> date string -> datetime string -> time string -> timestamp Doing this with different microsecond precission would be also good And the same with alter table. > === modified file 'mysql-test/r/cast.result' > --- mysql-test/r/cast.result 2009-05-21 08:06:43 +0000 > +++ mysql-test/r/cast.result 2011-03-18 13:52:20 +0000 > @@ -254,7 +254,7 @@ cast("2001-1-1" as datetime) = "2001-01- > 1 > select cast("1:2:3" as TIME) = "1:02:03"; > cast("1:2:3" as TIME) = "1:02:03" > -0 > +1 > select cast(NULL as DATE); > cast(NULL as DATE) > NULL > @@ -452,3 +452,9 @@ SELECT CONVERT(t2.a USING UTF8) FROM t1, > 1 > DROP TABLE t1; > End of 5.1 tests > +create table t1 (f1 time, f2 date, f3 datetime); > +insert into t1 values ('11:22:33','2011-12-13','2011-12-13 11:22:33'); > +select cast(f1 as unsigned), cast(f2 as unsigned), cast(f3 as unsigned) from t1; > +cast(f1 as unsigned) cast(f2 as unsigned) cast(f3 as unsigned) > +112233 20111213 20111213112233 > +drop table t1; Please also add a field datetime with microseconds to test cast to unsigned and double. > === modified file 'mysql-test/r/date_formats.result' > --- mysql-test/r/date_formats.result 2010-11-12 10:12:15 +0000 > +++ mysql-test/r/date_formats.result 2011-03-08 18:41:58 +0000 > @@ -196,16 +196,16 @@ date format datetime > 0003-01-02 8:11:2.123456 %Y-%m-%d %H:%i:%S.%# 0003-01-02 08:11:02 > 03-01-02 8:11:2.123456 %Y-%m-%d %H:%i:%S.%# 2003-01-02 08:11:02 > 2003-01-02 10:11:12 PM %Y-%m-%d %h:%i:%S %p 2003-01-02 22:11:12 > -2003-01-02 01:11:12.12345AM %Y-%m-%d %h:%i:%S.%f%p 2003-01-02 01:11:12.123450 > -2003-01-02 02:11:12.12345AM %Y-%m-%d %h:%i:%S.%f %p 2003-01-02 02:11:12.123450 > -2003-01-02 12:11:12.12345 am %Y-%m-%d %h:%i:%S.%f%p 2003-01-02 00:11:12.123450 > +2003-01-02 01:11:12.12345AM %Y-%m-%d %h:%i:%S.%f%p 2003-01-02 01:11:12 > +2003-01-02 02:11:12.12345AM %Y-%m-%d %h:%i:%S.%f %p 2003-01-02 02:11:12 > +2003-01-02 12:11:12.12345 am %Y-%m-%d %h:%i:%S.%f%p 2003-01-02 00:11:12 Why has microseconds been removed from the output ? It looks like 'cast(str_to_date(date, format) as datetime)' removes all microseconds, which is an incompatible change. In the past cast(..., datetime) as same as cast(..., datetime(6)) You should at least add a new test that uses 'cast(str_to_date(date, format) as datetime(6))' Please also compile a list for the KB about all incompatible changes. <cut> I think that it's more important that cast() produces the same value as MySQL did than that create ... select cast() produces the same type as before. With the first issue, you will get lost data (without any warnings) for old applictions if they would do things like: select * from t1 where cast(char_column as time) = "12:23:24.123456"; which works with MySQL but not with this patch. > === modified file 'mysql-test/r/func_in.result' > --- mysql-test/r/func_in.result 2010-06-22 18:53:08 +0000 > +++ mysql-test/r/func_in.result 2011-03-01 12:24:36 +0000 > @@ -544,15 +544,9 @@ id select_type table type possible_keys > select f2 from t2 where f2 in ('a','b'); > f2 > 0 > -Warnings: > -Warning 1292 Truncated incorrect DOUBLE value: 'a' > -Warning 1292 Truncated incorrect DOUBLE value: 'b' > explain select f2 from t2 where f2 in ('a','b'); > id select_type table type possible_keys key key_len ref rows Extra > 1 SIMPLE t2 index t2f2 t2f2 5 NULL 3 Using where; Using index > -Warnings: > -Warning 1292 Truncated incorrect DOUBLE value: 'a' > -Warning 1292 Truncated incorrect DOUBLE value: 'b' Good that we got rid of confusing internal error messages! > === modified file 'mysql-test/r/func_sapdb.result' > --- mysql-test/r/func_sapdb.result 2010-08-13 13:05:46 +0000 > +++ mysql-test/r/func_sapdb.result 2011-03-08 18:41:58 +0000 > @@ -113,10 +113,10 @@ subtime("01:00:00.999999", "02:00:00.999 > -00:59:59.999999 > select subtime("02:01:01.999999", "01:01:01.999999"); > subtime("02:01:01.999999", "01:01:01.999999") > -01:00:00.000000 > +01:00:00 Add also a test for: select subtime("02:01:01.999999", "01:01:01.999998"); To ensure we don't drop the sub seconds. > === modified file 'mysql-test/r/func_time.result' > --- mysql-test/r/func_time.result 2010-10-31 16:04:38 +0000 > +++ mysql-test/r/func_time.result 2011-03-19 14:49:36 +0000 > @@ -8,20 +8,20 @@ period_add("9602",-12) period_diff(19950 <cut> > +1994-03-02 10:11:12 1994-03-02 10:11:12 19940302101112 > select sec_to_time(9001),sec_to_time(9001)+0,time_to_sec("15:12:22"), > sec_to_time(time_to_sec("0:30:47")/6.21); > sec_to_time(9001) sec_to_time(9001)+0 time_to_sec("15:12:22") sec_to_time(time_to_sec("0:30:47")/6.21) > -02:30:01 23001.000000 54742 00:04:57 > +02:30:01 23001 54742 00:04:57.423510 Please add test for 'sec_to_time(9001.1)' and 'time_to_sec("15:12:22.123456')' and 'time_to_sec(15.55)'. I noticed that: select time_to_sec(15.55023124); -> 15.55023100 select time_to_sec('15.55023124'); -> 15.550231 Wouldn't this be better to limit the first one also to 6 digits? (Serg agreed to this on IRC) Add test for above cases too > @@ -798,9 +798,7 @@ TIMESTAMPDIFF(year,'2006-01-10 14:30:28' > 2 > select date_add(time,INTERVAL 1 SECOND) from t1; > date_add(time,INTERVAL 1 SECOND) > -NULL > -Warnings: > -Warning 1264 Out of range value for column 'time' at row 1 > +06:07:09 Add a note to compatible matrix that 'date_add()' now also works for time types. > @@ -941,10 +939,10 @@ sec_to_time(1) + 0, from_unixtime(1) + 0 > show create table t1; > Table Create Table > t1 CREATE TABLE `t1` ( > - `now() - now()` double(23,6) NOT NULL DEFAULT '0.000000', > - `curtime() - curtime()` double(23,6) NOT NULL DEFAULT '0.000000', > - `sec_to_time(1) + 0` double(23,6) DEFAULT NULL, > - `from_unixtime(1) + 0` double(23,6) DEFAULT NULL > + `now() - now()` double(17,0) NOT NULL DEFAULT '0', > + `curtime() - curtime()` double(17,0) NOT NULL DEFAULT '0', > + `sec_to_time(1) + 0` double(17,0) DEFAULT NULL, > + `from_unixtime(1) + 0` double(17,0) DEFAULT NULL > ) ENGINE=MyISAM DEFAULT CHARSET=latin1 To make example complete, add to the original create time some decimals for sec_to_time and also some decimals to one of the now() arguments. The CREATE was: create table t1 select now() - now(), curtime() - curtime(), sec_to_time(1) + 0, from_unixtime(1) + 0; As part of this, I noticed that from_unixtime() and unix_timestamp() is not enhanced to support microseconds. This needs to work: create table t2 (a int, b timestamp(6)) insert into t2 (a) values(1); select * from t2; I got '1 | 2011-03-29 15:38:53.395516' select unix_timestamp(b) from t2; -> 1301402333 (no decimals) This needs to returns sub seconds! I tested that: set timestamp=1301402333.55; select now(6); works (good)! <cut> > +create table t1 (f1 integer, f2 date); > +insert into t1 values (1,'2011-05-05'),(2,'2011-05-05'),(3,'2011-05-05'),(4,'2011-05-05'),(5,'2011-05-05'); > +select * from t1 where 1 and concat(f2)=MAKEDATE(2011, 125); > +f1 f2 > +1 2011-05-05 > +2 2011-05-05 > +3 2011-05-05 > +4 2011-05-05 > +5 2011-05-05 Add also at least one value for which there is no match. <cut> > --- mysql-test/r/func_time_hires.result 1970-01-01 00:00:00 +0000 > +++ mysql-test/r/func_time_hires.result 2011-03-17 13:13:03 +0000 > @@ -0,0 +1,177 @@ > +set timestamp=unix_timestamp('2011-01-01 01:01:01') + 0.123456, time_zone='+03:00'; > +select sec_to_time(12345), sec_to_time(12345.6789), sec_to_time(1234567e-2); > +sec_to_time(12345) 03:25:45 > +sec_to_time(12345.6789) 03:25:45.6789 > +sec_to_time(1234567e-2) 03:25:45.670000 > +select now(), curtime(0), utc_timestamp(1), utc_time(2), current_time(3), > +current_timestamp(4), localtime(5), localtimestamp(6), time_to_sec('12:34:56'), > +time_to_sec('12:34:56.789'); > +now() 2011-01-01 01:01:01 > +curtime(0) 01:01:01 > +utc_timestamp(1) 2010-12-31 22:01:01.1 > +utc_time(2) 22:01:01.12 > +current_time(3) 01:01:01.123 > +current_timestamp(4) 2011-01-01 01:01:01.1234 > +localtime(5) 2011-01-01 01:01:01.12345 > +localtimestamp(6) 2011-01-01 01:01:01.123456 > +time_to_sec('12:34:56') 45296 > +time_to_sec('12:34:56.789') 45296.789 > +select sec_to_time(time_to_sec('1:2:3')), sec_to_time(time_to_sec('2:3:4.567890')); > +sec_to_time(time_to_sec('1:2:3')) 01:02:03 > +sec_to_time(time_to_sec('2:3:4.567890')) 02:03:04.567890 > +select time_to_sec(sec_to_time(11111)), time_to_sec(sec_to_time(11111.22222)); > +time_to_sec(sec_to_time(11111)) 11111 > +time_to_sec(sec_to_time(11111.22222)) 11111.22222 > +select current_timestamp(7); > +ERROR HY000: Incorrect arguments to now <cut> > +select CAST(@a AS DATETIME(7)); > +ERROR 42000: Too big precision 7 specified for column '(@a)'. Maximum is 6. The above would be a better error message than 'Incorrect arguments to now'. Can you add precision and maximum to this error message? > +select time(f1) from t1 union all select time(f1) from t1; Add a time operation on the second time(f1) to make the rows a bit difference (and test more code) > +time(f1) > +21:00:00.000000 > +21:00:00.000000 > +drop table t1; > === modified file 'mysql-test/r/information_schema.result' > --- mysql-test/r/information_schema.result 2010-06-23 16:25:31 +0000 > +++ mysql-test/r/information_schema.result 2011-03-01 12:24:36 +0000 > @@ -189,8 +189,8 @@ Field Type Collation Null Key Default Ex > c varchar(64) utf8_general_ci NO select,insert,update,references > select * from information_schema.COLUMNS where table_name="t1" > and column_name= "a"; > -TABLE_CATALOG TABLE_SCHEMA TABLE_NAME COLUMN_NAME ORDINAL_POSITION COLUMN_DEFAULT IS_NULLABLE DATA_TYPE CHARACTER_MAXIMUM_LENGTH CHARACTER_OCTET_LENGTH NUMERIC_PRECISION NUMERIC_SCALE CHARACTER_SET_NAME COLLATION_NAME COLUMN_TYPE COLUMN_KEY EXTRA PRIVILEGES COLUMN_COMMENT > -NULL mysqltest t1 a 1 NULL YES int NULL NULL 10 0 NULL NULL int(11) select,insert,update,references > +TABLE_CATALOG TABLE_SCHEMA TABLE_NAME COLUMN_NAME ORDINAL_POSITION COLUMN_DEFAULT IS_NULLABLE DATA_TYPE CHARACTER_MAXIMUM_LENGTH CHARACTER_OCTET_LENGTH NUMERIC_PRECISION NUMERIC_SCALE DATETIME_PRECISION CHARACTER_SET_NAME COLLATION_NAME COLUMN_TYPE COLUMN_KEY EXTRA PRIVILEGES COLUMN_COMMENT > +NULL mysqltest t1 a 1 NULL YES int NULL NULL 10 0 NULL NULL NULL int(11) select,insert,update,references This is also an incompatible change that should be listed. > === modified file 'mysql-test/r/parser.result' > --- mysql-test/r/parser.result 2009-04-17 20:00:53 +0000 > +++ mysql-test/r/parser.result 2011-03-01 12:24:36 +0000 > @@ -556,9 +556,13 @@ DROP TABLE IF EXISTS t1; > SELECT STR_TO_DATE('10:00 PM', '%h:%i %p') + INTERVAL 10 MINUTE; > STR_TO_DATE('10:00 PM', '%h:%i %p') + INTERVAL 10 MINUTE > NULL > +Warnings: > +Error 1411 Incorrect datetime value: '10:00 PM' for function str_to_date Strange that 'str_to_date()' doesn't work with TIME arguments, but 'date_add()' does. > === modified file 'mysql-test/r/ps.result' > --- mysql-test/r/ps.result 2010-10-04 08:51:26 +0000 > +++ mysql-test/r/ps.result 2011-03-01 12:24:36 +0000 > @@ -3040,3 +3040,18 @@ id select_type table type possible_keys > DEALLOCATE PREPARE stmt; > DROP TABLE t1; > End of 5.1 tests. > +prepare stmt from "select date('2010-10-10') between '2010-09-09' and ?"; > +set @a='2010-11-11'; > +execute stmt using @a; > +date('2010-10-10') between '2010-09-09' and ? > +1 > +execute stmt using @a; > +date('2010-10-10') between '2010-09-09' and ? > +1 > +set @a='2010-08-08'; > +execute stmt using @a; > +date('2010-10-10') between '2010-09-09' and ? > +0 > +execute stmt using @a; > +date('2010-10-10') between '2010-09-09' and ? > +0 Please add a test that uses sub seconds (for completeness) set @a='2010-08-08 02:03:04.567890'; execute stmt using @a; > === modified file 'mysql-test/r/select.result' <cut> > -select str_to_date('2007-10-09','%Y-%m-%d') <= '2007/10/2000:00:00 GMT-6'; > -str_to_date('2007-10-09','%Y-%m-%d') <= '2007/10/2000:00:00 GMT-6' > +select str_to_date('2007-10-09','%Y-%m-%d') <= '2007/10/20 00:00:00 GMT-6'; > +str_to_date('2007-10-09','%Y-%m-%d') <= '2007/10/20 00:00:00 GMT-6' Please add a new test that shows that this fails: select str_to_date('2007-10-09','%Y-%m-%d') <= '2007/10/2000:00:00 GMT-6' As this is an incompatible change (but of course not critical) > Warnings: > -Warning 1292 Truncated incorrect date value: '2007/10/2000:00:00 GMT-6' > +Warning 1292 Truncated incorrect date value: '2007/10/20 00:00:00 GMT-6' > select str_to_date('2007-10-01','%Y-%m-%d') = '2007-10-1 00:00:00 GMT-6'; > str_to_date('2007-10-01','%Y-%m-%d') = '2007-10-1 00:00:00 GMT-6' > 1 > @@ -4179,22 +4179,17 @@ Warning 1292 Truncated incorrect datetim > select str_to_date('2007-10-00 12:34','%Y-%m-%d %H:%i') = '2007-10-01 12:34'; > str_to_date('2007-10-00 12:34','%Y-%m-%d %H:%i') = '2007-10-01 12:34' > 0 > -Warnings: > -Warning 1292 Truncated incorrect datetime value: '2007-10-00 12:34:00' Add a test here (which is run with SQL_MODE=traditional): select str_to_date('2007-10-00', "%Y-%m-%d %H:%i"); (Makes the test easier to understand) > @@ -4221,18 +4216,18 @@ str_to_date('1','%Y-%m-%d') = '1' > Warning 1292 Truncated incorrect date value: '1' > select str_to_date('','%Y-%m-%d') = ''; > str_to_date('','%Y-%m-%d') = '' This is an incompatible change (but for the better) In comparison string to date, we don't give errors for zero part dates independent of SQL_MODE. And as agreed on IRC, we can't have str_to_date('','%Y-%m-%d') return NULL as otherwise we couldn't use = to find rows with zero date in a table which would be worse. <cut> > Warnings: > Warning 1292 Truncated incorrect date value: '' > select str_to_date('1000-01-01','%Y-%m-%d') between '0000-00-00' and NULL; > str_to_date('1000-01-01','%Y-%m-%d') between '0000-00-00' and NULL > -0 > +NULL > select str_to_date('1000-01-01','%Y-%m-%d') between NULL and '2000-00-00'; > str_to_date('1000-01-01','%Y-%m-%d') between NULL and '2000-00-00' > -0 > +NULL > select str_to_date('1000-01-01','%Y-%m-%d') between NULL and NULL; > str_to_date('1000-01-01','%Y-%m-%d') between NULL and NULL > -0 > +NULL For completeness sake add: select str_to_date('2000-01-02','%Y-%m-%d') between NULL and '2000-01-01'; -> 0 select str_to_date('2000-01-02','%Y-%m-%d') between '2000-01-03' and NULL; -> 0 MariaDB 5.1 returns wrong result (NULL) for the above while 5.1-micro returns the correct answer. > === modified file 'mysql-test/r/subselect.result' <cut> > -delete from t1 where b = (select b from t1); > +delete from t1 where b in (select b from t1); Why the above change ? Please also add the original row, so that we can ensure it works the same way in the future. > === modified file 'mysql-test/r/type_datetime.result' > @@ -352,7 +352,7 @@ least(cast('01-01-01' as date), '01-01-0 > 2001-01-01 > select greatest(cast('01-01-01' as date), '01-01-02'); > greatest(cast('01-01-01' as date), '01-01-02') > -01-01-02 > +2001-01-02 Incompatible change (but ok) > === modified file 'mysql-test/r/type_time.result' > --- mysql-test/r/type_time.result 2010-05-31 09:25:11 +0000 > +++ mysql-test/r/type_time.result 2011-03-08 21:01:40 +0000 > @@ -1,6 +1,8 @@ > drop table if exists t1; > create table t1 (t time); > insert into t1 values("10:22:33"),("12:34:56.78"),(10),(1234),(123456.78),(1234559.99),("1"),("1:23"),("1:23:45"), ("10.22"), ("-10 1:22:33.45"),("20 10:22:33"),("1999-02-03 20:33:34"); > +Warnings: > +Note 1265 Data truncated for column 't' at row 13 Why the above note ? (I checked that the data is ok) Please document this in the compatible matrix. > === modified file 'mysql-test/suite/funcs_1/r/is_columns.result' > --- mysql-test/suite/funcs_1/r/is_columns.result 2008-11-13 09:50:20 +0000 > +++ mysql-test/suite/funcs_1/r/is_columns.result 2011-03-24 14:55:52 +0000 <cut> > +++ mysql-test/suite/funcs_1/r/is_columns_innodb.result 2011-03-24 14:55:52 +0000 > @@ -382,333 +382,333 @@ LOAD DATA INFILE '<MYSQLTEST_VARDIR>/std > SELECT * FROM information_schema.columns > WHERE table_schema LIKE 'test%' > ORDER BY table_schema, table_name, column_name; > -TABLE_CATALOG TABLE_SCHEMA TABLE_NAME COLUMN_NAME ORDINAL_POSITION COLUMN_DEFAULT IS_NULLABLE DATA_TYPE CHARACTER_MAXIMUM_LENGTH CHARACTER_OCTET_LENGTH NUMERIC_PRECISION NUMERIC_SCALE CHARACTER_SET_NAME COLLATION_NAME COLUMN_TYPE COLUMN_KEY EXTRA PRIVILEGES COLUMN_COMMENT > -NULL test t1 f1 1 NULL YES char 20 20 NULL NULL latin1 latin1_swedish_ci char(20) select,insert,update,references .... lots of changes How about changing the SELECT * FROM information_schema.columns to just list the old columns in suite/funcs_1/datadict/columns.inc This gives a smaller change list and will not cause a diff if we add another column... Especially this change caused really huge diff and will likely cause merge conflicts sooner or later... <cut> > === modified file 'mysys/my_getsystime.c' <cut> > +/* > + get time since epoc in 100 nanosec units > + > + NOTE: > + Thus to get the current time we should use the system function > + with the highest possible resolution > + > + The value is not subject to resetting or drifting by way of adjtime() or > + settimeofday(), and thus it is *NOT* appropriate for getting the current > + timestamp. It can be used for calculating time intervals, though. > + And it's good enough for UUID. > +*/ > + > ulonglong my_getsystime() > { > #ifdef HAVE_CLOCK_GETTIME > struct timespec tp; > clock_gettime(CLOCK_REALTIME, &tp); > return (ulonglong)tp.tv_sec*10000000+(ulonglong)tp.tv_nsec/100; > +#elif defined(HAVE_GETHRTIME) > + return gethrtime()/100-gethrtime_offset; > #elif defined(__WIN__) > LARGE_INTEGER t_cnt; > if (query_performance_frequency) > @@ -57,169 +73,70 @@ ulonglong my_getsystime() > #endif > } As discussed on IRC, I don't think it's make sense to try to have this aligned to 'epoch'. Better to just remove gethrtime_offset; Beeing 'free' from epoch will allow us to use other even faster clock's in the future like clock_gettime(CLOCK_MONOTONIC) > +my_hrtime_t my_hrtime() > { > + my_hrtime_t hrtime; > #if defined(__WIN__) > ulonglong newtime; > GetSystemTimeAsFileTime((FILETIME*)&newtime); > + hrtime.val= newtime/10; > #elif defined(HAVE_GETHRTIME) Shouldn't the above be 'HAVE_GETTIMEOFDAY' ? > struct timeval t; > /* > The following loop is here because gettimeofday may fail on some systems > */ > while (gettimeofday(&t, NULL) != 0) > {} > + hrtime.val= t.tv_sec*1000000 + t.tv_usec; > +#else > + hrtime.val= my_getsystime()/10; Don't think this option is correct as my_getsystime() is defined to be not reliable for timing. I would assume that all systems have either GetSystemTimeAsFileTime, gettimeofday or clock_gettime(). So I would instead add here: #ifdef HAVE_CLOCK_GETTIME struct timespec tp; clock_gettime(CLOCK_REALTIME, &tp); return (ulonglong)tp.tv_sec*10000000+(ulonglong)tp.tv_nsec/100; #else #error Need one of GetSystemTimeAsFileTime(), gettimeofday() or clock_gettime() #endif > + return hrtime; <cut> > +void my_time_init() > { <cut> > + gethrtime_offset= gethrtime(); I suggest we remove this > +#endif > } > === modified file 'sql-common/my_time.c' > --- sql-common/my_time.c 2010-07-09 12:00:17 +0000 > +++ sql-common/my_time.c 2011-03-26 10:59:34 +0000 > @@ -19,6 +19,9 @@ > /* Windows version of localtime_r() is declared in my_ptrhead.h */ > #include <my_pthread.h> > > +static enum enum_mysql_timestamp_type > +str_to_time(const char *str, uint length, MYSQL_TIME *l_time, int *warning) Make this again global; There are places where we can call this directly and there is no reason to do an extra jump to get here. <cut> > @@ -681,24 +692,31 @@ my_bool str_to_time(const char *str, uin > 1 time value is invalid > */ > > +int check_time_range(struct st_mysql_time *my_time, uint dec, int *warning) > { > longlong hour; > + static ulong max_sec_part[MAX_SEC_PART_DIGITS+1]= {000000, 900000, 990000, > + 999000, 999900, 999990, 999999}; > > if (my_time->minute >= 60 || my_time->second >= 60) > return 1; > > hour= my_time->hour + (24*my_time->day); > + > + if (dec == AUTO_SEC_PART_DIGITS) > + dec= MAX_SEC_PART_DIGITS; > + > if (hour <= TIME_MAX_HOUR && > (hour != TIME_MAX_HOUR || my_time->minute != TIME_MAX_MINUTE || > + my_time->second != TIME_MAX_SECOND || > + my_time->second_part <= max_sec_part[dec])) > return 0; > > my_time->day= 0; > my_time->hour= TIME_MAX_HOUR; > my_time->minute= TIME_MAX_MINUTE; > my_time->second= TIME_MAX_SECOND; > - my_time->second_part= 0; > + my_time->second_part= TIME_MAX_SECOND_PART; Shouldn't this be max_sec_part[dec] ? (Otherwise you may return a bigger value than what the function was called with) > *warning|= MYSQL_TIME_WARN_OUT_OF_RANGE; > return 0; > } <cut> > +int number_to_time(double nr, MYSQL_TIME *ltime, int *was_cut) > +{ > + ulong tmp; > + *was_cut= 0; > + ltime->year= ltime->month= ltime->day= 0; > + ltime->time_type= MYSQL_TIMESTAMP_TIME; > + > + if ((ltime->neg= nr < 0)) > + nr= -nr; > + > + if (nr > TIME_MAX_VALUE) > + { > + nr= TIME_MAX_VALUE; > + *was_cut= MYSQL_TIME_WARN_OUT_OF_RANGE; > + } > + tmp=(ulong)floor(nr); > + ltime->hour = tmp/100/100; > + ltime->minute= tmp/100%100; > + ltime->second= tmp%100; > + ltime->second_part= (ulong)((nr-tmp)*1e6); Change 1e6 to a define like 'TIME_SECOND_PART_TO_INT_MULTIPLIER' here and in all other places. > + if (ltime->minute < 60 && ltime->second < 60) > + return 0; > + > + *was_cut= MYSQL_TIME_WARN_TRUNCATED; > + return 1; > +} <cut> > === modified file 'sql/event_db_repository.cc' > --- sql/event_db_repository.cc 2010-01-22 09:38:21 +0000 > +++ sql/event_db_repository.cc 2011-03-01 12:24:36 +0000 > @@ -231,6 +231,9 @@ mysql_event_fill_row(THD *thd, > rs|= fields[ET_FIELD_STATUS]->store((longlong)et->status, TRUE); > rs|= fields[ET_FIELD_ORIGINATOR]->store((longlong)et->originator, TRUE); > > + if (!is_update) > + rs|= fields[ET_FIELD_CREATED]->set_time(); > + Don't understand why this was moved from Event_db_repository::create_event(). Probably safe but it would be nice to know the reason. (The bzr file comment didn't say anything about this) > === modified file 'sql/event_queue.cc' > --- sql/event_queue.cc 2008-05-09 07:43:02 +0000 > +++ sql/event_queue.cc 2011-03-01 12:24:36 +0000 > @@ -513,9 +513,10 @@ Event_queue::empty_queue() > */ > > void > +Event_queue::dbug_dump_queue(my_time_t when) > { > #ifndef DBUG_OFF > + my_time_t now= when; > Event_queue_element *et; > uint i; > DBUG_ENTER("Event_queue::dbug_dump_queue"); > @@ -592,14 +593,13 @@ Event_queue::get_top_for_execution_if_ti > thd->set_current_time(); /* Get current time */ > > next_activation_at= top->execute_at; > + if (next_activation_at >thd->query_start()) > { > /* > Not yet time for top event, wait on condition with > time or until signaled. Release LOCK_queue while waiting. > */ > - struct timespec top_time; > - set_timespec(top_time, next_activation_at - thd->query_start()); > + struct timespec top_time= { next_activation_at, 0 }; Not sure if the above change is portable. That is why we have the set_timespec() defines. Any particular reason why you did the change ? I would prefer to have the original code as we know it's works and compiles on a lot of platforms. However, after your definition change of my_getsystime() you have to redefine all the macros on my_pthread.h that calls my_getsystime() to call my_hrtime() instead as my_getsystime() can't anymore be used as a start point for timespec. If you want to keep the above change, you should also remove all the set_timespec() defines from my_pthread.h and fix all other code that uses this! > cond_wait(thd, &top_time, queue_wait_msg, SCHED_FUNC, __LINE__); > > continue; > === modified file 'sql/field.cc' > @@ -3151,13 +3141,9 @@ int Field_short::store(const char *from, > > error= get_int(cs, from, len, &rnd, UINT_MAX16, INT_MIN16, INT_MAX16); > store_tmp= unsigned_flag ? (int) (ulonglong) rnd : (int) rnd; > -#ifdef WORDS_BIGENDIAN > - if (table->s->db_low_byte_first) > - { > + if (ARCH_BIGENDIAN && table->s->db_low_byte_first) > int2store(ptr, store_tmp); > - } The above code can produce warnings on some compilers with the error 'if is always true'. I also find it much harder to read and understand! Please add back the original code and don't try to play smart games with the compiler! This is not part of microsecond patch and should not be done here. Same goes for all other changes of WORDS_BIGENDIAN. That said, we can probably remove the db_low_byte_first code in the future as no engine uses this anymore. This should however be a different patch! > @@ -4822,136 +4682,136 @@ timestamp_auto_set_type Field_timestamp: > } > } > > +long Field_timestamp::get_timestamp(ulong *sec_part) const > +{ > + ASSERT_COLUMN_MARKED_FOR_READ; > + *sec_part= 0; > + if (ARCH_BIGENDIAN && table && table->s->db_low_byte_first) > + return sint4korr(ptr); > + long tmp; > + longget(tmp,ptr); > + return tmp; > +} > Shouldn't this be of type 'my_time_t' for completeness? > -int Field_timestamp::store(const char *from,uint len,CHARSET_INFO *cs) > +int Field_timestamp::store_TIME_with_warning(THD *thd, MYSQL_TIME *l_time, > + const Lazy_string *str, > + bool was_cut, bool have_smth_to_conv) > { > ASSERT_COLUMN_MARKED_FOR_WRITE; > + uint error = 0; > + my_time_t timestamp; > my_bool in_dst_time_gap; > + if (was_cut || !have_smth_to_conv) > { > error= 1; > set_datetime_warning(MYSQL_ERROR::WARN_LEVEL_WARN, WARN_DATA_TRUNCATED, > + str, MYSQL_TIMESTAMP_DATETIME, 1); > } Add a comment here. The following test is not clear > + if (have_smth_to_conv && l_time->month) > { > + if (!(timestamp= TIME_to_timestamp(thd, l_time, &in_dst_time_gap))) > { > set_datetime_warning(MYSQL_ERROR::WARN_LEVEL_WARN, > ER_WARN_DATA_OUT_OF_RANGE, > + str, MYSQL_TIMESTAMP_DATETIME, !error); > error= 1; > } > else if (in_dst_time_gap) > { > set_datetime_warning(MYSQL_ERROR::WARN_LEVEL_WARN, > ER_WARN_INVALID_TIMESTAMP, > + str, MYSQL_TIMESTAMP_DATETIME, !error); > error= 1; > } > } As we are fixing things, it would be nice to change the two above 'if' in the common case to one if. We could do it by changing the last parameter in TIME_to_timestamp() to an enum and do: timestamp= TIME_to_timestamp(thd, l_time, &timstamp_error) if (timestamp_error) { set_datetime_warning(MYSQL_ERROR::WARN_LEVEL_WARN, (timestamp_error == MYSQL_TIMESTAMP_IN_DST_TIME_GAP ? ER_WARN_INVALID_TIMESTAMP : ER_WARN_DATA_OUT_OF_RANGE), str, MYSQL_TIMESTAMP_DATETIME, !error); error= 1; } Less code and only one if... > + else > + { > + timestamp= 0; > + l_time->second_part= 0; > + } > + store_TIME(timestamp, l_time->second_part); > return error; > } > +int Field_timestamp::store_time(MYSQL_TIME *ltime,timestamp_type time_type) > +{ > + THD *thd= table ? table->in_use : current_thd; You can assume that table->in_use is always set. (Other place in the code already assumes that). Feel free to fix this everywhere in field.cc > + int unused; > + MYSQL_TIME l_time= *ltime; > + Lazy_string_time str(ltime); > + bool valid= !check_date(&l_time, pack_time(&l_time) != 0, > + (thd->variables.sql_mode & MODE_NO_ZERO_DATE) | > + MODE_NO_ZERO_IN_DATE, &unused); > + > + return store_TIME_with_warning(thd, &l_time, &str, false, valid); > +} > + Add an empty line here > +int Field_timestamp::store(const char *from,uint len,CHARSET_INFO *cs) > +{ <cut> > int Field_timestamp::store(double nr) > + MYSQL_TIME l_time; > + int error; > + Lazy_string_dbl str(nr); > + THD *thd= table ? table->in_use : current_thd; > + > + /* We don't want to store invalid or fuzzy datetime values in TIMESTAMP */ > + longlong tmp= number_to_datetime((longlong) floor(nr), > + &l_time, (thd->variables.sql_mode & > + MODE_NO_ZERO_DATE) | > + MODE_NO_ZERO_IN_DATE, &error); > + l_time.second_part= (ulong)((nr-floor(nr))*1e6); > + return store_TIME_with_warning(thd, &l_time, &str, error, tmp != LL(-1)); > } You should add back the test: if (nr < 0 || nr > 99991231235959.9999999) the reason is that on some machines you get a trap or wrong result if you try to convert a too big double to a longlong! > longlong Field_timestamp::val_int(void) > { > MYSQL_TIME time_tmp; > THD *thd= table ? table->in_use : current_thd; > > thd->time_zone_used= 1; > ulong sec_part; > uint32 temp= get_timestamp(&sec_part); Add a comment here: /* Field_timestamp() and Field_timestamp_hres() shares this code. This is why are also testing sec_part below. */ > if (temp == 0 && sec_part == 0) > return(0); <cut> > longlong Field_timestamp::val_int(void) > { > MYSQL_TIME time_tmp; > THD *thd= table ? table->in_use : current_thd; Please add: ASSERT_COLUMN_MARKED_FOR_READ; > +static const char *zero_timestamp="0000-00-00 00:00:00.000000"; Move to start of file. > String *Field_timestamp::val_str(String *val_buffer, String *val_ptr) > { > - ASSERT_COLUMN_MARKED_FOR_READ; Add back ASSERT_COLUMN_MARKED_FOR_READ; <cut> > -void Field_timestamp::set_time() > +int Field_timestamp::set_time() > { > THD *thd= table ? table->in_use : current_thd; > - long tmp= (long) thd->query_start(); > set_notnull(); > - store_timestamp(tmp); > + store_TIME(thd->query_start(), 0); > + return 0; > } The set_time() function could be a void as it can never fail. (Yes, I agree that it means that we have to test type of field in event code, but that something we should do at event startup anyway...) <cut> > +static void store_native(ulonglong num, uchar *to, uint bytes) > { > + switch(bytes) { > + case 1: *to= (uchar)num; break; > + case 2: shortstore(to, (ushort)num); break; > + case 3: int3store(to, num); /* Sic!*/ break; > + case 4: longstore(to, (ulong)num); break; > + case 8: longlongstore(to, num); break; > + default: DBUG_ASSERT(0); > + } > +} What would be even better if each field would have a pointers to c functions with the right read and write functions; Would avoid a switch for every call... <cut> > +static uint sec_part_bytes[MAX_DATETIME_PRECISION+1]= { 0, 1, 1, 2, 2, 3, 3 }; Move to start of file and document this. <cut> > + if (!tmp) > + return fuzzydate & TIME_NO_ZERO_DATE; should be: > + return (fuzzydate & TIME_NO_ZERO_DATE) != 0; as the function is a bool function > +bool Field_datetime_hires::get_date(MYSQL_TIME *ltime, uint fuzzydate) > +{ > + ulonglong packed= read_bigendian(ptr, Field_datetime_hires::pack_length()); > + unpack_time(sec_part_unshift(packed, dec), ltime); > + if (!packed) > + return fuzzydate & TIME_NO_ZERO_DATE; Should be: return (fuzzydate & TIME_NO_ZERO_DATE) != 0; > + if (!ltime->month || !ltime->day) > + return !(fuzzydate & TIME_FUZZY_DATE); > + return 0; > +} > + > +uint32 Field_datetime_hires::pack_length() const > +{ > + return 5 + sec_part_bytes[dec]; > +} move to include file. <cut> > @@ -8217,7 +8020,7 @@ Field_blob::unpack_key(uchar *to, const > length+= *from++ << 8; > > /* put the length into the record buffer */ > - put_length(to, length); > + store_length(to, packlength, length, table->s->db_low_byte_first); Note that the old format was not depending on table->s->db_low_byte_first ! it should alway stored in low byte first order! Change above to store_lowendian() > === modified file 'sql/field.h' > --- sql/field.h 2010-03-17 18:15:41 +0000 > +++ sql/field.h 2011-03-24 11:30:03 +0000 > @@ -22,8 +22,13 @@ > #pragma interface /* gcc class implementation */ > #endif > > +#ifdef WORDS_ARCH_BIGENDIAN > +#define ARCH_BIGENDIAN 1 > +#else > +#define ARCH_BIGENDIAN 0 > +#endif > + <cut> > class Field_datetime :public Field_temporal { ... > enum Item_result cmp_type () const { return TIME_RESULT; } remove cmp_type() as it's already defined identically by Field_temporal > === modified file 'sql/item.cc' > --- sql/item.cc 2010-11-18 13:11:18 +0000 > +++ sql/item.cc 2011-03-24 14:55:52 +0000 > @@ -461,6 +461,34 @@ void Item::print_item_w_name(String *str > } > > > +void Item::print_value(String *str) > +{ > + char buff[MAX_FIELD_WIDTH]; > + String *ptr, tmp(buff,sizeof(buff),str->charset()); > + ptr= val_str(&tmp); > + if (!ptr) > + str->append("NULL"); > + else > + { > + switch (result_type()) > + { > + default: > + DBUG_ASSERT(0); > + case STRING_RESULT: > + str->append('\''); > + str->append(*ptr); > + str->append('\''); Shouldn't we also escape \' ? (As we never know what the print is used for) Doing 'append_unescaped(str, ptr->ptr(), ptr->length())' instead of above would fix that. (Yes, I know that the original code didn't do that, but that was also a bug). > + break; > + case DECIMAL_RESULT: > + case REAL_RESULT: > + case INT_RESULT: > + str->append(*ptr); > + break; > + } Shouldn't you have TIME_RESULT: here too ? If this never happens, you should at least have this as part of default to make it clear it wasn't forgotten. (Actually removing the default would be a good idea as then we will get a compiler warning if someone adds a new result type but don't take care if it here). > + } > +} > + > + > void Item::cleanup() > { > DBUG_ENTER("Item::cleanup"); > @@ -503,6 +531,45 @@ void Item::rename(char *new_name) > name= new_name; > } > > +Item_result Item::cmp_type() const > +{ > + switch(field_type()) { Add space after switch > bool Item::get_time(MYSQL_TIME *ltime) > { > return get_date(ltime, TIME_TIME_ONLY); > } You should probably add TIME_FUZZY_DATE to the above. This should fix the error that I have described at the end of the review. <cut> > /** > Traverse item tree possibly transforming it (replacing items). > @@ -915,13 +982,15 @@ bool Item_string::eq(const Item *item, b > > bool Item::get_date(MYSQL_TIME *ltime,uint fuzzydate) > { > - if (result_type() == STRING_RESULT) > + if (field_type() == MYSQL_TYPE_TIME) > + fuzzydate|= TIME_TIME_ONLY; > + if (result_type() == STRING_RESULT || fuzzydate & TIME_TIME_ONLY) > { Please add a comment under what conditions or why field_type() can be MYSQL_TYPE_TIME and result_type() is STRING_RESULT. (Looks like this only happens for Item_field(Field_time)) I am actually a bit confused by the fact that only Item_field can have TIME_RESULT. Why doesn't all time functions say they have TIME_RESULT? > @@ -2698,22 +2750,23 @@ void Item_param::set_time(MYSQL_TIME *tm > value.time= *tm; > value.time.time_type= time_type; > > + decimals= value.time.second_part > 0 ? MAX_SEC_PART_DIGITS : 0; > + > if (value.time.year > 9999 || value.time.month > 12 || > value.time.day > 31 || > (time_type != MYSQL_TIMESTAMP_TIME && value.time.hour > 23) || > - value.time.minute > 59 || value.time.second > 59) > + value.time.minute > 59 || value.time.second > 59 || > + value.time.second_part > MAX_SEC_PART_VALUE) > { > + Lazy_string_time str(&value.time); > make_truncated_value_warning(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN, > + &str, time_type, 0); > set_zero_time(&value.time, MYSQL_TIMESTAMP_ERROR); > } > > state= TIME_VALUE; > maybe_null= 0; > max_length= max_length_arg; > - decimals= 0; > DBUG_VOID_RETURN; Please move setting of decimal to end of function (where the old setting was). all Item_param::set... functions first checks the arguments and then sets all values at the end (in the same place). (Just makes the function easier to read) > } > > @@ -2790,10 +2843,12 @@ bool Item_param::set_from_user_var(THD * > case REAL_RESULT: > set_double(*(double*)entry->value); > item_type= Item::REAL_ITEM; > + param_type= MYSQL_TYPE_DOUBLE; > break; Here you set the param_type, but I wonder if there could be a problem that the param_type is different over two execution of a prepared statement. I tried some things to see if there is a problem, but couldn't see any behavior changes because of this, so this change may be safe. What did this change fix? (At at least a code comment why param_type is reset here) > @@ -4651,12 +4693,7 @@ Item *Item_field::equal_fields_propagato > item= this; > else if (field && (field->flags & ZEROFILL_FLAG) && IS_NUM(field->type())) > { > - if (item && field->type() != FIELD_TYPE_TIMESTAMP && cmp_context != INT_RESULT) > + if (item && (cmp_context == STRING_RESULT || cmp_context == (Item_result)-1)) When adding to 5.3, don't forget to change to use IMPOSSIBLE_RESULT > @@ -6852,11 +6879,19 @@ void resolve_const_item(THD *thd, Item * <cut> > + case TIME_RESULT: > + { > + bool is_null; > + Item **ref_copy= ref; Please add a short comment why we don't create an Item_datetime value here (as this is what we do in all the other cases) > + get_datetime_value(thd, &ref_copy, &new_item, comp_item, &is_null); > + if (is_null) > + new_item= new Item_null(name); > + break; > + } > @@ -7009,6 +7047,21 @@ int stored_field_cmp_to_item(THD *thd, F > field_val= field->val_decimal(&field_buf); > return my_decimal_cmp(item_val, field_val); > } Add comment: /* We have to check field->cmp_type() here as res_type is TIME_RESULT if either field or item is of type TIME_RESULT */ > + if (field->cmp_type() == TIME_RESULT) > + { > + MYSQL_TIME field_time, item_time; Fix indentation (should be 2 space, not 4) > + if (field->type() == MYSQL_TYPE_TIME) > + { > + field->get_time(&field_time); > + item->get_time(&item_time); > + } > + else > + { > + field->get_date(&field_time, TIME_FUZZY_DATE | TIME_INVALID_DATES); > + item->get_date(&item_time, TIME_FUZZY_DATE | TIME_INVALID_DATES); > + } > + return my_time_compare(&field_time, &item_time); > + } Looking at the code, it would probably be simpler if we replaced all the 'if (res_type)' with a swtich. > @@ -7048,6 +7101,8 @@ Item_cache* Item_cache::get_cache(const > return new Item_cache_str(item); > case ROW_RESULT: > return new Item_cache_row(); > + case TIME_RESULT: > + return new Item_cache_int(MYSQL_TYPE_DATETIME); Shouldn't this be a double or decimal as the time can contain microseconds? Looks like a potential bug! > === modified file 'sql/item.h' > --- sql/item.h 2010-07-30 13:35:06 +0000 > +++ sql/item.h 2011-03-24 11:30:03 +0000 > @@ -569,7 +569,8 @@ class Item { > virtual bool send(Protocol *protocol, String *str); > virtual bool eq(const Item *, bool binary_cmp) const; > virtual Item_result result_type() const { return REAL_RESULT; } > - virtual Item_result cast_to_int_type() const { return result_type(); } > + virtual Item_result cmp_type() const; Please add documentation what's the difference between cmp_type() and result_type() and when one should choose which. By the way, you can remove Field->cast_to_int_type() as this is always same as field->result_type(). I also noticed that you have a Field->cmp_type() but there is no Item_field->cmp_type() mapped to Field->cmp_type(). I couldn't As I assume that for all fields the following should hold: Item_field->cmp_type() == Item_field->field->cmp_type() If not that needs to be changed or properly documented as otherwise things are very confusing! > @@ -3031,7 +3022,7 @@ class Item_cache_int: public Item_cache > protected: > longlong value; > public: > - Item_cache_int(): Item_cache(), > + Item_cache_int(): Item_cache(MYSQL_TYPE_LONGLONG), > value(0) {} > Item_cache_int(enum_field_types field_type_arg): > Item_cache(field_type_arg), value(0) {} > @@ -3043,8 +3034,13 @@ class Item_cache_int: public Item_cache > String* val_str(String *str); > my_decimal *val_decimal(my_decimal *); > enum Item_result result_type() const { return INT_RESULT; } > - bool result_as_longlong() { return TRUE; } > bool cache_value(); > + Item *clone_item() > + { > + Item_cache_int *item= new Item_cache_int(cached_field_type); > + item->store(this, value); > + return item; > + } > }; Don't understand why Item_cache_int() needs a clone_item when Item_cache_real() or Item_cache_decimal() dosn't have one. If this was needed, please check if the other Item_cache_xxx() functions needs it. > === modified file 'sql/item_cmpfunc.cc' <cut> Please add documentation for this function: > @@ -891,65 +713,15 @@ int Arg_comparator::set_cmp_func(Item_re > Item **a1, Item **a2, > Item_result type) <cut> > @@ -1026,9 +761,14 @@ Item** Arg_comparator::cache_converted_c > Item **cache_item, > Item_result type) > { > - /* Don't need cache if doing context analysis only. */ > + /* > + Don't need cache if doing context analysis only. > + Also, get_datetime_value creates Item_cache internally. > + Unless fixed, we should not do it here. > + */ > if (!thd_arg->is_context_analysis_only() && > - (*value)->const_item() && type != (*value)->result_type()) > + (*value)->const_item() && type != (*value)->result_type() && > + type != TIME_RESULT) > { > Item_cache *cache= Item_cache::get_cache(*value, type); > cache->setup(*value); Why don't we cache it there instead of doing it in get_datetime_value()? This would save us one if and 3 test for every call to get_datetime_value(). You could here call get_datetime_value() and use this value in the cache. If this doesn't easily work (as indicated on IRC), at least expand the comment to explain the issue so that we can later try to fix it in a better way. > @@ -1071,81 +806,120 @@ void Arg_comparator::set_datetime_cmp_fu > DESCRIPTION > Retrieves the correct DATETIME value from given item for comparison by the > compare_datetime() function. > + > + If the value should be compared as time (TIME_RESULT), it's retrieved as > + MYSQL_TIME. Otherwise it's read as a number/string and converted to time. > + Constant items are cached, so the convertion is only done once for them. > + > + Note the f_type behavior: if the item can be compared as time, then > + f_type is this item's field_type(). Otherwise it's field_type() of > + warn_item (which is the other operand of the comparison operator). > + This logic provides correct string/number to date/time conversion > + depending on the other operand (when comparing a string with a date, it's > + parsed as a date, when comparing a string with a time it's parsed as a time) > > RETURN > + MYSQL_TIME value, packed in a longlong, suitable for comparison. > */ > Please add the following to the comment: For example, if we are executing WHERE datetime_6_column = 20010101000001.000020 for the first call the 'number-item' will have item->cmp_type == REAL_RESULT. It will create an item_cache with cmp_type() == TIME_RESULT and item->result_type() == INT_RESULT which will replace the item. No other item than an item_cache item will have cmp_type() == TIME_RESULT and result_type() == INT_RESULT, which makes the TIME_RESULT case safe. > longlong > get_datetime_value(THD *thd, Item ***item_arg, Item **cache_arg, > Item *warn_item, bool *is_null) > { > + longlong UNINIT_VAR(value); > Item *item= **item_arg; > + enum_field_types f_type= warn_item->field_type(); > + timestamp_type t_type= > + f_type == MYSQL_TYPE_DATE ? MYSQL_TIMESTAMP_DATE : > + f_type == MYSQL_TYPE_TIME ? MYSQL_TIMESTAMP_TIME : > + MYSQL_TIMESTAMP_DATETIME; > + > + switch (item->cmp_type()) { > + case TIME_RESULT: > + /* if it's our Item_cache_int, as created below, we simply use the value */ > + if (item->result_type() == INT_RESULT) > + value= item->val_int(); Please also set cache_item= 0 to make the end function test simpler. <cut> > + default: DBUG_ASSERT(0); > } > - if (*is_null) > + if ((*is_null= item->null_value)) > return ~(ulonglong) 0; > + if (cache_arg && item->const_item() && item->type() != Item::CACHE_ITEM) > { With my suggested change above, you can remove test of CACHE_ITEM. Another option would be to add to function start: if (item->type() == Item::CACHE_ITEM) { value= item->val_int(); *is_null= item->null_value; return; } Which would save us the calcuation of f_type and t_type for all cached objects. I would also suggest delaying calcuation of t_type until it's needed as in most cases we will not need it (constant items or fields) <cut> > +int Arg_comparator::compare_e_datetime() > +{ > + bool a_is_null, b_is_null; > + longlong a_value, b_value; > + > + if (set_null) > + owner->null_value= 0; Remove the above set; No other compare_e... function sets null_value > int Arg_comparator::compare_string() > { > @@ -2247,9 +1963,7 @@ void Item_func_between::fix_length_and_d > { > max_length= 1; > int i; > - bool datetime_found= FALSE; > - int time_items_found= 0; > - compare_as_dates= TRUE; > + compare_as_dates= 0; > THD *thd= current_thd; Please move declarations first and assignment of variables later. (Even better, move 'int i' to the block where it's used) > /* > @@ -2265,43 +1979,33 @@ void Item_func_between::fix_length_and_d > return; > > /* > + When comparing as date/time, we need to convert non-temporal values > + (e.g. strings) to MYSQL_TIME. get_datetime_value() doees it doees -> does > + automatically when one of the operands is a date/time. But here we > + may need to compare two strings as dates (str1 BETWEEN str2 AND date). > + For this to work, we need to know what date/time type we compare > + strings as. > */ > + if (cmp_type == TIME_RESULT) > { > for (i= 0; i < 3; i++) > { > + if (args[i]->cmp_type() == TIME_RESULT) > { > - datetime_found= TRUE; > + if (args[i]->field_type() != MYSQL_TYPE_TIME || > + (args[i]->field_type() == MYSQL_TYPE_TIME && compare_as_dates==0)) > + compare_as_dates= args[i]; > continue; > } > } > } I find it strange that you set compare_as_dates to the last not MYSQL_TYPE_TIME column as this is used to give warnings as the warning may in theory come from some other field. I tried to make a test case to show the problem but didn't succeed :( > @@ -2319,27 +2023,46 @@ void Item_func_between::fix_length_and_d > longlong Item_func_between::val_int() > { // ANSI BETWEEN > DBUG_ASSERT(fixed == 1); > - if (compare_as_dates) > + > + switch (cmp_type) { > + case TIME_RESULT: > { > - int ge_res, le_res; > + THD *thd= current_thd; > + longlong value, a, b; > + Item *cache, **ptr; > + bool value_is_null, a_is_null, b_is_null; > + Add a comment here that it's ok to have cache and ptr on stack as if we created a newcache item, it will be taken care of by the call to change_item_tree(). <cut> > + if (!a_is_null && !b_is_null) > + return (longlong) ((value >= a && value <= b) != negated); > + if (a_is_null && b_is_null) > + null_value=1; > + else if (a_is_null) > + null_value= value <= b; // not null if false range. > else > + null_value= value >= a; > + break; A slightly simpler if: if (a_is_null) null_value= b_is_null || value <= b; // not null if false range. else null_value= value >= a; <cut> > @@ -3193,6 +2940,13 @@ void Item_func_coalesce::fix_length_and_ > { > cached_field_type= agg_field_type(args, arg_count); > agg_result_type(&hybrid_type, args, arg_count); > + Item_result cmp_type; > + agg_cmp_type(&cmp_type, args, arg_count); > + if (cmp_type == TIME_RESULT) > + { > + count_real_length(); > + return; > + } Add a comment that this code is not needed when we have TIME_RESULT also as a result type. > === modified file 'sql/item_create.cc' > --- sql/item_create.cc 2010-07-02 18:30:47 +0000 > +++ sql/item_create.cc 2011-03-08 18:41:58 +0000 > @@ -5044,6 +5044,14 @@ find_qualified_function_builder(THD *thd > return & Create_sp_func::s_singleton; > } > > +static inline const char* item_name(Item *a, String *str) > +{ > + if (a->name) > + return a->name; > + str->length(0); > + a->print(str, QT_ORDINARY); > + return str->c_ptr_safe(); > +} Add a comment that 'item_name()' always returns a \0 ended string. > @@ -5051,6 +5059,8 @@ create_func_cast(THD *thd, Item *a, Cast > CHARSET_INFO *cs) <cut> > case ITEM_CAST_DATETIME: > - res= new (thd->mem_root) Item_datetime_typecast(a); > + { > + uint len; > + if (c_len) > + { > + errno= 0; > + len= strtoul(c_len, NULL, 10); Note that strtoul() on error doesn't set errno but my_errno (if we are using using our version of strtoul()). Anyway you don't have to check errno as it will return UTYPE_MAX on error, which is bigger than MAX_DATETIME_PRECISION. In general, it's better to use my_strtoll10() than strtoul() because this is faster and it returns the error in a variable that is easier to check than errno. Please also fix all other calls to strtoul() in this function > + if (errno != 0 || len > MAX_DATETIME_PRECISION) > + { > + my_error(ER_TOO_BIG_PRECISION, MYF(0), len, > + item_name(a, &buf), MAX_DATETIME_PRECISION); > + return NULL; > + } > + } > + else > + len= 0; Set len= 0 before the 'if'. This is faster and less code lines. > case ITEM_CAST_DECIMAL: > { > ulong len= 0; > @@ -5083,8 +5111,8 @@ create_func_cast(THD *thd, Item *a, Cast > decoded_size= strtoul(c_len, NULL, 10); > if (errno != 0) > { > - my_error(ER_TOO_BIG_PRECISION, MYF(0), c_len, a->name, > - DECIMAL_MAX_PRECISION); > + my_error(ER_TOO_BIG_PRECISION, MYF(0), decoded_size, > + item_name(a, &buf), DECIMAL_MAX_PRECISION); Good bug fix, but still not perfect as the value could be a longlong and then you get a strange number in the result. Best would be to print the result as ulonglong, but that would require us to add another error message which is not optimal either. Example: select cast('2001-01-01' as decimal(99999999999999)); -> ERROR 1426 (42000): Too big precision 276447231 specified for column '2001-01-01'. Maximum is 65 <cut> > @@ -5135,7 +5163,7 @@ create_func_cast(THD *thd, Item *a, Cast > decoded_size= strtoul(c_len, NULL, 10); > if ((errno != 0) || (decoded_size > MAX_FIELD_BLOBLENGTH)) > { > - my_error(ER_TOO_BIG_DISPLAYWIDTH, MYF(0), "cast as char", MAX_FIELD_BLOBLENGTH); > + my_error(ER_TOO_BIG_DISPLAYWIDTH, MYF(0), "cast as char", MAX_FIELD_BLOBLENGTH); > return NULL; > } > len= (int) decoded_size; > > === modified file 'sql/item_func.cc' > --- sql/item_func.cc 2010-11-23 13:08:10 +0000 > +++ sql/item_func.cc 2011-03-19 13:54:46 +0000 > @@ -940,8 +940,7 @@ longlong Item_func_signed::val_int() > longlong value; > int error; > > - if (args[0]->cast_to_int_type() != STRING_RESULT || > - args[0]->result_as_longlong()) > + if (args[0]->cast_to_int_type() != STRING_RESULT) > { > value= args[0]->val_int(); > null_value= args[0]->null_value; > @@ -982,8 +981,7 @@ longlong Item_func_unsigned::val_int() > value= 0; > return value; > } > - else if (args[0]->cast_to_int_type() != STRING_RESULT || > - args[0]->result_as_longlong()) > + else if (args[0]->cast_to_int_type() != STRING_RESULT) > { > value= args[0]->val_int(); > null_value= args[0]->null_value; > @@ -2220,10 +2218,10 @@ double Item_func_units::val_real() > void Item_func_min_max::fix_length_and_dec() > { > int max_int_part=0; > decimals=0; > max_length=0; > maybe_null=0; > + thd= current_thd; We should consider in the future adding thd as an argument to fix_length_and_dec(). The alternative is that in the functions one wants to access thd, one implements fix_fields() that just sets thd and call parent fix_fields(). As almost all Item_date_func's uses thd, it may be a good idea to add thd to this item and set it in fix_fields(). This would remove a LOT of calls to current_thd. <cut> > +bool Item_func_min_max::get_date(MYSQL_TIME *ltime, uint fuzzy_date) > { > longlong UNINIT_VAR(min_max); > + DBUG_ASSERT(fixed == 1); > + if (!compare_as_dates) > + return Item_func::get_date(ltime, fuzzy_date); Add a comment in which case the above may not be true. > > for (uint i=0; i < arg_count ; i++) > { > Item **arg= args + i; > bool is_null; > + longlong res= get_datetime_value(thd, &arg, 0, compare_as_dates, &is_null); > > /* Check if we need to stop (because of error or KILL) and stop the loop */ Using 'compare_as_dates' can give a strange warning: select greatest(cast("2001-01-5" as date), "2002-02-7aaa"); -> Incorrect date value: '2002-02-7aaa' for column 'cast("2001-01-5" as date)' at row 1 In cases like this when we don't store the value in a field, it would may be better to just give an error: Incorrect date value: '2002-02-7aaa' for function 'greatest' at row 1 We could get the name by just sending 'this' to the get_datetime_value instead of a found argument and we could choose error message based if the 'warn_item->real_item()->type()' is a FIELD_ITEM or not. > if (thd->is_error()) > { > null_value= 1; > + return 1; > } > > if ((null_value= args[i]->null_value)) > + return 1; You could combine the above two tests to: if (thd->is_error() || args[i]->null_value) { null_value= 1; return 1; } > if (i == 0 || (res < min_max ? cmp_sign : -cmp_sign) > 0) > min_max= res; > } > + unpack_time(min_max, ltime); > + if (compare_as_dates->field_type() == MYSQL_TYPE_DATE) > + ltime->time_type= MYSQL_TIMESTAMP_DATE; I would rather cache the correct value on upper level and store it in ltime->time_type directly. The above test also looks a bit suspicion if you would compare date with time. greatest(cast("2001-01-5" as date), cast("10:20:05" as time)) -> 2001-01-05 another strange effect is: greatest(cast("0-0-0" as date), cast("10:20:05" as time)) -> 0000-00-00 but: greatest(cast("0-0-0" as date), cast("10:20:05" as time)) = "0000-00-00" -> 0 (Which is wrong) compare to: concat(greatest(cast("0-0-0" as date), cast("10:20:05" as time))) = "0000-00-00" -> true This shows what is really happening: cast(greatest(cast("0-0-0" as date), cast("10:20:05" as time)) as datetime(6)) -> 0000-00-00 10:20:05.000000 Suggested fix: a) Do as before, when comparing datetime or date with time, convert to time, not datetime. b) Give an error when comparing date or datetime with time. c) When doing compare of date and time, upgrade type to compare to DATETIME. > @@ -2319,19 +2308,14 @@ String *Item_func_min_max::val_str(Strin > DBUG_ASSERT(fixed == 1); > if (compare_as_dates) > { > + MYSQL_TIME ltime; > + if (get_date(&ltime, TIME_FUZZY_DATE)) > return 0; > + char buf[MAX_DATE_STRING_REP_LENGTH]; > + int len= my_TIME_to_str(&ltime, buf, decimals); > + str->copy(buf, len, collation.collation); > + return str; A better way would be to to do: str->alloc(MAX_DATE_STRING_REP_LENGTH); str->set_charset(collation.collation). str->length(my_TIME_to_str(&ltime, str->ptr(), decimals)); This avoids the extra strcpy() (One should also check the return value for alloc() and copy() here). You could also make item_timefunc.cc:make_datetime() public and just call this. <cut> > @@ -3979,7 +3968,7 @@ double user_var_entry::val_real(my_bool > } > case STRING_RESULT: > return my_atof(value); // This is null terminated > - case ROW_RESULT: > + default: Add instead ROW_RESULT and TIME_RESULT here. This is safer as it's less chance we miss something when we add more result types later. > DBUG_ASSERT(1); // Impossible > break; > } > @@ -4010,7 +3999,7 @@ longlong user_var_entry::val_int(my_bool > int error; > return my_strtoll10(value, (char**) 0, &error);// String is null terminated > } > - case ROW_RESULT: > + default: Same as above. > DBUG_ASSERT(1); // Impossible > break; > } > @@ -4042,7 +4031,7 @@ String *user_var_entry::val_str(my_bool > case STRING_RESULT: > if (str->copy(value, length, collation.collation)) > str= 0; // EOM error > - case ROW_RESULT: > + default: > DBUG_ASSERT(1); // Impossible > break; Same as above. > } > @@ -4069,7 +4058,7 @@ my_decimal *user_var_entry::val_decimal( > case STRING_RESULT: > str2my_decimal(E_DEC_FATAL_ERROR, value, length, collation.collation, val); > break; > - case ROW_RESULT: > + default: Same as above. > DBUG_ASSERT(1); // Impossible > break; > } > > === modified file 'sql/item_timefunc.cc' <cut> > -static bool sec_to_time(longlong seconds, bool unsigned_flag, MYSQL_TIME *ltime) > +static bool sec_to_time(double seconds, MYSQL_TIME *ltime) > { > uint sec; > > bzero((char *)ltime, sizeof(*ltime)); > + > + ltime->time_type= MYSQL_TIMESTAMP_TIME; > > if (seconds < 0) > { > - if (unsigned_flag) > - goto overflow; > ltime->neg= 1; > if (seconds < -3020399) > goto overflow; Shouldn't this be: if (seconds < -3020399.999999) Same for the other test. (Instead of a constant number this should of course be TIME_MAX_VALUE_SECONDS + TIME_MAX_SECOND_PART/1e6) Add also tests for this in the test suite! > @@ -211,9 +95,8 @@ static bool sec_to_time(longlong seconds > ltime->minute= TIME_MAX_MINUTE; > ltime->second= TIME_MAX_SECOND; > > - char buf[22]; > - int len= (int)(longlong10_to_str(seconds, buf, unsigned_flag ? 10 : -10) > - - buf); > + char buf[100]; > + uint len= snprintf(buf, sizeof(buf), "%.80g", ltime->neg ? -seconds: seconds); I don't think we want to have 80 digit precission here; Looks strange.; 30 should be more than enough. <cut> > @@ -578,6 +464,10 @@ static bool extract_date_time(DATE_TIME_ > l_time->minute > 59 || l_time->second > 59) > goto err; > > + if ((fuzzy_date & TIME_NO_ZERO_DATE) && > + (l_time->year == 0 || l_time->month == 0 || l_time->day == 0)) > + goto err; > + This is something we didn't check before. If this is a compatibily issue, you should list it in your matrix. I tried to get the above to execute, but could not. I tried with: select str_to_date("1997-00-04 22:23:00","%Y-%m-%D"); but this returned "1997-00-04" independent of the sql mode. Please add a comment when the above case will be used. > @@ -1569,8 +1459,9 @@ void Item_func_curdate_local::store_now_ > */ > void Item_func_curdate_utc::store_now_in_TIME(MYSQL_TIME *now_time) > { > - my_tz_UTC->gmt_sec_to_TIME(now_time, > - (my_time_t)(current_thd->query_start())); > + THD *thd= current_thd; > + my_tz_UTC->gmt_sec_to_TIME(now_time, thd->query_start()); > + thd->time_zone_used= 1; > /* > We are not flagging this query as using time zone, since it uses fixed > UTC-SYSTEM time-zone. Why do you flag the time_zone_used when the function comment says that you don't need to do this? You also don't set time_zone_used in Item_func_now_utc::store_now_in_TIME() <cut> <cut> > bool Item_func_sysdate_local::get_date(MYSQL_TIME *res, > uint fuzzy_date __attribute__((unused))) > { > + MYSQL_TIME ltime; > store_now_in_TIME(&ltime); > *res= ltime; > return 0; > } Why not do directly: store_now_in_TIME(res); Actaully, I don't undestand why Item_func_sysdate_local() have to have a :store_now_in_TIME() function at all. As this function can only be called by a Item_func_sysdate_local object and we only call it from this place it would be more logical to have the code from store_now_in_time() here. It actually doesn't make sense that this function inherits from Item_func_now() as we get an extra store_now_in_TIME() call from Item_func_now::fix_length_and_dec() that we don't need or want. Not critical to do it now, but should be done eventually. <cut> > @@ -2050,42 +1845,14 @@ bool Item_func_from_unixtime::get_date(M > void Item_func_convert_tz::fix_length_and_dec() > { > collation.set(&my_charset_bin); > - decimals= 0; > - max_length= MAX_DATETIME_WIDTH*MY_CHARSET_BIN_MB_MAXLEN; > + max_length= MAX_DATETIME_WIDTH; Should probably be: max_length= MAX_DATETIME_WIDTH*MY_CHARSET_BIN_MB_MAXLEN; This is how all the other functions are defined. > + decimals= args[0]->decimals; > + if (decimals && decimals != NOT_FIXED_DEC) > + max_length+= min(decimals, MAX_SEC_PART_DIGITS) + 1; > maybe_null= 1; > } I was actually wondering why we don't have an Item_temporal_func::fix_fields() function that was defined to set the above as this should be the default for all DATETIME results. This would allow us to do something like: bool Item_func_convert_tz::fix_fields(thd, items) { decimals= args[0]->decimals; decimals= min(decimals, MAX_SEC_PART_DIGITS); return Item_temporal_func::fix_fields(thd, items); } And of course Item_temporal_func::fix_fields() could then check if decimals > MAX_SEC_PART_DIGITS and give an error. (Just an idea) <cut> > @@ -2137,8 +1908,7 @@ void Item_date_add_interval::fix_length_ > > collation.set(&my_charset_bin); > maybe_null=1; > - max_length=MAX_DATETIME_FULL_WIDTH*MY_CHARSET_BIN_MB_MAXLEN; > + max_length=MAX_DATETIME_WIDTH*MY_CHARSET_BIN_MB_MAXLEN; Please move setting of the above to where you do: if (decimals) max_length+= min(decimals, MAX_SEC_PART_DIGITS) + 1; So that the result is: max_length= MAX_DATETIME_WIDTH*MY_CHARSET_BIN_MB_MAXLEN; if (decimals) max_length+= min(decimals, MAX_SEC_PART_DIGITS) + 1; This makes it much easier to ensure that max_length is correct > @@ -2146,7 +1916,11 @@ void Item_date_add_interval::fix_length_ > > - If first arg is a MYSQL_TYPE_DATETIME result is MYSQL_TYPE_DATETIME > - If first arg is a MYSQL_TYPE_DATE and the interval type uses hours, > + minutes or seconds then type is MYSQL_TYPE_DATETIME > + otherwise it's MYSQL_TYPE_DATE > + - if first arg is a MYSQL_TYPE_TIME and the interval type isn't using > + anything larger than days, then the result is MYSQL_TYPE_TIME, > + otherwise - MYSQL_TYPE_DATETIME. > - Otherwise the result is MYSQL_TYPE_STRING > (This is because you can't know if the string contains a DATE, MYSQL_TIME or > DATETIME argument) > @@ -2163,6 +1937,20 @@ void Item_date_add_interval::fix_length_ > else > cached_field_type= MYSQL_TYPE_DATETIME; > } > + else if (arg0_field_type == MYSQL_TYPE_TIME) > + { > + if (int_type >= INTERVAL_DAY && int_type != INTERVAL_YEAR_MONTH) > + cached_field_type= arg0_field_type; > + else > + cached_field_type= MYSQL_TYPE_DATETIME; > + } > + if (int_type == INTERVAL_MICROSECOND || int_type >= INTERVAL_DAY_MICROSECOND) > + decimals= 6; > + else > + decimals= args[0]->decimals; Shouldn't we above use: decimals= min(args[0]->decimals, 6); I suspect that some of the other code may find it confusing if decimals have a too high value. I tried: select 20010101000203.000000004 + interval 1 day; and this did die in an assert, which seams to indicate the the above change is needed. > + if (decimals) > + max_length+= min(decimals, MAX_SEC_PART_DIGITS) + 1; > + value.alloc(max_length); > } > > > @@ -2172,7 +1960,7 @@ bool Item_date_add_interval::get_date(MY > { > INTERVAL interval; > > - if (args[0]->get_date(ltime, TIME_NO_ZERO_DATE) || > + if (args[0]->get_date(ltime, TIME_NO_ZERO_DATE|TIME_FUZZY_DATE) || Please add space around | to make the above readable > void Item_func_add_time::fix_length_and_dec() > { > enum_field_types arg0_field_type; > - decimals=0; > - max_length=MAX_DATETIME_FULL_WIDTH*MY_CHARSET_BIN_MB_MAXLEN; > + max_length= MAX_DATETIME_WIDTH; Move setting of max_length down before 'if (decimals)' (see earlier comment). > + decimals= max(args[0]->decimals, args[1]->decimals); Add: decimals= min(decimals, MAX_DATETIME_PRECISION); <cut> > + MYSQL_TIME copy= *ltime; > + Lazy_string_time str(&copy); > + > + check_time_range(ltime, decimals, &was_cut); > + if (was_cut) > + make_truncated_value_warning(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN, > + &str, MYSQL_TIMESTAMP_TIME, NullS); Move setting of Lazy_string_time str(&copy) inside 'if (was_cut)...' > +bool Item_func_timediff::get_date(MYSQL_TIME *ltime, uint fuzzy_date) > { > DBUG_ASSERT(fixed == 1); > longlong seconds; > long microseconds; > - int l_sign= 1; > - MYSQL_TIME l_time1 ,l_time2, l_time3; > + int l_sign= 1, was_cut= 0; > + MYSQL_TIME l_time1,l_time2,l_time3; > + Lazy_string_time str(&l_time3); Move setting of 'str' to where it's used. > @@ -2899,6 +2535,9 @@ String *Item_func_timediff::val_str(Stri > l_time1.time_type != l_time2.time_type) > goto null_date; > > + if (fuzzy_date & TIME_NO_ZERO_IN_DATE) > + goto null_date; It took some time to find a test to trigger this code. Please add a comment: /* The following test maybe true when we are called by a function that require a datetime object. For example when doing: select timediff('...') + interval 1 month */ In any case, this should be moved to start of the function as there is no way this function can ever return anything else than NULL if this flag is set and there is no reason to calculate any else in this case. > @@ -2915,16 +2554,22 @@ String *Item_func_timediff::val_str(Stri > if (l_time1.neg && (seconds || microseconds)) > l_time3.neg= 1-l_time3.neg; // Swap sign of result > Add comment here: /* Adjust second so that we don't loose information from (long) cast. This is safe to to as calc_time_from_sec() will overflow also for INT_MAX32 and we will get the warning we expect */ > + set_if_smaller(seconds, INT_MAX32); > calc_time_from_sec(&l_time3, (long) seconds, microseconds); > > + if ((fuzzy_date & TIME_NO_ZERO_DATE) && (seconds == 0) && (microseconds == 0)) > + goto null_date; TIME_NO_ZERO_DATE means that neither day or month should not be 0 As this is always the case for time_diff() we could merge this test with the above 'if (fuzzy_date & TIME_NO_ZERO_IN_DATE)' For some examples where this now goes strange: dayofyear(timediff('0000-00-01 00:00:05', '0000-00-01 00:00:05')) -> NULL dayofyear(timediff('0000-00-01 00:00:05', '0000-00-01 00:00:04')) -> 0 dayofyear(cast('0000-00-01 00:00:05' as datetime)) -> NULL I think it should return NULL in all cases (which is what happens if you move the TIME_NO_ZERO_DATE test as suggested above). <cut>> > > @@ -3292,36 +2930,45 @@ get_date_time_result_type(const char *fo > void Item_func_str_to_date::fix_length_and_dec() > { > maybe_null= 1; > - decimals=0; > cached_field_type= MYSQL_TYPE_DATETIME; > - max_length= MAX_DATETIME_FULL_WIDTH*MY_CHARSET_BIN_MB_MAXLEN; > - cached_timestamp_type= MYSQL_TIMESTAMP_NONE; > + max_length= MAX_DATETIME_WIDTH+MAX_SEC_PART_DIGITS; Add '*MY_CHARSET_BIN_MB_MAXLEN' for completeness here and in the other places we set max_length.. <cut> > +++ sql/item_timefunc.h 2011-03-23 12:31:06 +0000 > @@ -323,147 +323,100 @@ class Item_func_unix_timestamp :public I > }; > > > -class Item_func_time_to_sec :public Item_int_func > +class Item_func_time_to_sec :public Item_real_func > { > public: > - Item_func_time_to_sec(Item *item) :Item_int_func(item) {} > - longlong val_int(); > + Item_func_time_to_sec(Item *item) :Item_real_func(item) {} > const char *func_name() const { return "time_to_sec"; } > + double val_real(); > void fix_length_and_dec() > { > maybe_null= TRUE; > - decimals=0; > - max_length=10*MY_CHARSET_BIN_MB_MAXLEN; > + decimals=args[0]->decimals; > + max_length=17; max_length= 17*MY_CHARSET_BIN_MB_MAXLEN; <cut> > +class Item_temporal_func: public Item_func > { > public: > + Item_temporal_func() :Item_func() {} > + Item_temporal_func(Item *a) :Item_func(a) {} > + Item_temporal_func(Item *a, Item *b) :Item_func(a,b) {} > + Item_temporal_func(Item *a, Item *b, Item *c) :Item_func(a,b,c) {} > enum Item_result result_type () const { return STRING_RESULT; } > + enum_field_types field_type() const { return MYSQL_TYPE_DATETIME; } > String *val_str(String *str); > longlong val_int(); > + double val_real(); > + bool get_date(MYSQL_TIME *res, uint fuzzy_date) { DBUG_ASSERT(0); return 1; } > my_decimal *val_decimal(my_decimal *decimal_value) > + { return val_decimal_from_date(decimal_value); } > + Field *tmp_table_field(TABLE *table) > + { return tmp_table_field_from_field_type(table, 0); } > int save_in_field(Field *field, bool no_conversions) > + { return save_date_in_field(field); } > }; > See earlier suggestion to add a fix_fields() function and add a 'THD' variable. The only problem with this is to decide upon the value of 'decimal' for functions that depends on it's arguments for the value of decimal. One way to do this would bo first call item->fix_length_and_dec() and then use the value of decimals and field_type() to calculate max_length. Having a Item_temporal_func::fix_fields() would also make it trivial to ensure we have always a legal value for decimals! We also would not have to do the test: max_length+= (decimals ? min(decimals, MAX_SEC_PART_DIGITS)+1 : 0); In a lot of places. <cut> > @@ -563,16 +502,10 @@ class Item_func_now_utc :public Item_fun > class Item_func_sysdate_local :public Item_func_now Inheriting from Item_temporal_func() may be better. (See earlier comment) <cut> > +class Item_func_sec_to_time :public Item_timefunc > { > public: > + Item_func_sec_to_time(Item *item) :Item_timefunc(item) {} > + bool get_date(MYSQL_TIME *res, uint fuzzy_date); > void fix_length_and_dec() > { > collation.set(&my_charset_bin); > maybe_null=1; > + decimals= args[0]->decimals; > + if (decimals != NOT_FIXED_DEC && decimals > MAX_SEC_PART_DIGITS) > + decimals= MAX_SEC_PART_DIGITS; I am not sure that NOT_FIXED_DEC works for time; It may be better to always limit this to MAX_SEC_PART_DIGITS. <cut> > +class Item_date_typecast :public Item_temporal_typecast > { > public: > - Item_date_typecast(Item *a) :Item_typecast_maybe_null(a) {} > + Item_date_typecast(Item *a) :Item_temporal_typecast(a) {} > const char *func_name() const { return "cast_as_date"; } > - String *val_str(String *str); > bool get_date(MYSQL_TIME *ltime, uint fuzzy_date); > - bool get_time(MYSQL_TIME *ltime); > const char *cast_type() const { return "date"; } > enum_field_types field_type() const { return MYSQL_TYPE_DATE; } > - Field *tmp_table_field(TABLE *table) > - { > - return tmp_table_field_from_field_type(table, 0); > - } > void fix_length_and_dec() > { > collation.set(&my_charset_bin); > + decimals= 0; You don't need to set decimals to 0; This is already done in the item constructor. Of course, if we have Item_temporal::fix_fields() this function would not be needed at all... > + max_length= MAX_DATE_WIDTH; > maybe_null= 1; <cut> > +class Item_time_typecast :public Item_temporal_typecast > { > public: > - Item_time_typecast(Item *a) :Item_typecast_maybe_null(a) {} > + Item_time_typecast(Item *a, uint dec_arg) > + :Item_temporal_typecast(a) { decimals= dec_arg; } > const char *func_name() const { return "cast_as_time"; } > + bool get_date(MYSQL_TIME *ltime, uint fuzzy_date); > const char *cast_type() const { return "time"; } > enum_field_types field_type() const { return MYSQL_TYPE_TIME; } > + void fix_length_and_dec() > { > + collation.set(&my_charset_bin); > + maybe_null= 1; > + max_length= MIN_TIME_WIDTH; > + if (decimals == NOT_FIXED_DEC) > + decimals= args[0]->decimals; You proabaly should test for AUTO_SEC_PART_DIGITS as this is the define used in sql_yacc.yy > + if (decimals && decimals != NOT_FIXED_DEC) > + max_length+= min(decimals, MAX_SEC_PART_DIGITS) + 1; You need to limit decimals to be <= MAX_SEC_PART_DIGITS or you will get an assert in some code (see earlier test). However, I agree that this is nice: create table t1 (a double); insert into t1 values(1.0001),(1.000000000001) select time(a) from t1; -> | 00:00:01.000100 | | 00:00:01 | If I change the create to: create table t1 (a double(10,10)); the above will cause an assert in: my_time.c:1047: my_time_to_str: Assertion `digits >= 0 && digits <= 6' failed. <cut> > +class Item_func_last_day :public Item_datefunc > { > public: > - Item_func_last_day(Item *a) :Item_date(a) {} > + Item_func_last_day(Item *a) :Item_datefunc(a) {} > const char *func_name() const { return "last_day"; } > bool get_date(MYSQL_TIME *res, uint fuzzy_date); > + void fix_length_and_dec() > + { > + maybe_null=1; Better if Item_datefunc::fix_length_and_dec() or Item_temporal_func::fix_fields() sets maybe_null= 1 by default. > === modified file 'sql/log_event.cc' In the file you have: #ifdef MYSQL_CLIENT ... #ifdef MYSQL_CLIENT void free_table_map_log_event(...) #endif #endif Please remove the inner #ifdef MYSQL_CLIENT > @@ -3598,7 +3614,7 @@ bool Start_log_event_v3::write(IO_CACHE* > int2store(buff + ST_BINLOG_VER_OFFSET,binlog_version); > memcpy(buff + ST_SERVER_VER_OFFSET,server_version,ST_SERVER_VER_LEN); > if (!dont_set_created) > - created= when= get_time(); > + created= get_time(); Add a comment before get_time(): /* get_time sets 'when' and 'when_sec_part' as a side effect */ <cut> > @@ -1000,16 +1004,27 @@ class Log_event > { return 0; } > virtual bool write_data_body(IO_CACHE* file __attribute__((unused))) > { return 0; } > - inline time_t get_time() > + inline my_time_t get_time() > { > THD *tmp_thd; > if (when) > return when; > if (thd) > + { > + when= thd->start_time; > + when_sec_part= thd->start_time_sec_part; > + return when; > + } thd should alway be true. Can you at some point in time check when this is not the case and if needed set thd for the class ! > if ((tmp_thd= current_thd)) current_thd() should never fail. If it can, there should be a comment in the code about this! > + { > + when= tmp_thd->start_time; > + when_sec_part= tmp_thd->start_time_sec_part; > + return when; > + } > + my_hrtime_t hrtime= my_hrtime(); > + when= hrtime_to_my_time(hrtime); > + when_sec_part= hrtime_sec_part(hrtime); > + return when; > } > #endif > virtual Log_event_type get_type_code() = 0; > > === modified file 'sql/mysql_priv.h' <cut> > +/* > + to unify the code that differs only in the argument passed to the > + error message (string vs. number) > + > + We pass this container around, and only convert the number > + to a string when necessary. > +*/ Good idea, but please consider using a better name for the string. This is more a value wrapper than an string. > +class Lazy_string > +{ > +public: > + Lazy_string() {} > + virtual ~Lazy_string() {} > + virtual void copy(String *str) const = 0; > +}; > + <cut> > +class Lazy_string_dbl: public Lazy_string dbl -> double (No reason to save a couple of characters here) <cut> > === modified file 'sql/set_var.cc' > --- sql/set_var.cc 2010-10-20 18:21:40 +0000 > +++ sql/set_var.cc 2011-03-01 12:24:36 +0000 > @@ -2715,38 +2715,38 @@ int set_var_collation_client::update(THD > > bool sys_var_timestamp::check(THD *thd, set_var *var) > { > + double val= var->value->val_real(); > + if (val < 0 || val > MY_TIME_T_MAX) > { > my_message(ER_UNKNOWN_ERROR, > "This version of MySQL doesn't support dates later than 2038", > MYF(0)); > return TRUE; > } > + var->save_result.ulonglong_value= hrtime_from_time(var->value->val_real()); > return FALSE; > } > > > bool sys_var_timestamp::update(THD *thd, set_var *var) > { > - thd->set_time((time_t) var->save_result.ulonglong_value); > + my_hrtime_t hrtime = { var->save_result.ulonglong_value }; > + thd->set_time(hrtime); > return FALSE; > } > > > void sys_var_timestamp::set_default(THD *thd, enum_var_type type) > { > - thd->user_time=0; > + thd->user_time.val= 0; > } > > > uchar *sys_var_timestamp::value_ptr(THD *thd, enum_var_type type, > LEX_STRING *base) > { > - thd->sys_var_tmp.long_value= (long) thd->start_time; > - return (uchar*) &thd->sys_var_tmp.long_value; > + thd->sys_var_tmp.double_value= thd->start_time + thd->start_time_sec_part/1e6; > + return (uchar*) &thd->sys_var_tmp.double_value; > } > > > @@ -3045,10 +3045,10 @@ void sys_var_microseconds::set_default(T > uchar *sys_var_microseconds::value_ptr(THD *thd, enum_var_type type, > LEX_STRING *base) > { > - thd->tmp_double_value= (double) ((type == OPT_GLOBAL) ? > + thd->sys_var_tmp.double_value= (double) ((type == OPT_GLOBAL) ? > global_system_variables.*offset : > thd->variables.*offset) / 1000000.0; Change 100000.0 to same define constant as 1e6 > > === modified file 'sql/sql_class.cc' > @@ -2338,7 +2338,7 @@ bool select_max_min_finder_subselect::se > case DECIMAL_RESULT: > op= &select_max_min_finder_subselect::cmp_decimal; > break; > - case ROW_RESULT: > + default: Add back ROW_RESULT and add TIME_RESULT > +++ sql/sql_class.h 2011-03-26 10:59:34 +0000 <cut> > inline void set_time() > { > + if (user_time.val) > { > + start_time= hrtime_to_my_time(user_time); > + start_time_sec_part= hrtime_sec_part(user_time); > start_utime= utime_after_lock= my_micro_time(); > } > else > + { > + my_hrtime_t hrtime; > + my_timediff_t timediff; > + my_micro_and_hrtime(&timediff, &hrtime); > + start_time= hrtime_to_my_time(hrtime); > + start_time_sec_part= hrtime_sec_part(hrtime); > + utime_after_lock= start_utime= timediff.val; > + } For the future: As we are now always calling hrtime(), we should consider using hrtime also for start_utime. At least on systems with a fast hrtime() better to use this for both my_micro_time() and hrtime(). > +++ sql/sql_prepare.cc 2011-03-01 12:24:36 +0000 > @@ -487,8 +487,7 @@ static void set_param_time(Item_param *p > } > else > set_zero_time(&tm, MYSQL_TIMESTAMP_TIME); > - param->set_time(&tm, MYSQL_TIMESTAMP_TIME, > - MAX_TIME_WIDTH * MY_CHARSET_BIN_MB_MAXLEN); > + param->set_time(&tm, MYSQL_TIMESTAMP_TIME, MAX_TIME_FULL_WIDTH); Should be: MAX_TIME_FULL_WIDTH * MY_CHARSET_BIN_MB_MAXLEN > === modified file 'sql/sql_select.cc' > @@ -9455,7 +9445,7 @@ test_if_equality_guarantees_uniqueness(I > { > return r->const_item() && > /* elements must be compared as dates */ > - (Arg_comparator::can_compare_as_dates(l, r, 0) || > + (l->cmp_type() == TIME_RESULT || > /* or of the same result type */ > (r->result_type() == l->result_type() && > /* and must have the same collation if compared as strings */ Add a comment: /* It's enough to test 'l' for TIME_RESULT as only 'l' can be a field in this context. */ > === modified file 'sql/sql_show.cc' > --- sql/sql_show.cc 2010-11-02 13:20:02 +0000 <cut> > @@ -6288,6 +6305,8 @@ ST_FIELD_INFO columns_fields_info[]= > 0, (MY_I_S_MAYBE_NULL | MY_I_S_UNSIGNED), 0, OPEN_FRM_ONLY}, > {"NUMERIC_SCALE", MY_INT64_NUM_DECIMAL_DIGITS , MYSQL_TYPE_LONGLONG, > 0, (MY_I_S_MAYBE_NULL | MY_I_S_UNSIGNED), 0, OPEN_FRM_ONLY}, > + {"DATETIME_PRECISION", MY_INT64_NUM_DECIMAL_DIGITS, MYSQL_TYPE_LONGLONG, > + 0, (MY_I_S_MAYBE_NULL | MY_I_S_UNSIGNED), 0, OPEN_FRM_ONLY}, You have added a new field; Please add to compability documentation! > {"CHARACTER_SET_NAME", MY_CS_NAME_SIZE, MYSQL_TYPE_STRING, 0, 1, 0, > OPEN_FRM_ONLY}, > {"COLLATION_NAME", MY_CS_NAME_SIZE, MYSQL_TYPE_STRING, 0, 1, "Collation", > > +++ sql/sql_table.cc 2011-03-01 12:24:36 +0000 > @@ -34,26 +34,16 @@ const char *primary_key_name="PRIMARY"; > > static bool check_if_keyname_exists(const char *name,KEY *start, KEY *end); > static char *make_unique_key_name(const char *field_name,KEY *start,KEY *end); > -static int copy_data_between_tables(TABLE *from,TABLE *to, > - List<Create_field> &create, bool ignore, > - uint order_num, ORDER *order, > - ha_rows *copied,ha_rows *deleted, > - enum enum_enable_or_disable keys_onoff, > - bool error_if_not_empty); > +static int copy_data_between_tables(TABLE *,TABLE *, List<Create_field> &, bool, > + uint, ORDER *, ha_rows *,ha_rows *, > + enum enum_enable_or_disable, bool); I don't understand why you did this change here and in other places. I usually prefer to have the field names also in the prototype as it makes the prototype easier to read! <cut> > void make_time(const DATE_TIME_FORMAT *format __attribute__((unused)), > const MYSQL_TIME *l_time, String *str) > { > + uint length= (uint) my_time_to_str(l_time, (char*) str->ptr(), 0); > str->length(length); > str->set_charset(&my_charset_bin); It would be best to do str->alloc(MAX_DATETIME_FULL_WIDTH) at start of function. > } > @@ -713,15 +693,15 @@ void make_date(const DATE_TIME_FORMAT *f > void make_datetime(const DATE_TIME_FORMAT *format __attribute__((unused)), > const MYSQL_TIME *l_time, String *str) > { > + uint length= (uint) my_datetime_to_str(l_time, (char*) str->ptr(), 0); It would be best to do str->alloc(MAX_DATETIME_FULL_WIDTH) at start of function. > str->length(length); > str->set_charset(&my_charset_bin); > } <cut> > bool date_add_interval(MYSQL_TIME *ltime, interval_type int_type, INTERVAL interval) > { > long period, sign; > > + sign= (interval.neg == ltime->neg ? 1 : -1); Add a comment that shows why the above is needed > > switch (int_type) { > case INTERVAL_SECOND: > @@ -789,35 +770,29 @@ bool date_add_interval(MYSQL_TIME *ltime > case INTERVAL_DAY_SECOND: > case INTERVAL_DAY_MINUTE: > case INTERVAL_DAY_HOUR: > + case INTERVAL_DAY: > { > + longlong usec, daynr; > + my_bool neg= ltime->neg; > + enum enum_mysql_timestamp_type time_type= ltime->time_type; > + > + if (ltime->time_type != MYSQL_TIMESTAMP_TIME) Change to: if (time_type != MYSQL_TIMESTAMP_TIME) > + ltime->day+= calc_daynr(ltime->year, ltime->month, 1) - 1; > + > + usec= COMBINE(ltime) + sign*COMBINE(&interval); > + > + unpack_time(usec, ltime); > + ltime->time_type= time_type; > + ltime->neg^= neg; > + > + if (time_type == MYSQL_TIMESTAMP_TIME) > + break; > + > + if (int_type != INTERVAL_DAY) > + ltime->time_type= MYSQL_TIMESTAMP_DATETIME; // Return full date > + > + daynr= ltime->day + 32*(ltime->month + 13*ltime->year); Add a comment for the above: /* unpack_time() calculates month and day based on the assumption that the date was packed with 'pack_time()'. The following code restores the number of days. */ another option would be to do: daynr= usec / 1000000/24/60/60; This would at least be easier to understand... <cut> Here is some bugs I found: CREATE TABLE t3 (a time(6)); insert into t3 values(-100); select * from t3; ->8299:28:36.710656 If you use just (a time) you get: "-00:01:00" which is correct. cast("2101-00-01 02:03:04" as datetime) -> "2101-00-01 02:03:04" but cast(cast("2101-00-01 02:03:04" as datetime) as time) -> NULL Other things: Item_func_unix_timestamp() should return microseconds. Otherwise one can't acccess the full precision of Field_timestamp. One should also be able to do: select unix_timestamp(now(6)); For fast access to the current clock in maximum precision. --------- general_log.timestamp should be of type timestamp(6) slow_log.query_time and slow_log.lock_time() should be of type TIME(6) ----------- Regards, Monty

2 3

[Maria-developers] feedback on proposed fix to MySQL bug 57430
by Zardosht Kasheff 19 May '11

19 May '11

Hello all, Continuing with patch contributions (as encouraged at the storage engine summit last Friday), here is a very simple patch we have to fix MySQL bug 57430 (http://bugs.mysql.com/bug.php?id=57430). The problem is described in the bug report we sent to MySQL. We think this simple fix addresses the problem that we ran into, but we do not understand the code well enough to know if this fix is sufficient. Feedback is welcome. Thanks -Zardosht

3 4

[Maria-developers] proposed patches for MWL#113: many clustered keys (not only primary)
by Zardosht Kasheff 19 May '11

19 May '11

Hello all, First off, thanks for hosting the storage engine summit last Friday. At the beginning of the day, I was encouraged to send the patches that I had for MWL#113 to the MariaDB developers list, in hopes possibly getting them into MariaDB 5.3. Here they are. They are patched off of MariaDB 5.2.3. They are all relatively simple. Here is what the patches do. - 1.diff - add an index flag HA_CLUSTERED_INDEX. If a storage engine exposes this flag for an index, then that index is clustered. This was proposed in http://bugs.mysql.com/bug.php?id=51687 - 2.diff - simple fix to get select ... order by DESC working - 3.diff - fix for index merge and clustering keys. This was inspired by Sergey Petrunya originally proposing http://lists.mysql.com/internals/37165. It has been altered a bit now that HA_CLUSTERED_INDEX is added. - 4.diff - extend fix of MySQL bug 50843 for clustering keys - http://lists.askmonty.org/pipermail/commits/2010-October/000626.html. I cannot find this checked in anywhere, but it seems to be a good patch for clustering keys. This was inspired from http://www.mail-archive.com/maria-developers@lists.launchpad.net/msg03491.h…. We currently have a hacky workaround for this issue, but this seems like the right fix. I also have one more fix for clustering keys and joins (in make_join_readinfo), but I want to look over it and before sending it. Also, if for whatever reason all of the patches are not good enough for checking in, please consider some subset of the patches. This is by no means "all or none". That is why I broke it up into separate diff files. I would love to hear feedback on these patches. Thanks -Zardosht

3 4

[Maria-developers] proposed patch to add new handler error code
by Zardosht Kasheff 18 May '11

18 May '11

Hello all, In continuing with the suggestion to provide patches (given at the storage engine summit last friday), here is another one (last one for tonight). Our storage engine wanted to add the ability to return an error to the user that states that the disk is full. To do so, we added, we added the handler error code HA_ERR_DISK_FULL. When returned, the error ER_DISK_FULL (which already exists) is returned to the user. This does not have a bug number or worklog number. Is there something I should do besides sending this patch to the list? Thanks -Zardosht

2 2

[Maria-developers] Dynamic column for review
by Oleksandr Byelkin 29 Apr '11

29 Apr '11

Hi! Here is 2 patches: one cut version - to review, other one full version.

1 0

Re: [Maria-developers] [Commits] Rev 2908: Fixed LP bugs #717577, #724942. in file:///home/igor/maria/maria-5.3-bug717577/
by Sergey Petrunya 25 Apr '11

25 Apr '11

Hello Igor, First, an overall comment: there are lots of typos/coding style violations in the patch. To reduce amount of effort spent on such things, I was just fixing them as I saw them and I'm attaching the patch with all the fixes (i.e. this patch should be applied on top of the patch that I was reviewing). More important comments are bellow, marked with 'psergey:' On Fri, Mar 25, 2011 at 10:31:19PM -0700, Igor Babaev wrote: > At file:///home/igor/maria/maria-5.3-bug717577/ > > ------------------------------------------------------------ > revno: 2908 > revision-id: igor(a)askmonty.org-20110326053117-x50cah9krd4f1345 > parent: wlad(a)montyprogram.com-20110212174322-f7ptnc0u32vasdwy > committer: Igor Babaev <igor(a)askmonty.org> > branch nick: maria-5.3-bug717577 > timestamp: Fri 2011-03-25 22:31:17 -0700 > message: > Fixed LP bugs #717577, #724942. > > Both these two bugs happened due to the following problem. > When a view column is referenced in the query an Item_direct_view_ref > object is created that is refers to the Item_field for the column. > All references to the same view column refer to the same Item_field. > Different references can belong to different AND/OR levels and, > as a result, can be included in different Item_equal object. > These Item_equal objects may include different constant objects. > If these constant objects are substituted for the Item_field created > for a view column we have a conflict situation when the second > substitution annuls the first substitution. This leads to > wrong result sets returned by the query. Bug #724942 demonstrates > such an erroneous behaviour. > Test case of the bug #717577 produces wrong result sets because best > equal fields of the multiple equalities built for different OR levels > of the WHERE condition differs. The subsitution for the best equal field > in the second OR branch overwrites the the substitution made for the > first branch. > > To avoid such conflicts we have to substitute for the references > to the view columns rather than for the underlying field items. > To make such substitutions possible we have to include into > multiple equalities references to view columns rather than > field items created for such columns. > > This patch modifies the Item_equal class to include references > to view columns into multiple equality objects. It also performs > a clean up of the class methods and adds more comments. The methods > of the Item_direct_view_ref class that assist substitutions for > references to view columns has been also added by this patch. > === modified file 'sql/item.cc' > --- a/sql/item.cc 2011-02-01 03:33:32 +0000 > +++ b/sql/item.cc 2011-03-26 05:31:17 +0000 > @@ -7226,6 +7233,129 @@ > return FALSE; > } > > + > +Item_equal *Item_direct_view_ref::find_item_equal(COND_EQUAL *cond_equal) > +{ > + Item* field_item= real_item(); > + if (field_item->type() != FIELD_ITEM) > + return NULL; > + return ((Item_field *) field_item)->find_item_equal(cond_equal); > +} > + > + > +/** > + Check whether a reference to field item can be substituted b an equal item > + > + @details > + The function checks whether a substitution of a reference to field item for > + an equal item is valid. > + > + @param arg *arg != NULL && **arg <-> the reference is in the context > + where substitution for an equal item is valid > + > + @note > + See also the note for Item_field::subst_argument_checker > + > + @retval > + TRUE substitution is valid > + @retval > + FALSE otherwise > +*/ > +bool Item_direct_view_ref::subst_argument_checker(uchar **arg) > +{ > + bool res= (!(*arg) && (result_type() != STRING_RESULT)) || > + ((*arg) && (**arg)); > + if (*arg) > + **arg= (uchar) 0; psergey: What is the above for? Do I understand it correctly that this is needed so that Item_field that this item is wrapping is not substituted? Please add a comment. > + return res; > +} > + > + > +/** > + Set a pointer to the multiple equality the view field reference belongs to > + (if any). > + > + @details > + The function looks for a multiple equality containing this item of the type > + Item_direct_view_ref among those referenced by arg. > + In the case such equality exists the function does the following. > + If the found multiple equality contains a constant, then the item > + is substituted for this constant, otherwise the function sets a pointer > + to the multiple equality in the item. > + > + @param arg reference to list of multiple equalities where > + the item (this object) is to be looked for > + > + @note > + This function is supposed to be called as a callback parameter in calls > + of the compile method. > + > + @note > + The function calls Item_field::equal_fields_propagator for the field item > + this->real_item() to do the job. Then it takes the pointer to equal_item > + from this field item and assigns it to this->item_equal. > + > + @return > + - pointer to the replacing constant item, if the field item was substituted > + - pointer to the field item, otherwise. > +*/ > + > +Item *Item_direct_view_ref::equal_fields_propagator(uchar *arg) > +{ > + Item *field_item= real_item(); > + if (field_item->type() != FIELD_ITEM) > + return this; > + Item *item= field_item->equal_fields_propagator(arg); > + set_item_equal(field_item->get_item_equal()); > + field_item->set_item_equal(0); psergey: Please use NULL here, we use that for pointers. > + if (item != field_item) > + return item; > + return this; > +} > + ... > === modified file 'sql/item_cmpfunc.cc' > --- a/sql/item_cmpfunc.cc 2011-02-06 04:57:03 +0000 > +++ b/sql/item_cmpfunc.cc 2011-03-26 05:31:17 +0000 > @@ -5516,43 +5516,93 @@ > return 0; > } > > -Item_equal::Item_equal(Item_field *f1, Item_field *f2) > - : Item_bool_func(), const_item(0), eval_item(0), cond_false(0), > - compare_as_dates(FALSE) > -{ > - const_item_cache= 0; > - fields.push_back(f1); > - fields.push_back(f2); > -} > - > -Item_equal::Item_equal(Item *c, Item_field *f) > + > +/** > + Construct a minimal multiple equality item > + > + @param f1 the first equal item > + @param f2 the second equal item > + @param with_const_item TRUE if the first item is constant > + > + @details > + The constructor builds a new item equal object for the equality f1=f2. > + One if the equal items can be constant. If this is the case it is passed > + always as the first parameter and the parameter with_const_item serves > + as an indicator of this case. > + Currently any non-constant parameter items must refer to an item of the > + of the type FIELD_ITEM. psergey: The above is still true for the case where passed item is an Item_direct_view_ref which refers to an Item_field. The wording may be confusing for the new readers, though. Please change to explicitly indicate that f1 and f2 may point to Item_field or Item_direct_view_ref(Item_field) > +*/ > + > +Item_equal::Item_equal(Item *f1, Item *f2, bool with_const_item) > : Item_bool_func(), eval_item(0), cond_false(0) > { > const_item_cache= 0; > - fields.push_back(f); > - const_item= c; > - compare_as_dates= f->is_datetime(); > + with_const= with_const_item; > + compare_as_dates= with_const_item && f2->is_datetime(); > + equal_items.push_back(f1); > + equal_items.push_back(f2); > } > > > +/** > + Copy constructor for a multiple equality > + > + @param item_equal source item for the constructor > + > + @details > + The function creates a copy of an Item_equal object. > + This constructor is used when an item belongs to a multiple equality > + of an upper level (an upper AND/OR level or an upper level of a nested > + outer join). > +*/ > + > Item_equal::Item_equal(Item_equal *item_equal) > : Item_bool_func(), eval_item(0), cond_false(0) > { > const_item_cache= 0; > - List_iterator_fast<Item_field> li(item_equal->fields); > - Item_field *item; > + List_iterator_fast<Item> li(item_equal->equal_items); > + Item *item; > while ((item= li++)) > { > - fields.push_back(item); > + equal_items.push_back(item); > } > - const_item= item_equal->const_item; > + with_const= item_equal->with_const; > compare_as_dates= item_equal->compare_as_dates; > cond_false= item_equal->cond_false; > } > > > -void Item_equal::compare_const(Item *c) > +/* > + @brief > + Add a constant item to the Item_equal object > + > + @param[in] c the constant to add > + @param[in] f item from the list equal_items the item c is equal to > + (this parameter is optional) > + > + @details > + The method adds the constant item c to the list equal_items. If the list > + hasn't not contained any constant item yet the item c is just added > + to the very beginning of the list. Otherwise the value of c is compared > + with the value of the constant item from equal_items. If they are not > + equal cond_false is set to TRUE. This serves as an indicator that this > + Item_equal is always FALSE. psergey: Does function comment really need to duplicate the function body? In my opinion, the above should be split into multiple comments inside the function body. > + The optional parameter f is used to adjust the flag compare_as_dates. > +*/ > + > +void Item_equal::add_const(Item *c, Item *f) > { > + if (cond_false) > + return; > + if (!with_const) > + { > + with_const= TRUE; > + if (f) > + compare_as_dates= f->is_datetime(); > + equal_items.push_front(c); > + return; > + } > + Item *const_item= get_const(); > if (compare_as_dates) > { > cmp.set_datetime_cmp_func(this, &c, &const_item); ... > @@ -5811,19 +5911,15 @@ > return Item_func::transform(transformer, arg); > } > > + > void Item_equal::print(String *str, enum_query_type query_type) > { > str->append(func_name()); > str->append('('); > - List_iterator_fast<Item_field> it(fields); > + List_iterator_fast<Item> it(equal_items); > Item *item; > - if (const_item) > - const_item->print(str, query_type); > - else > - { > - item= it++; > - item->print(str, query_type); > - } > + item= it++; > + item->print(str, query_type); > while ((item= it++)) > { > str->append(','); > @@ -5834,6 +5930,14 @@ > } > > > +CHARSET_INFO *Item_equal::compare_collation() psergey: One expects compare_xxx() functions to compare something, in particular this function looks like it's going to compare some collation. Could you please rename it to e.g. comparison_collation()? > +{ > + Item_equal_fields_iterator it(*this); > + Item *item= it++; > + return item->collation.collation; > +} > + > + > /* > @brief Get the first equal field of multiple equality. > @param[in] field the field to get equal field to ... > === modified file 'sql/item_cmpfunc.h' > --- a/sql/item_cmpfunc.h 2010-10-10 14:18:11 +0000 > +++ b/sql/item_cmpfunc.h 2011-03-26 05:31:17 +0000 > @@ -1672,23 +1707,47 @@ > }; > > psergey: Please add a one-line comment describing what this class does. > -class Item_equal_iterator : public List_iterator_fast<Item_field> > +class Item_equal_fields_iterator : public List_iterator_fast<Item> > { > + Item_equal *item_equal; > + Item *curr_item; > public: > - inline Item_equal_iterator(Item_equal &item_equal) > - :List_iterator_fast<Item_field> (item_equal.fields) > - {} > - inline Item_field* operator++(int) > - { > - Item_field *item= (*(List_iterator_fast<Item_field> *) this)++; > - return item; > - } > - inline void rewind(void) > - { > - List_iterator_fast<Item_field>::rewind(); > - } > + Item_equal_fields_iterator(Item_equal &item_eq) > + :List_iterator_fast<Item> (item_eq.equal_items) > + { > + curr_item= NULL; > + item_equal= &item_eq; > + if (item_eq.with_const) > + { > + List_iterator_fast<Item> *list_it= (List_iterator_fast<Item> *) this; psergey: Why do redundant explicit typecasting? > + curr_item= (*list_it)++; > + } > + } > + Item* operator++(int) > + { > + List_iterator_fast<Item> *list_it= (List_iterator_fast<Item> *) this; psergey: Why do redundant explicit typecasting? > + curr_item= (*list_it)++; > + return curr_item; > + } > + Item ** ref() > + { > + return List_iterator_fast<Item>::ref(); > + } > + void rewind(void) > + { > + List_iterator_fast<Item> *list_it= (List_iterator_fast<Item> *) this; > + list_it->rewind(); > + if (item_equal->with_const) > + curr_item= (*list_it)++; > + } > + Field *get_curr_field() > + { > + Item_field *item= (Item_field *) (curr_item->real_item()); > + return item->field; > + } > }; > > + > class Item_cond_and :public Item_cond > { > public: > > === modified file 'sql/sql_select.cc' > --- a/sql/sql_select.cc 2011-02-11 10:27:35 +0000 > +++ b/sql/sql_select.cc 2011-03-26 05:31:17 +0000 > @@ -9623,7 +9630,8 @@ > args->concat((List<Item> *)&cond_equal.current_level); > } > } > - else if (cond->type() == Item::FUNC_ITEM) > + else if (cond->type() == Item::FUNC_ITEM || > + cond->real_item()->type() == Item::FIELD_ITEM) psergey: As far as I understand the last part of the condition is there so that direct references like ... AND view.column AND ... are processed with subst_argument_checker/equal_fields_propagator. Is it intentional that direct references like ... AND base_table.column AND ... are now processed with subst_argument_checker/equal_fields_propagator as well? > { > List<Item> eq_list; > /* -- BR Sergey -- Sergey Petrunia, Software Developer Monty Program AB, http://askmonty.org Blog: http://s.petrunia.net/blog

2 3

Re: [Maria-developers] A serious merge bug in 5.3.
by Sergei Golubchik 24 Apr '11

24 Apr '11

Hi, Igor! On Apr 24, Igor Babaev wrote: > > When debugging I discovered the following bad code in sql_select.cc of > the current 5.3 tree: > tmp= table->file->read_time(key, 1, > min(tmp,s->worst_seeks)-1); <= !!!!! ? > tmp*= record_count; > > gannotate says that it appeared in the code after your merge with 5.2. > > The fact is that 5.2 contains the correct expression here > min(tmp,s->worst_seeks) > not > min(tmp,s->worst_seeks)-1 > > As a result if the number of records is 1 then min(tmp,s->worst_seeks)-1 > is equal to 0 and we pass 0 as the last parameter to > table->file->read_time. Naturally it returns 0. > > Will you fix this problem or you'd rather want me to do it? In 5.2 it is tmp= record_count*min(tmp,s->worst_seeks); In 5.3, record_count* is moved to a separate line, so it should just be tmp= min(tmp,s->worst_seeks); But it uses table->file->read_time(). Which is defined as virtual double read_time(uint index, uint ranges, ha_rows rows) { return rows2double(ranges+rows); } so, when ranges=1 and we pass rows=min(tmp,s->worst_seeks)-1, we get the same value of min(tmp,s->worst_seeks) as the result. This was the change of introducing handler::keyread_time(), and at the same time using handler::read_time() where appropriate, to have all values comparable. Regards, Sergei

2 2

[Maria-developers] MariaDB/MySQL error code issue
by Kristian Nielsen 19 Apr '11

19 Apr '11

Hi, For merging MySQL into MariaDB, we need to work out what to do about error code. The problem occurs as (different) new error messages are added in both MySQL and MariaDB. When we then merge from MySQL, we get conflicts between the error numbers. If we do not handle this somehow, it means that error numbers for a given error message will be different, either between MySQL and MariaDB, or alternatively between one version of MariaDB and another (depending on how conflicts are resolved). The problem with error code changing is that this makes it much harder for applications to check error codes and handle errors as it want in a portable way between MySQL and MariaDB. I think we have these options: 1. Cooperate with MySQL@Oracle, so that whenever we add an error code in MariaDB, the corresponding code is also reserved in MySQL (even if not used), and vice versa. 2. Create different "namespaces" for error codes added to MariaDB and MySQL, eg. new MariaDB codes are assigned with some offset to MySQL codes, so we avoid conflicts. 3. Keep MariaDB error code numbers stable, re-assign MySQL error code numbers when merging from MySQL (I rather dislike this as well because it introduces incompatibility with MySQL). 4. Keep MySQL error code numbers when merging from MySQL, so that MariaDB-only errors will fluctuate between releases (I really dislike this option). It seems option (4) is the best option. Is someone from MySQL/Oracle willing to work on getting such a procedure running? Failing that, it seems (3) is the best option. In this case, we need (at least) to assign a suitable error code range to new MariaDB errors and change the source code to be able to handle this (eg. maybe we need a sql/share/errmsg-mariadb.txt or something). We will also need to handle the error codes already assigned to virtual columns, which I think already conflict with new MySQL error codes. I would suggest doing a one-time incompatible change to map them into the MariaDB range. So, any hope for getting solution (4) working with MySQL@Oracle? And failing that, any opinions on how to proceed? Thanks, - Kristian.

4 6

[Maria-developers] WL#198 New (by Sergei): Roles
by worklog-noreply＠askmonty.org 19 Apr '11

19 Apr '11

----------------------------------------------------------------------- WORKLOG TASK -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- TASK...........: Roles CREATION DATE..: Tue, 19 Apr 2011, 10:01 SUPERVISOR.....: IMPLEMENTOR....: COPIES TO......: CATEGORY.......: Server-RawIdeaBin TASK ID........: 198 (http://askmonty.org/worklog/?tid=198) VERSION........: WorkLog-4.0 STATUS.........: Un-Assigned PRIORITY.......: 60 WORKED HOURS...: 0 ESTIMATE.......: 250 (hours remain) ORIG. ESTIMATE.: 250 PROGRESS NOTES: DESCRIPTION: Roles, as defined by the SQL standard. See also http://forge.mysql.com/worklog/task.php?id=988 ESTIMATED WORK TIME ESTIMATED COMPLETION DATE ----------------------------------------------------------------------- WorkLog (v4.0.0)

1 0

[Maria-developers] Testing group commit with XtraBackup
by Kristian Nielsen 19 Apr '11

19 Apr '11

Hi Philip, I thought of another test of group commit that ought to be done (not sure why I didn't think to mention this earlier, sorry about that). It's not urgent, just something for the list. The test is basically the same you already did for non-blocking `mysqldump --master-data --single-transaction` used to provision a slave, but using XtraBackup instead of mysqldump. >From what I could learn, the original reason that InnoDB added the mutex that kills group commit when binlog is enabled is the following: To ensure that commit order in the InnoDB transaction log is the same as in the binlog. This is needed when XtraBackup (innobackup really, but they work the same and XtraBackup is Free Software) takes a non-blocking backup of the InnoDB data (only), and this is used to provision a new slave. If commit order was different, there might not be any binlog position on the master corresponding to the backup snapshot used to setup the new slave. Now, my group commit work also guarantees consistent commit order, but using a completely different method that does not kill group commit. So we ought to test this. So the test would be similar to your existing mysqldump test. But while the master is running, we would use non-blocking XtraBackup to take a snapshot. This snapshot we would then restore onto a new slave. The slave would then be configured with CHANGE MASTER TO, I believe one gets the correct binlog position to start from in the log file of XtraDB and/or slave mysqld (I need to check up on details). Then the test proceeds as the mysqldump case, letting the slave catch up and checking that replication is ok etc. What do you think? Now, to do this we would need to be able to use XtraBackup. I am wondering if it wouldn't make sense to at the same time include XtraBackup into MariaDB? XtraBackup seems to me quite mature already, and a very good product besides. It is arguably long overdue for us to include it. So if we include XtraBackup in mariadb-5.2-rpl (/5.3), I think this should be possible to setup without too much trouble (I can provide specific details/scripts for how to actually run XtraBackup and setup slave start position, I just need to research that a bit). Or we could also do the test first with XtraBackup as external, shouldn't be much harder, and would give us some experience with it. What do you think? Maybe we can look at it in more detail in Lisbon. - Kristian.

2 3