maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Oct 20, Alexander Barkov wrote:
> >> === modified file 'mysql-test/suite/binlog/r/binlog_mysqlbinlog_row.result'
> >> --- mysql-test/suite/binlog/r/binlog_mysqlbinlog_row.result	2013-09-14 01:09:36 +0000
> >> +++ mysql-test/suite/binlog/r/binlog_mysqlbinlog_row.result	2014-08-29 06:20:22 +0000
> >> @@ -1625,7 +1625,7 @@ BEGIN
> >>   #010909  4:46:40 server id 1  end_log_pos # 	Write_rows: table id # flags: STMT_END_F
> >>   ### INSERT INTO `test`.`t1`
> >>   ### SET
> >> -###   @1=2001-02-03 10:20:30 /* DATETIME meta=0 nullable=1 is_null=0 */
> >> +###   @1='2001-02-03 10:20:30' /* DATETIME(0) meta=0 nullable=1 is_null=0 */
> >
> > Why is that?
> > 1. quoting
> > 2. (0)
> >
> > hmm, quoting is ok, I suppose. time values were (and are) quoted, timestamp values were (and are) not quoted,
> > so it's kind of ok to quote datetime values here
> 
> I added '(0)' when implemented the FSP data types MySQL,
> so one can distinguish between the old format and the new format
> when reading the mysqlbinlog output.
> 
> If you don't like this, I can suppress printing the precision
> in case of FSP=0.

Let's keep your '(0)' - it seems useful.
It users will complain, we can reconsider it.

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

> >> === added file 'mysql-test/suite/rpl/t/rpl_temporal_format_mariadb53_to_mariadb53.test'
> >> --- mysql-test/suite/rpl/t/rpl_temporal_format_mariadb53_to_mariadb53.test	1970-01-01 00:00:00 +0000
> >> +++ mysql-test/suite/rpl/t/rpl_temporal_format_mariadb53_to_mariadb53.test	2014-08-29 10:10:47 +0000
> >> @@ -0,0 +1,4 @@
> >> +--let $force_master_mysql56_temporal_format=false;
> >> +--let $force_slave_mysql56_temporal_format=false;
> >> +
> >> +--source rpl_temporal_format_default_to_default.test
> >
> > If you need to run one test with different settings, you can, of course, include it many times
> > in different files, but there's also an alternative approach - combinations. It's not always
> > applicable, but when it is, it's pretty convenient and concise.
> 
> Thanks for the tip. Would combinations work for my purposes?
> Do you want me to switch the tests to use combinations?

No, wait... Sorry. I'm afraid, currently combinations don't support
different settings for master and slave. So, they wouldn't help here.
I didn't think of that use case before...

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

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

> >>     m_type= sp_map_item_type(sp_var_type);
> >>     m_field_type= sp_var_type;
> >>     m_result_type= sp_map_result_type(sp_var_type);
> >>
> >> === modified file 'sql/sys_vars.cc'
> >> --- sql/sys_vars.cc	2014-08-07 16:06:56 +0000
> >> +++ sql/sys_vars.cc	2014-08-28 14:16:31 +0000
> >> @@ -4815,3 +4815,8 @@ static Sys_var_mybool Sys_pseudo_slave_m
> >>          SESSION_ONLY(pseudo_slave_mode), NO_CMD_LINE, DEFAULT(FALSE),
> >>          NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(check_pseudo_slave_mode));
> >>
> >> +static Sys_var_mybool Sys_mysql56_temporal_format(
> >> +       "mysql56_temporal_format",
> >> +       "Use MySQL-5.6 (instead of MariaDB-5.3) format for TIME, DATETIME, TIMESTAMP columns.",
> >> +       GLOBAL_VAR(opt_mysql56_temporal_format),
> >
> > why is it global, not session?
> 
> Are you sure we want this as a session variable?
> 
> From my understanding, it has a very low impact on the end user,
> because there are no visible behaviour change other than the column
> sizes in "EXPLAIN SELECT" output. So the end users does not really care.

Hmm. Generally I prefer to have session variables for anything that can
be session local behavior. Only variables that have global effect, like
change global buffers, are created as global-only variables.

But that's a guideline, not a strict rule. If you think this variable
makes no sense as a session variables, okay, make it global.

Regards,
Sergei