---------- Forwarded message --------- From: Nikita Malyavin <nikitamalya...@gmail.com> Date: пт, 21 дек. 2018 г. в 04:52 Subject: Re: bc7d600a27f: MDEV-15951 system versioning by trx id doesn't work with partitioning To: Sergei Golubchik <s...@mariadb.org>
вт, 13 нояб. 2018 г. в 04:43, Sergei Golubchik <s...@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
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp