maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Varun,

On Wed, Apr 08, 2020 at 11:48:28PM +0530, Varun wrote:
> revision-id: c1394ab6b5c0830ec09f6afdae11fa82bae1a123 (mariadb-5.5.67-9-gc1394ab6b5c)
> parent(s): 64b70b09e6ac253b7915f6120ade5e69fa750b18
> author: Varun Gupta
> committer: Varun Gupta
> timestamp: 2020-04-08 23:47:03 +0530
> message:
> 
> MDEV-22191: Range access is not picked when index_merge_sort_union is turned off
> 
> When index_merge_sort_union is turned off only ror scans were considered for range
> scans, which is wrong.
> To fix the problem ensure both ror scans and non ror scans are considered for range
> access
> 
> ---
>  mysql-test/r/range.result         | 19 +++++++++++++++++++
>  mysql-test/r/range_mrr_icp.result | 19 +++++++++++++++++++
>  mysql-test/t/range.test           | 14 ++++++++++++++
>  sql/opt_range.cc                  | 16 ++++++++++------
>  4 files changed, 62 insertions(+), 6 deletions(-)
...

> --- a/sql/opt_range.cc
> +++ b/sql/opt_range.cc
> @@ -949,7 +949,8 @@ QUICK_RANGE_SELECT *get_quick_select(PARAM *param,uint index,
>  static TRP_RANGE *get_key_scans_params(PARAM *param, SEL_TREE *tree,
>                                         bool index_read_must_be_used,
>                                         bool update_tbl_stats,
> -                                       double read_time);
> +                                       double read_time,
> +                                       bool ror_scans_required);
>  static
>  TRP_INDEX_INTERSECT *get_best_index_intersect(PARAM *param, SEL_TREE *tree,
>                                                double read_time);
> @@ -3146,7 +3147,7 @@ int SQL_SELECT::test_quick_select(THD *thd, key_map keys_to_use,
>  
>        /* Get best 'range' plan and prepare data for making other plans */
>        if ((range_trp= get_key_scans_params(&param, tree, FALSE, TRUE,
> -                                           best_read_time)))
> +                                           best_read_time, FALSE)))
>        {
>          best_trp= range_trp;
>          best_read_time= best_trp->read_cost;
> @@ -4708,7 +4709,8 @@ TABLE_READ_PLAN *get_best_disjunct_quick(PARAM *param, SEL_IMERGE *imerge,
>    {
>      DBUG_EXECUTE("info", print_sel_tree(param, *ptree, &(*ptree)->keys_map,
>                                          "tree in SEL_IMERGE"););
> -    if (!(*cur_child= get_key_scans_params(param, *ptree, TRUE, FALSE, read_time)))
> +    if (!(*cur_child= get_key_scans_params(param, *ptree, TRUE, FALSE,
> +                                           read_time, TRUE)))
>      {
>        /*
>          One of index scans in this index_merge is more expensive than entire
> @@ -5030,7 +5032,7 @@ TABLE_READ_PLAN *merge_same_index_scans(PARAM *param, SEL_IMERGE *imerge,
>           index merge retrievals are not well calibrated
>      */
>      trp= get_key_scans_params(param, *imerge->trees, FALSE, TRUE,
> -                              read_time);
> +                              read_time, TRUE);
>    }

As far as I understand, this call constructs range scan, not a portion of
index_merge scan.
So, it is not correct to require a ROR scan here.

>  
>    DBUG_RETURN(trp); 
> @@ -6747,6 +6749,7 @@ TRP_ROR_INTERSECT *get_best_covering_ror_intersect(PARAM *param,
>        index_read_must_be_used if TRUE, assume 'index only' option will be set
>                               (except for clustered PK indexes)
>        read_time    don't create read plans with cost > read_time.
> +      ror_scans_required           set to TRUE for index merge
>    RETURN
>      Best range read plan
>      NULL if no plan found or error occurred
> @@ -6755,7 +6758,8 @@ TRP_ROR_INTERSECT *get_best_covering_ror_intersect(PARAM *param,
>  static TRP_RANGE *get_key_scans_params(PARAM *param, SEL_TREE *tree,
>                                         bool index_read_must_be_used, 
>                                         bool update_tbl_stats,
> -                                       double read_time)
> +                                       double read_time,
> +                                       bool ror_scans_required)
>  {
>    uint idx;
>    SEL_ARG **key,**end, **key_to_read= NULL;
> @@ -6802,7 +6806,7 @@ static TRP_RANGE *get_key_scans_params(PARAM *param, SEL_TREE *tree,
>                                          update_tbl_stats, &mrr_flags,
>                                          &buf_size, &cost);
>  
> -      if (!param->is_ror_scan &&
> +      if (ror_scans_required && !param->is_ror_scan &&
>            !optimizer_flag(param->thd, OPTIMIZER_SWITCH_INDEX_MERGE_SORT_UNION))

So, the meaning if "ror_scans_required" parameter is actually:

" require ror scans if index_merge optimizer settings are such that it cannot
use ROR-scans"

This is very complicated and non-orthogonal.
How about moving the  

  "!optimizer_flag(param->thd, OPTIMIZER_SWITCH_INDEX_MERGE_SORT_UNION)"

check up into get_best_disjunct_quick()? 

That is, get_best_disjunct_quick() will pass ror_scans_required=true iff
OPTIMIZER_SWITCH_INDEX_MERGE_SORT_UNION is not set.

This way, index_merge switches will be only checked in index_merge code and 
ror_scans_required will mean what its name says.


>        {
>          /* The scan is not a ROR-scan, just skip it */

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