maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Varun,

On Mon, Nov 12, 2018 at 02:41:51PM +0530, Varun wrote:
> revision-id: 3f4dab80912682325b61b288178febe9ee55de49 (mariadb-10.0.36-78-g3f4dab80912)
> parent(s): 6cecb10a2f8b6536bed78ab6d3791d8befc9d732
> author: Varun Gupta
> committer: Varun Gupta
> timestamp: 2018-11-12 14:40:07 +0530
> message:
> 
> MDEV-17032: Estimates are higher for partitions of a table with @@use_stat_tables= PREFERABLY
> 
> The problem here is EITS statistics does not calculate statistics for the partitions of the table.
> So a temporary solution would be to not read EITS statistics for partitioned tables.
> 
> Also disabling reading of EITS for columns that participate in the partition list of a table.
> 
> ---
>  mysql-test/r/partition.result | 37 +++++++++++++++++++++++++++++++++++++
>  mysql-test/t/partition.test   | 35 +++++++++++++++++++++++++++++++++++
>  sql/opt_range.cc              | 12 ++++++++++--
>  sql/partition_info.cc         | 21 +++++++++++++++++++++
>  sql/partition_info.h          |  1 +
>  sql/sql_statistics.cc         |  6 ++++++
>  6 files changed, 110 insertions(+), 2 deletions(-)
> 
> diff --git a/mysql-test/r/partition.result b/mysql-test/r/partition.result
> index c6669176b3d..abfbc21dccb 100644
> --- a/mysql-test/r/partition.result
> +++ b/mysql-test/r/partition.result
> @@ -2645,3 +2645,40 @@ Warnings:
>  Note	1517	Duplicate partition name p2
>  DEALLOCATE PREPARE stmt;
>  DROP TABLE t1;
> +#
> +# MDEV-17032: Estimates are higher for partitions of a table with @@use_stat_tables= PREFERABLY
> +#
> +create table ten(a int);
> +insert into ten values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9);
> +create table one_k(a int);
> +insert into one_k select A.a + B.a* 10 + C.a * 100 from ten A, ten B, ten C;

Please rename the tables to follow the t0,t1,t2... convention.
> +create table t1 (
> +part_key int,
> +a int,
> +b int
> +) partition by list(part_key) (
> +partition p0 values in (0),
> +partition p1 values in (1),
> +partition p2 values in (2),
> +partition p3 values in (3),
> +partition p4 values in (4)
> +);
> +insert into t1
> +select mod(a,5), a/100, a from one_k;
> +set @save_use_stat_tables= @@use_stat_tables;
> +set @save_optimizer_use_condition_selectivity=@@optimizer_use_condition_selectivity;
> +set @@use_stat_tables= PREFERABLY;
> +set @@optimizer_use_condition_selectivity=4;
> +analyze table t1;
> +Table	Op	Msg_type	Msg_text
> +test.t1	analyze	status	Engine-independent statistics collected
> +test.t1	analyze	status	OK

After some time passes, it gets hard to understand what exactly is being
checked. Then, there's some change in the EXPLAIN (typically for an unrelated
reason) and one is left wondering, whether the new EXPLAIN output is ok or not.

* please use EXPLAIN PARTITIONS.
* please add something like "--echo # rows should be X, not Y" before the
query.

> +explain select * from t1 where  part_key in (1,2);
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	SIMPLE	t1	ALL	NULL	NULL	NULL	NULL	400	Using where
> +explain select * from t1 where part_key > 1;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	SIMPLE	t1	ALL	NULL	NULL	NULL	NULL	600	Using where

This does not provide sufficient coverage. I get this result if I just apply
the change in sql_statistics.cc.

mysql> explain select * from t1 where  part_key in (1,2);
+------+-------------+-------+------+---------------+------+---------+------+------+-------------+
| id   | select_type | table | type | possible_keys | key  | key_len | ref  | rows | Extra       |
+------+-------------+-------+------+---------------+------+---------+------+------+-------------+
|    1 | SIMPLE      | t1    | ALL  | NULL          | NULL | NULL    | NULL |  400 | Using where |
+------+-------------+-------+------+---------------+------+---------+------+------+-------------+
1 row in set (0.01 sec)

mysql> explain select * from t1 where part_key > 1;
+------+-------------+-------+------+---------------+------+---------+------+------+-------------+
| id   | select_type | table | type | possible_keys | key  | key_len | ref  | rows | Extra       |
+------+-------------+-------+------+---------------+------+---------+------+------+-------------+
|    1 | SIMPLE      | t1    | ALL  | NULL          | NULL | NULL    | NULL |  600 | Using where |
+------+-------------+-------+------+---------------+------+---------+------+------+-------------+
1 row in set (0.00 sec)

Please add testcases for other changes.

