← Back to team overview

maria-developers team mailing list archive

Re: [Commits] a1929d001c7: MDEV-18741: Optimizer trace: multi-part key ranges are printed incorrectly

 

Hi Varun,

In general, the patch moves in the right direction, but it is not yet there. 

Consider this example:

create table ten(a int);
insert into ten values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9);
create table t1 (
  a int not null,
  b int not null,
  c int not null,
  d int not null,
  key(a,b,c)
);

insert into t1 select a,a, a,a from ten;

set optimizer_trace=1;
explain select * from t1 where a between 1 and 4 and b < 50;

mysql> select
    -> JSON_DETAILED(JSON_EXTRACT(trace, '$**.analyzing_range_alternatives'))
    -> from INFORMATION_SCHEMA.OPTIMIZER_TRACE;

    {
        "range_scan_alternatives": 
        [
            
            {
                "index": "a",
                "ranges": 
                [
                    "(a) < (4,50)"
                ],
                "rowid_ordered": false,
                "using_mrr": false,
                "index_only": false,
                "rows": 1,
                "cost": 2.5002,
                "chosen": true
            }
        ],
        "analyzing_roworder_intersect": 
        {
            "cause": "too few roworder scans"
        },
        "analyzing_index_merge_union": 
        [
        ]
    }
] |

while print_quick produces:

T@18   : | | | | | | | | | | >print_quick
quick range select, key a, length: 8
  1 <= X < 4/50
other_keys: 0x0:
T@18   : | | | | | | | | | | <print_quick

Please investigate why this happens.

On Wed, Apr 03, 2019 at 01:16:56PM +0530, Varun wrote:
> revision-id: a1929d001c7b25e26d178834a196020bade819b8 (mariadb-10.3.6-318-ga1929d001c7)

Any idea why does the commit trigger show "mariadb-10.3.." ? The patch is
against 10.4, isnt it?

> parent(s): 0dc442ac61ac7ff1a1d6bd7d6090c0f2251fb558
> author: Varun Gupta
> committer: Varun Gupta
> timestamp: 2019-04-03 13:10:07 +0530
> message:
> 
> MDEV-18741: Optimizer trace: multi-part key ranges are printed incorrectly
> 
> Changed the function append_range_all_keyparts to use sel_arg_range_seq_init / sel_arg_range_seq_next to produce ranges.
> Also adjusted to print format for the ranges, now the ranges are printed as:
>     (keypart1_min, keypart2_min,..)  OP (keypart1_name,keypart2_name, ..) OP (keypart1_max,keypart2_max, ..)
> 
> Also added more tests for range and index merge access for optimizer trace

...
> diff --git a/sql/opt_range.cc b/sql/opt_range.cc
> index 5ab3d70214d..f53efc55158 100644
> --- a/sql/opt_range.cc
> +++ b/sql/opt_range.cc
> @@ -431,16 +431,18 @@ static int and_range_trees(RANGE_OPT_PARAM *param,
>  static bool remove_nonrange_trees(RANGE_OPT_PARAM *param, SEL_TREE *tree);
>  
>  static void print_key_value(String *out, const KEY_PART_INFO *key_part,
> -                            const uchar *key);
> +                            const uchar* key, uint length);
> +static void print_keyparts_name(String *out, const KEY_PART_INFO *key_part,
> +                         uint n_keypart, key_part_map keypart_map);
Please fix indentation ^

