---------- Forwarded message --------- From: Nikita Malyavin <nikitamalyavin@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 <serg@mariadb.org> вт, 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