maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Apr 21, Alexander Barkov wrote:
> Hi Sergei,
> 
> Please review a patch for MDEV-8024.
> 
> It's needed to as a pre-requisite for
> "MDEV-7950 Item_func::type() takes 0.26% in OLTP RO"

Looks ok. Just a few comments:

> diff --git a/sql/item.cc b/sql/item.cc
> index e9bbdc7..502b8ae 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -59,6 +59,21 @@ bool cmp_items(Item *a, Item *b)
>  }
>  
>  
> +void
> +Used_tables_and_const_cache::
> +  used_tables_and_const_cache_update_and_join(List<Item> &list)
> +{
> +  List_iterator_fast<Item> li(list);
> +  Item *item;
> +  while ((item=li++))
> +  {
> +    item->update_used_tables();
> +    used_tables_and_const_cache_join(item);
> +  }
> +}

Why didn't you put that in the header? It's as small as the other
used_tables_and_const_cache_update_and_join().

>  /*****************************************************************************
>  ** Item functions
>  *****************************************************************************/
> diff --git a/sql/item_func.h b/sql/item_func.h
> index 653c08e..c78c585 100644
> --- a/sql/item_func.h
> +++ b/sql/item_func.h
> @@ -1367,8 +1362,7 @@ class Item_udf_func :public Item_func
>    {
>      DBUG_ASSERT(fixed == 0);
>      bool res= udf.fix_fields(thd, this, arg_count, args);
> -    used_tables_cache= udf.used_tables_cache;
> -    const_item_cache= udf.const_item_cache;
> +    used_tables_and_const_cache_copy(udf.used_tables_and_const_cache());

why not used_tables_and_const_cache_copy(udf) ?

>      fixed= 1;
>      return res;
>    }
> diff --git a/sql/item_func.cc b/sql/item_func.cc
> index 50bc85f..4007d6b 100644
> --- a/sql/item_func.cc
> +++ b/sql/item_func.cc
> @@ -178,8 +178,44 @@ Item_func::fix_fields(THD *thd, Item **ref)
>    Item **arg,**arg_end;
>    uchar buff[STACK_BUFF_ALLOC];			// Max argument in function
>  
> -  used_tables_cache= not_null_tables_cache= 0;
> -  const_item_cache=1;
> +  // used_tables_and_const_cache_init(); // QQ: is it needed, for safety?

Add an assert, please

> +  /*
> +    Comments about the commented used_tables_and_const_cache_init() above:

Regards,
Sergei