maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Varun,

The patch is mostly fine. Please find some input on the comments / coding style
below. Ok to push after it is addressed.

On Fri, May 17, 2019 at 01:44:36PM +0530, Varun wrote:
> revision-id: 66349551bf3a266fbefaebfe044abd05108723c2 (mariadb-10.3.12-205-g66349551bf3)
> parent(s): 3d56adbfac394b2b3ffd22a89fe7c2978ed9a505
> author: Varun Gupta
> committer: Varun Gupta
> timestamp: 2019-05-17 13:44:05 +0530
> message:
> 
> MDEV-16214: Incorrect plan taken by the optimizer , uses INDEX instead of ref access with ORDER BY
> 
> The issue in this case is that we take in account the estimates from quick keys instead of rec_per_key.
> The estimates for quick keys are better than rec_per_key only if we have ref(const), so we need to check
> that all keyparts in the ref key are of the type ref(const).
> 
> ---
>  mysql-test/main/order_by.result | 57 +++++++++++++++++++++++++++++++++++++++++
>  mysql-test/main/order_by.test   | 37 ++++++++++++++++++++++++++
>  sql/sql_select.cc               | 42 ++++++++++++++++++++----------
>  3 files changed, 122 insertions(+), 14 deletions(-)
...
> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index f36a68bc7ae..8cdcf3afc8b 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -9986,31 +9986,35 @@ static bool create_ref_for_key(JOIN *join, JOIN_TAB *j,
>          j->ref.null_rejecting|= (key_part_map)1 << i;
>        keyuse_uses_no_tables= keyuse_uses_no_tables && !keyuse->used_tables;
>        /*
> -        Todo: we should remove this check for thd->lex->describe on the next
> -        line. With SHOW EXPLAIN code, EXPLAIN printout code no longer depends
> -        on it. However, removing the check caused change in lots of query
> -        plans! Does the optimizer depend on the contents of
> -        table_ref->key_copy ? If yes, do we produce incorrect EXPLAINs? 
> +        We don't want to compute heavy expressions in EXPLAIN, an example would
> +        select * from t1 where t1.key=(select thats very heavy);
> +
> +        (select thats very heavy) => is a constant here
> +        eg: (select avg(order_cost) from orders) => constant but expensive
>        */
>        if (!keyuse->val->used_tables() && !thd->lex->describe)
>        {					// Compare against constant
> -	store_key_item tmp(thd, 
> +        store_key_item tmp(thd,
>                             keyinfo->key_part[i].field,
>                             key_buff + maybe_null,
>                             maybe_null ?  key_buff : 0,
>                             keyinfo->key_part[i].length,
>                             keyuse->val,
>                             FALSE);
> -	if (unlikely(thd->is_fatal_error))
> -	  DBUG_RETURN(TRUE);
> -	tmp.copy();
> +        if (unlikely(thd->is_fatal_error))
> +          DBUG_RETURN(TRUE);
> +        tmp.copy();
>          j->ref.const_ref_part_map |= key_part_map(1) << i ;
>        }
>        else
> -	*ref_key++= get_store_key(thd,
> -				  keyuse,join->const_table_map,
> -				  &keyinfo->key_part[i],
> -				  key_buff, maybe_null);
> +      {
> +        *ref_key++= get_store_key(thd,
> +                                  keyuse,join->const_table_map,
> +                                  &keyinfo->key_part[i],
> +                                  key_buff, maybe_null);
> +        if (!keyuse->val->used_tables())
> +          j->ref.const_ref_part_map |= key_part_map(1) << i ;
> +      }
>        /*
>  	Remember if we are going to use REF_OR_NULL
>  	But only if field _really_ can be null i.e. we force JT_REF
> @@ -25256,6 +25260,8 @@ bool JOIN_TAB::save_explain_data(Explain_table_access *eta,
>          {
>            if (!(eta->ref_list.append_str(thd->mem_root, "const")))
>              return 1;
> +          if (thd->lex->describe)
> +            key_ref++;
This is very cryptic.

Please provide an explanation, something like:

  create_ref_for_key() handles keypart=const equalities as follows:
  - non-EXPLAIN execution will copy the "const" to lookup tuple immediately
    and will not add an element to ref.key_copy 
  - EXPLAIN will put an element into ref.key_copy. Since we've just printed
    "const" for it, we should skip it here.

>          }
>          else
>          {
> @@ -26921,7 +26927,15 @@ test_if_cheaper_ordering(const JOIN_TAB *tab, ORDER *order, TABLE *table,
>    */
>    if (ref_key >= 0 && ref_key != MAX_KEY && tab->type == JT_REF)
>    {
> -    if (table->quick_keys.is_set(ref_key))
> +    /*
> +      For all the parts of the ref key we check if all of them belong
> +      to the type ref(const). This is done because if all parts of the ref
> +      key are of type  ref(const), then we are sure that the estimates
> +      provides by quick keys is better than that provide by rec_per_key.

The above is hard to read and has typos. Here's a shorter variant:

  If ref access uses keypart=const for all its key parts,
  and quick select uses the same # of key parts, then they are equivalent.
  Reuse #rows estimate from quick select as it is more precise.

> +    */
> +    if (tab->ref.const_ref_part_map == make_prev_keypart_map(tab->ref.key_parts) &&
> +        table->quick_keys.is_set(ref_key) &&
> +             table->quick_key_parts[ref_key] == tab->ref.key_parts)
Please fix indetation;
One of the lines is too long.

>        refkey_rows_estimate= table->quick_rows[ref_key];
>      else
>      {

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