maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita!

On Nov 12, Nikita Malyavin wrote:
> revision-id: bc7d600a27f4c6c14deea5f1d674c621f85b23ba (versioning-1.0.5-141-gbc7d600a27f)
> parent(s): 90b292ce31170be33796b07723ffcef56bf8a679
> author: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
> committer: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
> timestamp: 2018-09-21 22:42:03 +1000
> message:
> 
> MDEV-15951 system versioning by trx id doesn't work with partitioning
> 
> Fix partitioning for trx_id-versioned tables.
> `partition by hash`, `range` and others now work.
> `partition by system_time` is forbidden.
> Currently we cannot use row_start and row_end in `partition by`, because
> insertion of versioned field is done by engine's handler, as well as
> row_start/row_end's value set up, which is a transaction id -- so it's
> also forbidden.
> 
> The drawback is that it's now impossible to use `partition by key()`
> without parameters for such tables, because it references row_start and
> row_end implicitly.
> 
> * add handler::vers_can_native()
> * drop Table_scope_and_contents_source_st::vers_native()
> * drop partition_element::find_engine_flag as unused
> * forbid versioning partitioning for trx_id as not supported
> * adopt vers tests for trx_id partitioning
> * forbid any row_end referencing in `partition by` clauses,
>   including implicit `by key()`
> 
> ---
>  mysql-test/suite/versioning/r/partition.result     | 79 ++++++++++++++++--
>  .../suite/versioning/t/partition.combinations      |  3 +
>  mysql-test/suite/versioning/t/partition.test       | 97 ++++++++++++++++++++--
>  sql/ha_partition.h                                 | 10 +++
>  sql/handler.cc                                     | 34 ++------
>  sql/handler.h                                      |  8 +-
>  sql/partition_element.h                            | 15 ----
>  sql/sql_class.h                                    |  4 +-
>  sql/sql_partition.cc                               |  9 ++
>  sql/sql_table.cc                                   |  6 +-
>  sql/table.cc                                       |  4 +-
>  sql/temporary_tables.cc                            | 10 ++-
>  12 files changed, 214 insertions(+), 65 deletions(-)
> 
> diff --git a/mysql-test/suite/versioning/r/partition.result b/mysql-test/suite/versioning/r/partition.result
> index bfec0ce2d4b..cf4203b7949 100644
> --- a/mysql-test/suite/versioning/r/partition.result
> +++ b/mysql-test/suite/versioning/r/partition.result
> @@ -1,6 +1,6 @@
>  set system_versioning_alter_history=keep;
>  # Check conventional partitioning on temporal tables
> -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

>  with system versioning
>  partition by range columns (x) (
>  partition p0 values less than (100),
> @@ -501,6 +504,72 @@ set timestamp=1523466002.799571;
>  insert into t1 values (11),(12);
>  set timestamp=1523466004.169435;
>  delete from t1 where pk in (11, 12);
> +# MDEV-15951 system versioning by trx id doesn't work with partitioning
> +# currently trx_id does not support partitioning by system_time
> +create or replace table t1(
> +i int,
> +row_start bigint unsigned generated always as row start,
> +row_end bigint unsigned generated always as row end,
> +period for system_time(row_start, row_end)
> +) engine=InnoDB with system versioning partition by system_time (

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.

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

> diff --git a/sql/ha_partition.h b/sql/ha_partition.h
> index 8a251016703..ef49011b7f8 100644
> --- a/sql/ha_partition.h
> +++ b/sql/ha_partition.h
> @@ -413,6 +413,16 @@ class ha_partition :public handler
>  
>    virtual void return_record_by_parent();
>  
> +  virtual bool vers_can_native(THD *thd)
> +  {
> +    // PARTITION BY SYSTEM_TIME is not supported for now
> +    bool can= !thd->lex->part_info
> +              || thd->lex->part_info->part_type != VERSIONING_PARTITION;
> +    for (uint i= 0; i < m_tot_parts && can; i++)
> +      can= can && m_file[i]->vers_can_native(thd);
> +    return can;
> +  }
> +
>    /*
>      -------------------------------------------------------------------------
>      MODULE create/delete handler object
> diff --git a/sql/handler.cc b/sql/handler.cc
> index 306b0868d15..a7962039463 100644
> --- a/sql/handler.cc
> +++ b/sql/handler.cc
> @@ -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?

>  
>      // 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) ?

>  bool Table_scope_and_contents_source_st::vers_fix_system_fields(
>    THD *thd, Alter_info *alter_info, const TABLE_LIST &create_table,
>    bool create_select)

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx