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>