> +set @@optimizer_use_condition_selectivity=@save_optimizer_use_condition_selectivity;
> +set @@use_stat_tables= @save_use_stat_tables;
> +drop table t1, one_k, ten;
> diff --git a/sql/opt_range.cc b/sql/opt_range.cc
> index 3bcaa72e32f..993b7d57e0a 100644
> --- a/sql/opt_range.cc
> +++ b/sql/opt_range.cc
> @@ -3322,6 +3322,10 @@ bool create_key_parts_for_pseudo_indexes(RANGE_OPT_PARAM *param,
>  {
>    Field **field_ptr;
>    TABLE *table= param->table;
> +  partition_info *part_info= NULL;
> +  #ifdef WITH_PARTITION_STORAGE_ENGINE
> +    part_info= table->part_info;
> +  #endif
>    uint parts= 0;
>  
>    for (field_ptr= table->field; *field_ptr; field_ptr++)
> @@ -3329,7 +3333,9 @@ bool create_key_parts_for_pseudo_indexes(RANGE_OPT_PARAM *param,
>      Column_statistics* col_stats= (*field_ptr)->read_stats;
>      if (bitmap_is_set(used_fields, (*field_ptr)->field_index)
>         && col_stats && !col_stats->no_stat_values_provided()
> -       && !((*field_ptr)->type() == MYSQL_TYPE_GEOMETRY))
> +       && !((*field_ptr)->type() == MYSQL_TYPE_GEOMETRY)
> +       && (!part_info ||
> +        part_info->disable_eits_for_partitioning_columns(*field_ptr)))
>        parts++;
>    }
>  
> @@ -3350,7 +3356,9 @@ bool create_key_parts_for_pseudo_indexes(RANGE_OPT_PARAM *param,
>      if (bitmap_is_set(used_fields, (*field_ptr)->field_index))
>      {
>        Field *field= *field_ptr;
> -      if (field->type() == MYSQL_TYPE_GEOMETRY)
> +      if (field->type() == MYSQL_TYPE_GEOMETRY
> +         && (!part_info ||
> +            part_info->disable_eits_for_partitioning_columns(*field_ptr)))
>          continue;
Why use 'field_ptr' if you have 'field' ? 

The above condition seems to be wrong. 
Suppose that the following holds:
  field->type()==MYSQL_TYPE_GEOMETRY,
  part_info!=NULL
  part_info->disable_eits_for_partitioning_columns(field) == FALSE // makes
  // sense as one typically does not do partitioning on geometry columns.

if-condition evaluates to:

  (TRUE && (FALSE || FALSE)) -> (TRUE && FALSE) -> FALSE.

So we will not execute 'continue;' for a geometry column. This seems incorrect.

>  
>        uint16 store_length;
> diff --git a/sql/partition_info.cc b/sql/partition_info.cc
> index 52bda560c1c..91d74b28d0e 100644
> --- a/sql/partition_info.cc
> +++ b/sql/partition_info.cc
> @@ -3164,6 +3164,27 @@ void partition_info::print_debug(const char *str, uint *value)
>      DBUG_PRINT("info", ("parser: %s", str));
>    DBUG_VOID_RETURN;
>  }
> +
> +/*
> +
> + Disabling reading EITS statistics for columns involved in the
> + partition list of a table.
> + We assume the selecticivity for such columns would be handled
> + during partition pruning.
> +
> +*/
> +
> +bool partition_info::disable_eits_for_partitioning_columns(Field *field)

The function name is misleading. If a function is named disable_XYZ() I would
have expected the code inside to actually take the steps to prevent XYZ from
happening.

I think it would be more straightforward if it was called 

bool partition_info::field_is_used_in_partition_expr() const

or something like that.
Please also declare the function as 'const' and maybe its parameter too.

(As discussed on Slack, the function should also search for the field in the
list of sub-partitioning columns)

> +{
> +  uint i;
> +  for (i= 0; i < num_part_fields; i++)
> +  {
> +    if (field->eq(part_field_array[i]))
> +      return FALSE;
> +  }
> +  return TRUE;
> +}
> +
>  #else /* WITH_PARTITION_STORAGE_ENGINE */
>   /*
>     For builds without partitioning we need to define these functions
> diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc
> index cb75a5c2176..33a12588434 100644
> --- a/sql/sql_statistics.cc
> +++ b/sql/sql_statistics.cc
> @@ -3589,6 +3589,12 @@ void set_statistics_for_table(THD *thd, TABLE *table)
>      (use_stat_table_mode <= COMPLEMENTARY ||
>       !table->stats_is_read || read_stats->cardinality_is_null) ?
>      table->file->stats.records : read_stats->cardinality;
> +
> +  #ifdef WITH_PARTITION_STORAGE_ENGINE
> +    if (table->part_info)
> +      table->used_stat_records= table->file->stats.records;
> +  #endif

This looks cryptic. 
Please add a comment, something like:

/*
  For partitioned table, EITS statistics is based on data from all partitions.
  
  On the other hand, Partition Pruning figures which partitions will be
  accessed and then computes the estimate of rows in used_partitions.

  Use the estimate from Partition Pruning as it is typically more precise.
  Ideally, EITS should provide per-partition statistics but this is not
  implemented currently.

*/
>    KEY *key_info, *key_info_end;
>    for (key_info= table->key_info, key_info_end= key_info+table->s->keys;
>         key_info < key_info_end; key_info++)

BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog