[Maria-developers] Please review: MDEV-5528 Command line variable to choose MariaDB-5.3 vs MySQL-5.6 temporal data formats
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 Thanks.
Hi, Forgot to mention: I have some doubts about the changed order of records in mysql-test/r/distinct.result. Please suggest if it's fine. Thanks. On 08/29/2014 02:53 PM, 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
Thanks.
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
Hi Sergei, Thanks for review. My comments go inline: On 10/17/2014 07:29 PM, Sergei Golubchik wrote:
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.
Okey.
=== 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?
Yes. There's no reason to spend extra 3 bytes. Also, I believe MySQL will deprecate support for the old formats sooner or later. I'd like to deprecate at the same time in Maria.
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.
Okey, I will add --sorted_result.
=== 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?
There are no reasons. I will push it into 10.0.
+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
I added '(0)' when implemented the FSP data types MySQL, so one can distinguish between the old format and the new format when reading the mysqlbinlog output. If you don't like this, I can suppress printing the precision in case of FSP=0.
# 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
Can you suggest how to test this? I have never done this kind of tests with InnoDB yet. I guess I need create InnoDB tables in MariaDB-10.0, MySQL-5.5 and MySQL-5.6, put them into mysql-test/std_data/. In the new test I'll copy these files into the data directory and try to do online alter on them. What I don't understand: should I use innodb_file_per_table=YES and put individual table files into std_data, or should I use innodb_file_per_table=NO and put the entire ibdata file into std_data? Perhaps we need to cover both. But the minimum ibdata size is quite huge, 10Mb. Also, what is the proper way to replace the InnoDB files in a working server? Do I need to do "FLUSH TABLES" before "cp", or some other command? By the way, there should not be any problems with MySQL-5.6, because it has, or course, exactly the same patch about the InnoDB data type mapping.
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.
Thanks for the tip. Would combinations work for my purposes? Do you want me to switch the tests to use combinations?
=== 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?
The actual object that is created is Field_timestampf, which is derived from Field_timestamp_with_dec. It is implemented this way in MySQL, and the idea is to deprecate the old type codes asap. Why have two type codes?
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
Okey, will push it into 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?
Are you sure we want this as a session variable? From my understanding, it has a very low impact on the end user, because there are no visible behaviour change other than the column sizes in "EXPLAIN SELECT" output. So the end users does not really care. This can be important for DBAs only, when they want to upgrade with a minimum risk.
+ 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?
This is only for DATETIME, and only for FSP=5. Tokudb can do ALTER only if binary size of the old and the new column are the same. In MariaDB-5.3, binary size of datetime(5) is equal to binary size of datetime(4). MySQL-5.6, binary size of datetime(5) is equal to binary size of datetime(6).
drop table foo; create table foo ( a time,
Regards, Sergei
Hi, Alexander! On Oct 20, Alexander Barkov wrote:
=== 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
I added '(0)' when implemented the FSP data types MySQL, so one can distinguish between the old format and the new format when reading the mysqlbinlog output.
If you don't like this, I can suppress printing the precision in case of FSP=0.
Let's keep your '(0)' - it seems useful. It users will complain, we can reconsider it.
=== 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
Can you suggest how to test this? I have never done this kind of tests with InnoDB yet.
I mean to test it manually. Normally the data_types.test ensures that type codes don't change, so there's no need to put the whole upgrade test into mysql-test.
=== 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.
Thanks for the tip. Would combinations work for my purposes? Do you want me to switch the tests to use combinations?
No, wait... Sorry. I'm afraid, currently combinations don't support different settings for master and slave. So, they wouldn't help here. I didn't think of that use case before...
=== 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?
The actual object that is created is Field_timestampf, which is derived from Field_timestamp_with_dec.
It is implemented this way in MySQL, and the idea is to deprecate the old type codes asap. Why have two type codes?
That's not about type codes. We used to have Field_timestamp that handles TIMESTAMP(0) and Field_timestamp_hires that handles TIMESTAMP(N) for N>0. And the old class Field_timestamp was optimized for old timestamps without microseconds. Now you seem to create a generic Field_timestampf for all TIMESTAMP fields.
=== 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?
You seem to have missed this question.
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?
Are you sure we want this as a session variable?
From my understanding, it has a very low impact on the end user, because there are no visible behaviour change other than the column sizes in "EXPLAIN SELECT" output. So the end users does not really care.
Hmm. Generally I prefer to have session variables for anything that can be session local behavior. Only variables that have global effect, like change global buffers, are created as global-only variables. But that's a guideline, not a strict rule. If you think this variable makes no sense as a session variables, okay, make it global. Regards, Sergei
Hi Sergei, This is the update about MDEV-5528. On 10/20/2014 12:54 PM, Sergei Golubchik wrote: <skip>
=== 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
Can you suggest how to test this? I have never done this kind of tests with InnoDB yet.
I mean to test it manually. Normally the data_types.test ensures that type codes don't change, so there's no need to put the whole upgrade test into mysql-test.
I tested upgrade from MariaDB-10.0 to MariaDB with the 5528 patch applied. All online alters worked fine. I tried these ALTER commands: ALTER ONLINE TABLE t1_online ADD a INT NOT NULL DEFAULT 0; ALTER TABLE t1_default ALGORITHM=DEFAULT, ADD a INT NOT NULL DEFAULT 0; ALTER TABLE t1_inplace ALGORITHM=INPLACE, ADD a INT NOT NULL DEFAULT 0; ALTER TABLE t1_copy ALGORITHM=COPY, ADD a INT NOT NULL DEFAULT 0; After doing the above ALTERs, I checked "EXPLAIN SELECT" output. All ALTERs preserved the old column formats, according to "Key_len" value. Everything looks correct. Also, while experimenting, I noticed that InnoDB complains to stderr about unexpected format for innodb_table_stats.last_update. I had to add these lines into scripts/mysql_system_tables_fix.sql to fix the problem:
+set @str="alter table mysql.innodb_table_stats modify last_update timestamp not null default current_timestamp on update current_timestamp"; +set @str=if(@have_innodb <> 0, @str, "set @dummy = 0"); +prepare stmt from @str; +execute stmt; +
Does it look fine? <skip>
=== 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?
The actual object that is created is Field_timestampf, which is derived from Field_timestamp_with_dec.
It is implemented this way in MySQL, and the idea is to deprecate the old type codes asap. Why have two type codes?
That's not about type codes. We used to have Field_timestamp that handles TIMESTAMP(0) and Field_timestamp_hires that handles TIMESTAMP(N) for N>0. And the old class Field_timestamp was optimized for old timestamps without microseconds.
Now you seem to create a generic Field_timestampf for all TIMESTAMP fields.
Well, that's both about type codes and an optimized field. There are no optimized fields for FSP=0 for the new formats MYSQL_TYPE_{TIME2|DATETIME2|TIMESTAMP2}. MySQL-5.6 also does not have optimized fields for FSP=0. But there is a reason for that: MySQL rounds, not truncates. In MariaDB we can add optimized versions. Would you like me to add them in this patch? Maybe I could do it in a separate patch later...
=== 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?
You seem to have missed this question.
Sorry, I missed this. This is to avoid crash in Item::tmp_table_field_from_field_type(). It expects type(), it does not expect real_type(). When a SP variable type is parsed in sql_yacc.yy, it returns real_type, i.e. MYSQL_TYPE_TIME2/DATETIME2/TIMESTAMP2. This is needed to create proper fields when doing "CREATE TABLE t1 (a TIME(6))". But as a side effect, we have real_type in Item_splocal::Item::splocal. This line changes real_type to type. Another option would be to do this outside of the constructor, in the caller code. But, it should not be harmful inside the constructor. Btw, I wish we had separate enums for type() and real_type(). :) <skip>
Hi, Alexander! On Oct 22, Alexander Barkov wrote:
Also, while experimenting, I noticed that InnoDB complains to stderr about unexpected format for innodb_table_stats.last_update.
I had to add these lines into scripts/mysql_system_tables_fix.sql to fix the problem:
+set @str="alter table mysql.innodb_table_stats modify last_update timestamp not null default current_timestamp on update current_timestamp"; +set @str=if(@have_innodb <> 0, @str, "set @dummy = 0"); +prepare stmt from @str; +execute stmt;
Does it look fine?
Yes.
=== 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?
The actual object that is created is Field_timestampf, which is derived from Field_timestamp_with_dec.
It is implemented this way in MySQL, and the idea is to deprecate the old type codes asap. Why have two type codes?
That's not about type codes. We used to have Field_timestamp that handles TIMESTAMP(0) and Field_timestamp_hires that handles TIMESTAMP(N) for N>0. And the old class Field_timestamp was optimized for old timestamps without microseconds.
Now you seem to create a generic Field_timestampf for all TIMESTAMP fields.
Well, that's both about type codes and an optimized field.
There are no optimized fields for FSP=0 for the new formats MYSQL_TYPE_{TIME2|DATETIME2|TIMESTAMP2}.
Okay, I see.
=== 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?
You seem to have missed this question.
Sorry, I missed this.
This is to avoid crash in Item::tmp_table_field_from_field_type(). It expects type(), it does not expect real_type().
When a SP variable type is parsed in sql_yacc.yy, it returns real_type, i.e. MYSQL_TYPE_TIME2/DATETIME2/TIMESTAMP2. This is needed to create proper fields when doing "CREATE TABLE t1 (a TIME(6))".
But as a side effect, we have real_type in Item_splocal::Item::splocal. This line changes real_type to type.
Is there a test that crashes without this line?
Btw, I wish we had separate enums for type() and real_type(). :)
Agree. May be in a followup patch for your IPv6 patch. Regards, Sergei
Hi Sergei, On 10/23/2014 02:48 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Oct 22, Alexander Barkov wrote:
Also, while experimenting, I noticed that InnoDB complains to stderr about unexpected format for innodb_table_stats.last_update.
I had to add these lines into scripts/mysql_system_tables_fix.sql to fix the problem:
+set @str="alter table mysql.innodb_table_stats modify last_update timestamp not null default current_timestamp on update current_timestamp"; +set @str=if(@have_innodb <> 0, @str, "set @dummy = 0"); +prepare stmt from @str; +execute stmt;
Does it look fine?
Yes.
=== 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?
The actual object that is created is Field_timestampf, which is derived from Field_timestamp_with_dec.
It is implemented this way in MySQL, and the idea is to deprecate the old type codes asap. Why have two type codes?
That's not about type codes. We used to have Field_timestamp that handles TIMESTAMP(0) and Field_timestamp_hires that handles TIMESTAMP(N) for N>0. And the old class Field_timestamp was optimized for old timestamps without microseconds.
Now you seem to create a generic Field_timestampf for all TIMESTAMP fields.
Well, that's both about type codes and an optimized field.
There are no optimized fields for FSP=0 for the new formats MYSQL_TYPE_{TIME2|DATETIME2|TIMESTAMP2}.
Okay, I see.
=== 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?
You seem to have missed this question.
Sorry, I missed this.
This is to avoid crash in Item::tmp_table_field_from_field_type(). It expects type(), it does not expect real_type().
When a SP variable type is parsed in sql_yacc.yy, it returns real_type, i.e. MYSQL_TYPE_TIME2/DATETIME2/TIMESTAMP2. This is needed to create proper fields when doing "CREATE TABLE t1 (a TIME(6))".
But as a side effect, we have real_type in Item_splocal::Item::splocal. This line changes real_type to type.
Is there a test that crashes without this line?
Yes, some sp.*.test fail without this line.
Btw, I wish we had separate enums for type() and real_type(). :)
Agree. May be in a followup patch for your IPv6 patch.
Regards, Sergei
participants (2)
-
Alexander Barkov -
Sergei Golubchik