maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

Thanks! The patch is pretty much ok.
There are only few comments/questions, see below.

On Aug 29, Alexander Barkov wrote:
>    Hi,
> 
> Please review my patch for MDEV-5528.
> 
> It also fixes a bug:
> 
> MDEV-6649 Different warnings for TIME and TIME(N) when 
> @@old_mode=zero_date_time_cast

Please, try to put different changes in different commit.
In git you can use "git add -p", "git stash",
"git rebase --interactive", and other commands to split all your changes
in logical commits.

> === modified file 'mysql-test/r/ctype_binary.result'
> --- mysql-test/r/ctype_binary.result	2014-05-09 10:35:11 +0000
> +++ mysql-test/r/ctype_binary.result	2014-08-29 05:37:33 +0000
> @@ -2769,7 +2769,7 @@ id	select_type	table	type	possible_keys
>  ALTER TABLE t1 MODIFY date_column DATETIME DEFAULT NULL;
>  EXPLAIN SELECT * FROM t1 WHERE date_column BETWEEN '2010-09-01' AND '2010-10-01';
>  id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> -1	SIMPLE	t1	range	date_column	date_column	9	NULL	1	Using index condition
> +1	SIMPLE	t1	range	date_column	date_column	6	NULL	1	Using index condition

Hm. So, you've changed the default DATETIME(0) format too?

>  DROP TABLE t1;
>  #
>  # Bug #31384 	DATE_ADD() and DATE_SUB() return binary data
> 
> === modified file 'mysql-test/r/distinct.result'
> --- mysql-test/r/distinct.result	2014-04-22 21:39:57 +0000
> +++ mysql-test/r/distinct.result	2014-08-29 07:17:11 +0000
> @@ -926,8 +926,8 @@ SELECT STRAIGHT_JOIN DISTINCT t1.id  FRO
>  t1, v1, t2 WHERE v1.id = t2.i AND t1.i1 = v1.i1 AND t2.i != 3;
>  id
>  7
> -8
>  9
> +8
>  18
>  20
>  24

I think, the change itself is ok - this SELECT uses internal temporary table, it's HEAP,
with HASH indexes, and the hash value of the datetime(0) column has changed when you changed the storage
format. But to keep results stable better add --sorted_result to the test file.

> === modified file 'mysql-test/r/old-mode.result'
> --- mysql-test/r/old-mode.result	2014-06-06 06:29:52 +0000
> +++ mysql-test/r/old-mode.result	2014-08-27 10:20:12 +0000
> @@ -101,3 +101,29 @@ NULL
>  Warning	1292	Incorrect datetime value: '0000-00-00 00:20:12'
>  Warning	1292	Truncated incorrect datetime value: '-00:20:12'
>  DROP TABLE t1;
> +#
> +# MDEV-6649 Different warnings for TIME and TIME(N) when @@old_mode=zero_date_time_cast
> +#

Having this bugfix in this commit means that it'll only go in 10.1.
Is there a good reason for it?

