Hi, Jeremy! On Nov 11, Jeremy Cole wrote:
Hi Sergei,
- It is almost certainly worth creating test cases with data files from older MariaDB and newer MySQL versions to exercise the usage of the code paths that only occur during upgrade; that doesn't need to happen here necessarily. Yes, Elena is doing that. But I don't think mtr is suited for this kind of tests. I think mtr is not great for it, but it's also not awful. It just requires pre-generating the data files.
Yes, I know, it's possible to make it work.
+ case MYSQL_TYPE_NEWDATE: + return(DATA_INT); + case MYSQL_TYPE_TIMESTAMP: + *unsigned_flag = 0; + /* fall through */
That's the first change to get_innobase_type_from_mysql_type() as copied from 5.6. Because in 5.6 TIMESTAMP is signed, in MariaDB - unsigned.
I don't think that's correct. In MySQL 5.6, the Field_timestamp constructor sets "flags|= ZEROFILL_FLAG | UNSIGNED_FLAG;", which will end up with this being marked unsigned due to the generic handling of "if (field->flags & UNSIGNED_FLAG)" at the very top, as long as nothing else *unsets* it. So this should not be changed relative to MySQL 5.6.
5.6 never creates Field_timestamp fields, only Field_timestampf, that are signed. And note my change dict0stats.cc that removes DATA_UNSIGNED from last_update column in innodb stats tables. 5.6 doesn't have DATA_UNSIGNED there, so timestamp column in 5.6 is signed. In 5.5 it is unsigned though, I'll take this into account, thanks.
You also need to support MYSQL_TYPE_{TIMESTAMP,TIME,DATETIME}2 for MySQL 5.6 here.
I do. That's the second difference from get_innobase_type_from_mysql_type(). In 5.6 they do a switch on field->real_type(). But in MariaDB, temporal types with microsecond have the same real_type as without, one need to look at the key_type(). Incidentally, this also covers {TIMESTAMP,TIME,DATETIME}2 from 5.6, they also have key_type() == HA_KEYTYPE_BINARY. So, this if() covers everything.
Huh. Sorry, I didn't notice the change to the value being switch'ed on. Why change this? This is a pretty dangerous (IMHO) deviation from MySQL which may cause problems, and is unnecessary. Why not just stick with exactly what MySQL is doing in 5.6, plus whatever minor changes are needed to support MariaDB-format microsecond time?
Because changes to support MariaDB format make switch into a dead code. Or into redundant code, depending on whether I put if() before the switch or after. Because this if() has to be there, and it does cover all temporal type variants, MariaDB or MySQL. I rather add a test case to verify that 5.6-style temporal2 types are mapped correctly. Regards, Sergei