maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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