> +SET @@global.mysql56_temporal_format=true;
> +SET @@old_mode=zero_date_time_cast;
> +CREATE TABLE t1 (a TIME,b TIME(1));
> +INSERT INTO t1 VALUES (TIME'830:20:30',TIME'830:20:30');
> +SELECT TO_DAYS(a), TO_DAYS(b) FROM t1;
> +TO_DAYS(a)	TO_DAYS(b)
> +NULL	NULL
> +Warnings:
> +Warning	1264	Out of range value for column 'a' at row 1
> +Warning	1264	Out of range value for column 'b' at row 1
> +DROP TABLE t1;
> +SET @@global.mysql56_temporal_format=false;
> +SET @@old_mode=zero_date_time_cast;
> +CREATE TABLE t1 (a TIME,b TIME(1));
> +INSERT INTO t1 VALUES (TIME'830:20:30',TIME'830:20:30');
> +SELECT TO_DAYS(a), TO_DAYS(b) FROM t1;
> +TO_DAYS(a)	TO_DAYS(b)
> +NULL	NULL
> +Warnings:
> +Warning	1264	Out of range value for column 'a' at row 1
> +Warning	1264	Out of range value for column 'b' at row 1
> +DROP TABLE t1;
> +SET @@global.mysql56_temporal_format=DEFAULT;
> 
> === modified file 'mysql-test/suite/binlog/r/binlog_mysqlbinlog_row.result'
> --- mysql-test/suite/binlog/r/binlog_mysqlbinlog_row.result	2013-09-14 01:09:36 +0000
> +++ mysql-test/suite/binlog/r/binlog_mysqlbinlog_row.result	2014-08-29 06:20:22 +0000
> @@ -1625,7 +1625,7 @@ BEGIN
>  #010909  4:46:40 server id 1  end_log_pos # 	Write_rows: table id # flags: STMT_END_F
>  ### INSERT INTO `test`.`t1`
>  ### SET
> -###   @1=2001-02-03 10:20:30 /* DATETIME meta=0 nullable=1 is_null=0 */
> +###   @1='2001-02-03 10:20:30' /* DATETIME(0) meta=0 nullable=1 is_null=0 */

Why is that?
1. quoting
2. (0)

hmm, quoting is ok, I suppose. time values were (and are) quoted, timestamp values were (and are) not quoted,
so it's kind of ok to quote datetime values here

>  # at #
>  #010909  4:46:40 server id 1  end_log_pos # 	Query	thread_id=#	exec_time=#	error_code=0
>  SET TIMESTAMP=1000000000/*!*/;
> === modified file 'mysql-test/suite/innodb/r/data_types.result'
> --- mysql-test/suite/innodb/r/data_types.result	2013-11-13 21:58:19 +0000
> +++ mysql-test/suite/innodb/r/data_types.result	2014-08-29 05:46:28 +0000
> @@ -112,7 +112,7 @@ t1_BLOB	DATA_BLOB
>  t1_CHAR_100	DATA_CHAR	
>  t1_CHAR_100_BINARY	DATA_MYSQL	
>  t1_DATE	DATA_INT	
> -t1_DATETIME	DATA_INT	
> +t1_DATETIME	DATA_FIXBINARY	

Hmm. The comment in the test file says

   This test records what *internal type codes* innodb is using for every
   MariaDB data type. THEY MUST ALWAYS BE THE SAME AND NEVER CHANGE!
   Otherwise we create a compatibility problem and possible silent data
   corruption too, see MDEV-5248

I suspect in your case the change is ok, as you've also changed the server type code
for a field. But still please MDEV-5248, and test a scenario from there
(upgrade + online alter) to make sure you didn't break it. And the same scenario
for upgrade from mysql-5.5 and from mysql-5.6

>  t1_DATETIME_6	DATA_FIXBINARY	
>  t1_DECIMAL_10_3	DATA_FIXBINARY	
>  t1_DECIMAL_10_3_UNSIGNED	DATA_FIXBINARY	UNSIGNED
> === added file 'mysql-test/suite/rpl/t/rpl_temporal_format_mariadb53_to_mariadb53.test'
> --- mysql-test/suite/rpl/t/rpl_temporal_format_mariadb53_to_mariadb53.test	1970-01-01 00:00:00 +0000
> +++ mysql-test/suite/rpl/t/rpl_temporal_format_mariadb53_to_mariadb53.test	2014-08-29 10:10:47 +0000
> @@ -0,0 +1,4 @@
> +--let $force_master_mysql56_temporal_format=false;
> +--let $force_slave_mysql56_temporal_format=false;
> +
> +--source rpl_temporal_format_default_to_default.test

If you need to run one test with different settings, you can, of course, include it many times
in different files, but there's also an alternative approach - combinations. It's not always
applicable, but when it is, it's pretty convenient and concise.

