---------- Forwarded message ---------
From: Nikita Malyavin
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
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