maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Igor!

On Apr 30, Igor Babaev wrote:
> revision-id: f3dda8e2002dc2ad630c07bda0d8c00be7269907 (mariadb-10.2.14-72-gf3dda8e)
> parent(s): b4c5e4a717e3ce2c2d434106cc74417fe9a1d3dc
> author: Igor Babaev
> committer: Igor Babaev
> timestamp: 2018-04-30 21:26:38 -0700
> message:
> 
> MDEV-15357 Wrong result with join_cache_level=4, BNLH join
> 
> This bug was introduced by the architectural changes of the patch
> for MDEV-11640. The patch moved initialization of join caches after
> the call of make_join_readinfo(). As any failure to initialize
> a join cache caused denial of its usage the execution code for
> the query had to be revised. This revision required rolling back many
> actions taken by make_join_readinfo(). It was not done in the patch.
> As a result if a denial of join cache happened some join conditions
> could be lost. This was exactly the cause of wrong results in the
> bug's reported test case.
> 
> Thus the introduced architectural change is not valid and it would be
> better to roll it back. At the same time two new methods
> adjust_read_set_for_vcol_keyread() and get_covering_index_for_scan()
> were added to the class JOIN_TAB to resolve the problems of MDEV-11640.
> 
> diff --git a/sql/sql_join_cache.cc b/sql/sql_join_cache.cc
> index 3612cb6..242cb91 100644
> --- a/sql/sql_join_cache.cc
> +++ b/sql/sql_join_cache.cc
> @@ -226,6 +226,16 @@ void JOIN_CACHE::calc_record_fields()
>      flag_fields+= MY_TEST(tab->table->maybe_null);
>      fields+= tab->used_fields;
>      blobs+= tab->used_blobs;
> +    if (tab->type == JT_ALL || tab->type == JT_HASH)
> +    {
> +      uint idx= tab->get_covering_index_for_scan();

as you moved JOIN_CACHE initialization back, I suspect that
this calc_record_fields() now happens too early for
get_covering_index_for_scan() to be useful.

And why only JT_ALL and JT_HASH?

> +      if (idx != MAX_KEY)
> +      {
> +        fields-= bitmap_bits_set(tab->table->read_set);
> +        tab->adjust_read_set_for_vcol_keyread(idx);
> +        fields+= bitmap_bits_set(tab->table->read_set);
> +      }
> +    }
>    }
>    if ((with_match_flag= join_tab->use_match_flag()))
>      flag_fields++;
> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index 30ef26f..e5b75ec 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -11404,6 +11375,61 @@ void JOIN_TAB::remove_redundant_bnl_scan_conds()
>  }
>  
>  
> +uint JOIN_TAB::get_covering_index_for_scan()
> +{
> +  int idx= MAX_KEY;
> +  if (table->no_keyread)
> +    return idx;
> +  if (select && select->quick &&
> +      select->quick->index != MAX_KEY && //not index_merge
> +      table->covering_keys.is_set(select->quick->index))
> +    idx= select->quick->index;
> +  else if (!table->covering_keys.is_clear_all() &&
> +          !(select && select->quick))
> +  {                                    // Only read index tree
> +    if (loosescan_match_tab)
> +      idx= loosescan_key;
> +    else
> +    {
> +#ifdef BAD_OPTIMIZATION
> +      /*
> +        It has turned out that the below change, while speeding things
> +        up for disk-bound loads, slows them down for cases when the data
> +        is in disk cache (see BUG#35850):
> +        See bug #26447: "Using the clustered index for a table scan
> +        is always faster than using a secondary index".
> +      */
> +      if (table->s->primary_key != MAX_KEY &&
> +          table->file->primary_key_is_clustered())
> +        idx= table->s->primary_key;
> +      else
> +#endif
> +        idx= find_shortest_key(table, & table->covering_keys);
> +    }
> +  }
> +  return idx;
> +}
> +
> +
> +void JOIN_TAB::adjust_read_set_for_vcol_keyread(uint keyread_idx)
> +{
> +  if (!table->vcol_set || bitmap_is_clear_all(table->vcol_set))
> +    return;
> +
> +  MY_BITMAP *keyread_set= &table->cond_set;    // is free for use at call

normally one uses tmp_set for this, not hijacks cond_set

> +  bitmap_clear_all(keyread_set);
> +  table->mark_columns_used_by_index(keyread_idx, keyread_set);
> +  bitmap_intersect(keyread_set, table->read_set);
> +  bitmap_intersect(keyread_set, table->vcol_set);

this looks wrong, you probably should do

  keyread_set INTERSECT (read_set UNION vcol_set)

> +  if (!bitmap_is_clear_all(keyread_set))
> +  {
> +    bitmap_clear_all(keyread_set);
> +    table->mark_columns_used_by_index(keyread_idx, keyread_set);
> +    bitmap_intersect(table->read_set, keyread_set);
> +  }

I don't understand that.
1. What if bitmap_is_clear_all(keyread_set) - what does it mean?
2. Why do you recalculate keyread_set, why not simply copy keyread_set
   into read_set ?

> +}
> +
> +
>  /*
>    Plan refinement stage: do various setup things for the executor
>  
> @@ -11526,7 +11552,10 @@ make_join_readinfo(JOIN *join, ulonglong options, uint no_jbuf_after)
>        tab->read_first_record= tab->type == JT_SYSTEM ? join_read_system
>                                                       : join_read_const;
>        if (table->covering_keys.is_set(tab->ref.key) && !table->no_keyread)
> +      {
> +        tab->adjust_read_set_for_vcol_keyread(tab->ref.key);
>          table->file->ha_start_keyread(tab->ref.key);

better combine both into

    inline void JOIN_TAB::start_keyread()
    {
      adjust_read_set_for_vcol_keyread(ref.key);
      tab->table->file->ha_start_keyread(ref.key);
    }

or even JOIN_TAB::start_keyread_if_needed() putting the if() inside too

> +      }
>        else if ((!jcl || jcl > 4) && !tab->ref.is_access_triggered())
>          push_index_cond(tab, tab->ref.key);
>        break;
> @@ -11606,35 +11641,19 @@ make_join_readinfo(JOIN *join, ulonglong options, uint no_jbuf_after)
>             }
>           }
>         }
> -       if (!table->no_keyread)
> +       uint idx= tab->get_covering_index_for_scan();
> +       if (idx != MAX_KEY)
>         {
>           if (tab->select && tab->select->quick &&
>                tab->select->quick->index != MAX_KEY && //not index_merge
>               table->covering_keys.is_set(tab->select->quick->index))
> -            table->file->ha_start_keyread(tab->select->quick->index);
> +            table->file->ha_start_keyread(idx);

why do you need an if() here - you already checked that in
get_covering_index_for_scan().

I generally don't like this code at all. In JOIN_CACHE you simply
use tab->get_covering_index_for_scan(), but here you use it
*plus* a bunch of if()s afterwards. It surely creates an impression
that JOIN_CACHE and make_join_readinfo may have a different idea
about whether keyread is used.

Ideally here should be just

      if (idx != MAX_KEY)
        tab->start_keyread(idx);

and even then it would be kinda questionable (because preconditions
might change between two get_covering_index_for_scan() calls).

>           else if (!table->covering_keys.is_clear_all() &&
>                    !(tab->select && tab->select->quick))
> -         {                                     // Only read index tree
> -            if (tab->loosescan_match_tab)
> -              tab->index= tab->loosescan_key;
> -            else 
> -            {
> -#ifdef BAD_OPTIMIZATION
> -              /*
> -                It has turned out that the below change, while speeding things
> -                up for disk-bound loads, slows them down for cases when the data
> -                is in disk cache (see BUG#35850):
> -                See bug #26447: "Using the clustered index for a table scan
> -                is always faster than using a secondary index".
> -              */
> -              if (table->s->primary_key != MAX_KEY &&
> -                  table->file->primary_key_is_clustered())
> -                tab->index= table->s->primary_key;
> -              else
> -#endif
> -                tab->index=find_shortest_key(table, & table->covering_keys);
> -            }
> +         {                                     // Only read index tree
> +            tab->index= idx;
>             tab->read_first_record= join_read_first;
> +            table->file->ha_start_keyread(tab->index);
>              /* Read with index_first / index_next */
>             tab->type= tab->type == JT_ALL ? JT_NEXT : JT_HASH_NEXT;            
>           }

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx