maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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>