вт, 13 нояб. 2018 г. в 04:43, Sergei Golubchik <serg@mariadb.org>:
>
> Hi, Nikita!
>
Hi, Sergei!

I've updated the tests with respect to issues You mentioned. For the
rest i'll give my answers.


> > -create table t1 (x int)
> > +create or replace table t1 (x int, ROW_START, ROW_END, period for system_time(row_start, row_end))
>
> ouch. that looks very weird. please, only replace the type, not the
> whole column definition. Like everywhere else:
>
> --replace_result $sys_datatype_expl SYS_DATATYPE
>
Fixed it, thanks. Did not notice the weirdness in test results reading.


> if you have tests with innodb and trx_id, please, put them in a separate
> test file, say, partition_innodb.test. partition.test is for common
> tests that are repeated three times, with myisam, innodb, timestamps,
> and trx_id. There is no need to repeat your test three times.

Did it, though i have an opinion that it makes tests run slower at overall.
Every test case adds quite a noticeable lag, more than several test(s) runs.

> > diff --git a/mysql-test/suite/versioning/t/partition.combinations b/mysql-test/suite/versioning/t/partition.combinations
> > index 4d73ef5a5ea..561c5656929 100644
> > --- a/mysql-test/suite/versioning/t/partition.combinations
> > +++ b/mysql-test/suite/versioning/t/partition.combinations
> > @@ -1,5 +1,8 @@
> >  [timestamp]
> >  default-storage-engine=innodb
> >
> > +[trx_id]
> > +default-storage-engine=innodb
> > +
> >  [myisam]
> >  default-storage-engine=myisam
>
> But then you don't need partition.combinations at all, just add
>
>    --source engines.inc
>
> like other tests do.
>

-- Fixed

> > @@ -4938,6 +4938,7 @@ int ha_create_table(THD *thd, const char *path,
> >                          !create_info->tmp_table();
> >
> >      share.frm_image= frm;
> > +    share.orig_table_name= table_name;
>
> why? if orig_table_name is NULL, error_table_name() will use table_name,
> so there's never need to make orig_table_name==table_name.
>
> and I don't understand all your other manipulations with
> orig_table_name. Could you please explain that?
>

It affects ALTER TABLE error messages. table_name will contain temporary name,
 and error_table_name() will output "(temporary)", which is rather inconvenient.
AFAICS, orig_table_name purpose is exactly for fixing that, but some
corner cases
 were not covered to this moment.

> >
> >      // open an frm image
> >      if (share.init_from_binary_frm_image(thd, write_frm_now,
> > @@ -6952,29 +6953,6 @@ bool Vers_parse_info::fix_implicit(THD *thd, Alter_info *alter_info)
> >    return false;
> >  }
> >
> > -bool Table_scope_and_contents_source_st::vers_native(THD *thd) const
> > -{
> > -  if (ha_check_storage_engine_flag(db_type, HTON_NATIVE_SYS_VERSIONING))
> > -    return true;
> > -
> > -#ifdef WITH_PARTITION_STORAGE_ENGINE
> > -  partition_info *info= thd->work_part_info;
> > -  if (info && !(used_fields & HA_CREATE_USED_ENGINE))
> > -  {
> > -    if (handlerton *hton= info->default_engine_type)
> > -      return ha_check_storage_engine_flag(hton, HTON_NATIVE_SYS_VERSIONING);
> > -
> > -    List_iterator_fast<partition_element> it(info->partitions);
> > -    while (partition_element *partition_element= it++)
> > -    {
> > -      if (partition_element->find_engine_flag(HTON_NATIVE_SYS_VERSIONING))
> > -        return true;
> > -    }
> > -  }
> > -#endif
> > -  return false;
> > -}
>
> what was wrong with that (and all other changed code in this file) ?
>

Mostly I'm ok with handler.cc code 🙂 except it is mixed with various
classes implementations.

First, find_engine_flag is not the correct function here, as it says
"if there's _any_ node in a tree
 with corresponding flag". We should check _all_ nodes instead of _any_.
Second, we need to make this check at
TABLE_SHARE::init_from_binary_frm_image  to
 set TABLE_SHARE::versioned properly and handle errors in place.
That's why I've introduced
handler::vers_can native [a nice thing about it is that wrote it first
and then found same-named
 share's field used exactly where it's needed, just with lack of tree
traversal].
But it makes Table_scope_and_contents_source_st::vers_native
redundant, as well as all related
 code in handler.cc.
Other important thing is that TABLE_SHARE::vers_can_native value is
now also correct with respect
 to partition tree, but i have not it used anywhere.


I have updated my pull request at github. I don't know how it keeps in
sync with mailing lists,
 so here's a link: https://github.com/MariaDB/server/pull/870. Also
rebased to latest 10.3 for convenience

Regards!

Nikita Malyavin
Tempesta Technologies
https://github.com/tempesta-tech