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.

> > + 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.

> 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?

Regards,

Jeremy