>  
>  static void append_range_all_keyparts(Json_writer_array *range_trace,
> -                                      String *range_string,
> -                                      String *range_so_far, const SEL_ARG *keypart,
> +                                      PARAM param, uint idx,
> +                                      SEL_ARG *keypart,
>                                        const KEY_PART_INFO *key_parts);
>  
>  static
> -void append_range(String *out, const KEY_PART_INFO *key_parts,
> -                  const uchar *min_key, const uchar *max_key, const uint flag);
> +void append_range(String *out, const KEY_PART_INFO *key_part,
> +                          KEY_MULTI_RANGE *range, uint n_key_parts);
Please fix indentation ^
>  
>  
>  /*
> @@ -2273,10 +2275,7 @@ void TRP_RANGE::trace_basic_info(const PARAM *param,
>    // TRP_RANGE should not be created if there are no range intervals
>    DBUG_ASSERT(key);
>  
> -  String range_info;
> -  range_info.length(0);
> -  range_info.set_charset(system_charset_info);
> -  append_range_all_keyparts(&trace_range, NULL, &range_info, key, key_part);
> +  append_range_all_keyparts(&trace_range, *param, key_idx, key, key_part);
>  }
>  
>  
> @@ -2489,10 +2488,8 @@ void TRP_GROUP_MIN_MAX::trace_basic_info(const PARAM *param,
>    // can have group quick without ranges
>    if (index_tree)
>    {
> -    String range_info;
> -    range_info.set_charset(system_charset_info);
> -    append_range_all_keyparts(&trace_range, NULL, &range_info, index_tree,
> -                              key_part);
> +    append_range_all_keyparts(&trace_range, *param, param_idx,
> +                                      index_tree, key_part);
>    }
>  }
>  
> @@ -6398,20 +6395,9 @@ void TRP_ROR_INTERSECT::trace_basic_info(const PARAM *param,
>      trace_isect_idx.add("rows", (*cur_scan)->records);
>  
>      Json_writer_array trace_range(thd, "ranges");
> -    for (const SEL_ARG *current= (*cur_scan)->sel_arg->first(); current;
> -                                                 current= current->next)
> -    {
> -      String range_info;
> -      range_info.set_charset(system_charset_info);
> -      for (const SEL_ARG *part= current; part;
> -           part= part->next_key_part ? part->next_key_part : nullptr)
> -      {
> -        const KEY_PART_INFO *cur_key_part= key_part + part->part;
> -        append_range(&range_info, cur_key_part, part->min_value,
> -                     part->max_value, part->min_flag | part->max_flag);
> -      }
> -      trace_range.add(range_info.ptr(), range_info.length());
> -    }
> +
> +    append_range_all_keyparts(&trace_range, *param, (*cur_scan)->idx,
> +                             (*cur_scan)->sel_arg->first(), key_part);
Please fix indentation ^
>    }
>  }
>  
> @@ -7389,9 +7375,6 @@ static TRP_RANGE *get_key_scans_params(PARAM *param, SEL_TREE *tree,
>          const KEY &cur_key= param->table->key_info[keynr];
>          const KEY_PART_INFO *key_part= cur_key.key_part;
>  
> -        String range_info;
> -        range_info.set_charset(system_charset_info);
> -
>          index_scan->idx= idx;
>          index_scan->keynr= keynr;
>          index_scan->key_info= &param->table->key_info[keynr];
> @@ -7402,8 +7385,8 @@ static TRP_RANGE *get_key_scans_params(PARAM *param, SEL_TREE *tree,
>          *tree->index_scans_end++= index_scan;
>  
>          if (unlikely(thd->trace_started()))
> -          append_range_all_keyparts(&trace_range, NULL, &range_info, key,
> -                                    key_part);
> +          append_range_all_keyparts(&trace_range, (*param), idx,
> +                                              key, key_part);
Please fix indentation ^
>          trace_range.end();
>  
>          trace_idx.add("rowid_ordered", param->is_ror_scan)
> @@ -13554,11 +13537,8 @@ get_best_group_min_max(PARAM *param, SEL_TREE *tree, double read_time)
>          Json_writer_array trace_range(thd, "ranges");
>  
>          const KEY_PART_INFO *key_part= cur_index_info->key_part;
> -
> -        String range_info;
> -        range_info.set_charset(system_charset_info);
> -        append_range_all_keyparts(&trace_range, NULL, &range_info,
> -                                  cur_index_tree, key_part);
> +        append_range_all_keyparts(&trace_range, *param, cur_param_idx,
> +                                          cur_index_tree, key_part);
Please fix indentation ^
>        }
>      }
>      cost_group_min_max(table, cur_index_info, cur_used_key_parts,
> @@ -15730,12 +15710,17 @@ void QUICK_GROUP_MIN_MAX_SELECT::dbug_dump(int indent, bool verbose)
>  }
>  
>  #endif /* !DBUG_OFF */
> +
>  static
>  void append_range(String *out, const KEY_PART_INFO *key_part,
> -                  const uchar *min_key, const uchar *max_key, const uint flag)
> +                  KEY_MULTI_RANGE *range, uint n_key_parts)
>  {
> -  if (out->length() > 0)
> -    out->append(STRING_WITH_LEN(" AND "));
> +  uint flag= range->range_flag;
> +  String key_name;
> +  key_name.set_charset(system_charset_info);
> +  key_part_map keypart_map= range->start_key.length ?
> +                            range->start_key.keypart_map :
> +                            range->end_key.keypart_map;
>  
>    if (flag & GEOM_FLAG)
>    {
> @@ -15744,22 +15729,24 @@ void append_range(String *out, const KEY_PART_INFO *key_part,
>        range types, so printing "col < some_geom" doesn't make sense.
>        Just print the column name, not operator.
>      */
> -    out->append(key_part->field->field_name);
> +    print_keyparts_name(out, key_part, n_key_parts, keypart_map);
>      out->append(STRING_WITH_LEN(" "));
> -    print_key_value(out, key_part, min_key);
> +    print_key_value(out, key_part, range->start_key.key,
> +                    range->start_key.length);
>      return;
>    }
>  
>    if (!(flag & NO_MIN_RANGE))
>    {
> -    print_key_value(out, key_part, min_key);
> +    print_key_value(out, key_part, range->start_key.key,
> +                    range->start_key.length);
>      if (flag & NEAR_MIN)
>        out->append(STRING_WITH_LEN(" < "));
>      else
>        out->append(STRING_WITH_LEN(" <= "));
>    }
>  
> -  out->append(key_part->field->field_name);
> +  print_keyparts_name(out, key_part, n_key_parts, keypart_map);
>  
>    if (!(flag & NO_MAX_RANGE))
>    {
> @@ -15767,7 +15754,8 @@ void append_range(String *out, const KEY_PART_INFO *key_part,
>        out->append(STRING_WITH_LEN(" < "));
>      else
>        out->append(STRING_WITH_LEN(" <= "));
> -    print_key_value(out, key_part, max_key);
> +    print_key_value(out, key_part, range->end_key.key,
> +                    range->end_key.length);
>    }
>  }
>  
> @@ -15775,60 +15763,39 @@ void append_range(String *out, const KEY_PART_INFO *key_part,
>  
>    Add ranges to the trace
>    For ex:
> -    query: select * from t1 where a=2 ;
> -    and we have an index on a , so we create a range
> -      2 <= a <= 2
> +    lets say we have an index a_b(a,b)
> +    query: select * from t1 where a=2 and b=4 ;
> +    so we create a range:
> +      (2,4) <= (a,b) <= (2,4)
>      this is added to the trace
>  */
>  
>  static void append_range_all_keyparts(Json_writer_array *range_trace,
> -                                      String *range_string,
> -                                      String *range_so_far, const SEL_ARG *keypart,
> +                                      PARAM param, uint idx,
> +                                      SEL_ARG *keypart,
>                                        const KEY_PART_INFO *key_parts)
>  {
> -
> -  DBUG_ASSERT(keypart);
> -  DBUG_ASSERT(keypart && keypart != &null_element);
> -
> -  // Navigate to first interval in red-black tree
> +  SEL_ARG_RANGE_SEQ seq;
> +  KEY_MULTI_RANGE range;
> +  range_seq_t seq_it;
> +  uint flags= 0;
> +  RANGE_SEQ_IF seq_if = {NULL, sel_arg_range_seq_init,
> +                         sel_arg_range_seq_next, 0, 0};
> +  KEY *keyinfo= param.table->key_info + param.real_keynr[idx];
> +  uint n_key_parts= param.table->actual_n_key_parts(keyinfo);
> +  seq.keyno= idx;
> +  seq.real_keyno= param.real_keynr[idx];
> +  seq.param= &param;
> +  seq.start= keypart;
>    const KEY_PART_INFO *cur_key_part= key_parts + keypart->part;
> -  const SEL_ARG *keypart_range= keypart->first();
> -  const size_t save_range_so_far_length= range_so_far->length();
> -
> +  seq_it= seq_if.init((void *) &seq, 0, flags);
>  
> -  while (keypart_range)
> +  while (!seq_if.next(seq_it, &range))
>    {
> -    // Append the current range predicate to the range String
> -    switch (keypart->type)
> -    {
> -      case SEL_ARG::Type::KEY_RANGE:
> -        append_range(range_so_far, cur_key_part, keypart_range->min_value,
> -                     keypart_range->max_value,
> -                     keypart_range->min_flag | keypart_range->max_flag);
> -        break;
> -      case SEL_ARG::Type::MAYBE_KEY:
> -        range_so_far->append("MAYBE_KEY");
> -        break;
> -      case SEL_ARG::Type::IMPOSSIBLE:
> -        range_so_far->append("IMPOSSIBLE");
> -        break;
> -      default:
> -        DBUG_ASSERT(false);
> -        break;
> -    }
> -
> -    if (keypart_range->next_key_part &&
> -        keypart_range->next_key_part->part ==
> -              keypart_range->part + 1 &&
> -        keypart_range->is_singlepoint())
> -    {
> -      append_range_all_keyparts(range_trace, range_string, range_so_far,
> -                                keypart_range->next_key_part, key_parts);
> -    }
> -    else
> -      range_trace->add(range_so_far->c_ptr_safe(), range_so_far->length());
> -    keypart_range= keypart_range->next;
> -    range_so_far->length(save_range_so_far_length);
> +    String range_info;
> +    range_info.set_charset(system_charset_info);
> +    append_range(&range_info, cur_key_part, &range, n_key_parts);
> +    range_trace->add(range_info.c_ptr_safe(), range_info.length());
>    }
>  }
>  
> @@ -15838,70 +15805,110 @@ static void append_range_all_keyparts(Json_writer_array *range_trace,
>    @param[out] out          String the key is appended to
>    @param[in]  key_part     Index components description
>    @param[in]  key          Key tuple
> +  @param[in]  used_length  length of the key tuple
>  */
> +
>  static void print_key_value(String *out, const KEY_PART_INFO *key_part,
> -                            const uchar *key)
> +                            const uchar* key, uint used_length)
>  {
> +  out->append(STRING_WITH_LEN("("));
>    Field *field= key_part->field;
> +  StringBuffer<128> tmp(system_charset_info);
> +  TABLE *table= field->table;
> +  uint store_length;
> +  my_bitmap_map *old_sets[2];
> +  dbug_tmp_use_all_columns(table, old_sets, table->read_set, table->write_set);
> +  const uchar *key_end= key+used_length;
>  
> -  if (field->flags & BLOB_FLAG)
> +  for (; key < key_end; key+=store_length, key_part++)
>    {
> -    // Byte 0 of a nullable key is the null-byte. If set, key is NULL.
> -    if (field->real_maybe_null() && *key)
> -      out->append(STRING_WITH_LEN("NULL"));
> -    else
> -      (field->type() == MYSQL_TYPE_GEOMETRY)
> -          ? out->append(STRING_WITH_LEN("unprintable_geometry_value"))
> -          : out->append(STRING_WITH_LEN("unprintable_blob_value"));
> -    return;
> -  }
> +    field= key_part->field;
> +    store_length= key_part->store_length;
> +    if (field->flags & BLOB_FLAG)
> +    {
> +      // Byte 0 of a nullable key is the null-byte. If set, key is NULL.
> +      if (field->real_maybe_null() && *key)
> +        out->append(STRING_WITH_LEN("NULL"));
> +      else
> +        (field->type() == MYSQL_TYPE_GEOMETRY)
> +            ? out->append(STRING_WITH_LEN("unprintable_geometry_value"))
> +            : out->append(STRING_WITH_LEN("unprintable_blob_value"));
> +      goto next;
> +    }
>  
> -  uint store_length= key_part->store_length;
> +    if (field->real_maybe_null())
> +    {
> +      /*
> +        Byte 0 of key is the null-byte. If set, key is NULL.
> +        Otherwise, print the key value starting immediately after the
> +        null-byte
> +      */
> +      if (*key)
> +      {
> +        out->append(STRING_WITH_LEN("NULL"));
> +        goto next;
> +      }
> +      key++;  // Skip null byte
> +      store_length--;
> +    }
>  
> -  if (field->real_maybe_null())
> -  {
>      /*
> -      Byte 0 of key is the null-byte. If set, key is NULL.
> -      Otherwise, print the key value starting immediately after the
> -      null-byte
> +      Binary data cannot be converted to UTF8 which is what the
> +      optimizer trace expects. If the column is binary, the hex
> +      representation is printed to the trace instead.
>      */
> -    if (*key)
> +    if (field->flags & BINARY_FLAG)
>      {
> -      out->append(STRING_WITH_LEN("NULL"));
> -      return;
> +      out->append("0x");
> +      for (uint i = 0; i < store_length; i++)
> +      {
> +        out->append(_dig_vec_lower[*(key + i) >> 4]);
> +        out->append(_dig_vec_lower[*(key + i) & 0x0F]);
> +      }
> +      goto next;
>      }
> -    key++;  // Skip null byte
> -    store_length--;
> +
> +    field->set_key_image(key, key_part->length);
> +    if (field->type() == MYSQL_TYPE_BIT)
> +      (void)field->val_int_as_str(&tmp, 1);  // may change tmp's charset
> +    else
> +      field->val_str(&tmp);  // may change tmp's charset
> +    out->append(tmp.ptr(), tmp.length(), tmp.charset());
> +
> +  next:
> +    if (key + store_length < key_end)
> +      out->append(STRING_WITH_LEN(","));
>    }
> +  dbug_tmp_restore_column_maps(table->read_set, table->write_set, old_sets);
> +  out->append(STRING_WITH_LEN(")"));
> +}
>  
> -  /*
> -    Binary data cannot be converted to UTF8 which is what the
> -    optimizer trace expects. If the column is binary, the hex
> -    representation is printed to the trace instead.
> -  */
> -  if (field->flags & BINARY_FLAG)
> +/**
> +  Print key parts involed in a range
> +  @param[out] out          String the key is appended to
> +  @param[in]  key_part     Index components description
> +  @param[in]  n_keypart    Number of keyparts in index
> +  @param[in]  keypart_map  map for keyparts involved in the range
> +*/
> +
> +void print_keyparts_name(String *out, const KEY_PART_INFO *key_part,
> +                         uint n_keypart, key_part_map keypart_map)
> +{
> +  uint i;
> +  out->append(STRING_WITH_LEN("("));
> +  bool first_keypart= TRUE;
> +  for (i=0; i < n_keypart; key_part++, i++)
>    {
> -    out->append("0x");
> -    for (uint i = 0; i < store_length; i++)
> +    if (keypart_map & (1 << i))
>      {
> -      out->append(_dig_vec_lower[*(key + i) >> 4]);
> -      out->append(_dig_vec_lower[*(key + i) & 0x0F]);
> +      if (first_keypart)
> +        first_keypart= FALSE;
> +      else
> +        out->append(STRING_WITH_LEN(","));
> +      out->append(key_part->field->field_name);
>      }
> -    return;
> +    else
> +      break;
>    }
> -
> -  StringBuffer<128> tmp(system_charset_info);
> -  TABLE *table= field->table;
> -  my_bitmap_map *old_sets[2];
> -
> -  dbug_tmp_use_all_columns(table, old_sets, table->read_set, table->write_set);
> -
> -  field->set_key_image(key, key_part->length);
> -  if (field->type() == MYSQL_TYPE_BIT)
> -    (void)field->val_int_as_str(&tmp, 1);  // may change tmp's charset
> -  else
> -    field->val_str(&tmp);  // may change tmp's charset
> -  out->append(tmp.ptr(), tmp.length(), tmp.charset());
> -
> -  dbug_tmp_restore_column_maps(table->read_set, table->write_set, old_sets);
> -}
> +  out->append(STRING_WITH_LEN(")"));
> +}

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