> 
> === modified file 'sql/field.cc'
> --- sql/field.cc	2014-06-11 08:08:08 +0000
> +++ sql/field.cc	2014-08-29 10:45:32 +0000
> @@ -5042,7 +5042,8 @@ int Field_timestamp_with_dec::set_time()
>  {
>    THD *thd= get_thd();
>    set_notnull();
> -  store_TIME(thd->query_start(), thd->query_start_sec_part());
> +  // Avoid writing microseconds into binlog for FSP=0
> +  store_TIME(thd->query_start(), decimals() ? thd->query_start_sec_part() : 0);

I'd expect that when decimals() == 0, the object would be Field_timestamp, not
Field_timestamp_with_dec. How do you end up here with decimals() == 0?

>    return 0;
>  }
>  
> @@ -5565,6 +5573,8 @@ double Field_time_with_dec::val_real(voi
>  
>  bool Field_time_hires::get_date(MYSQL_TIME *ltime, ulonglong fuzzydate)
>  {
> +  if (check_zero_in_date_with_warn(fuzzydate))
> +    return true;

Ok, that's your bugfix, I suppose. It's small enough,
you can easily do it in 10.0

>    uint32 len= pack_length();
>    longlong packed= read_bigendian(ptr, len);
>  
> === modified file 'sql/item.cc'
> --- sql/item.cc	2014-08-07 16:06:56 +0000
> +++ sql/item.cc	2014-08-27 07:27:31 +0000
> @@ -1585,6 +1585,7 @@ Item_splocal::Item_splocal(const LEX_STR
>  {
>    maybe_null= TRUE;
>  
> +  sp_var_type= real_type_to_type(sp_var_type);

What's that for? Another bug fix? Do you have a test case for it?

>    m_type= sp_map_item_type(sp_var_type);
>    m_field_type= sp_var_type;
>    m_result_type= sp_map_result_type(sp_var_type);
> 
> === modified file 'sql/sys_vars.cc'
> --- sql/sys_vars.cc	2014-08-07 16:06:56 +0000
> +++ sql/sys_vars.cc	2014-08-28 14:16:31 +0000
> @@ -4815,3 +4815,8 @@ static Sys_var_mybool Sys_pseudo_slave_m
>         SESSION_ONLY(pseudo_slave_mode), NO_CMD_LINE, DEFAULT(FALSE),
>         NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(check_pseudo_slave_mode));
>  
> +static Sys_var_mybool Sys_mysql56_temporal_format(
> +       "mysql56_temporal_format",
> +       "Use MySQL-5.6 (instead of MariaDB-5.3) format for TIME, DATETIME, TIMESTAMP columns.",
> +       GLOBAL_VAR(opt_mysql56_temporal_format),

why is it global, not session?

> +       CMD_LINE(OPT_ARG), DEFAULT(TRUE), NO_MUTEX_GUARD, NOT_IN_BINLOG);
> 
> === modified file 'storage/tokudb/mysql-test/tokudb_alter_table/r/fractional_time_alter_table.result'
> --- storage/tokudb/mysql-test/tokudb_alter_table/r/fractional_time_alter_table.result	2014-03-26 08:33:54 +0000
> +++ storage/tokudb/mysql-test/tokudb_alter_table/r/fractional_time_alter_table.result	2014-08-29 06:14:31 +0000
> @@ -99,10 +99,10 @@ ERROR 42000: Table 'foo' uses an extensi
>  alter table foo change d d datetime(2);
>  ERROR 42000: Table 'foo' uses an extension that doesn't exist in this MariaDB version
>  alter table foo change d d datetime(5);
> +ERROR 42000: Table 'foo' uses an extension that doesn't exist in this MariaDB version
>  alter table foo change d d datetime(6);
>  ERROR 42000: Table 'foo' uses an extension that doesn't exist in this MariaDB version
>  alter table foo change g g datetime(5);
> -ERROR 42000: Table 'foo' uses an extension that doesn't exist in this MariaDB version

Why did that change?

>  drop table foo;
>  create table foo (
>  a time, 

Regards,
Sergei