maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Michael!

On Sep 08, Michael Widenius wrote:
> revision-id: bf8bd057344 (mariadb-10.5.2-267-gbf8bd057344)
> parent(s): 32a29afea77
> author: Michael Widenius <michael.widenius@xxxxxxxxx>
> committer: Michael Widenius <michael.widenius@xxxxxxxxx>
> timestamp: 2020-09-02 20:58:32 +0300
> message:
> 
> MDEV-23001 Precreate static Item_bool() to simplify code
> 
> The following changes where done:
> - Create global Item: Item_false and Item_true
> - Replace all creation if 'FALSE' and 'TRUE' top level items used for
>   WHERE/HAVING/ON clauses to use Item_false and Item_true.
> 
> The benefit are:
> - Less and faster code
> - No test needed if we where able to create the new item.
> - Fixed possible errors if 'new' would have failed for the Item_bool's
> 
> diff --git a/sql/item.cc b/sql/item.cc
> index dbf20f31d7a..b24103f2330 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -55,7 +55,8 @@ const char *item_empty_name="";
>  const char *item_used_name= "\0";
>  
>  static int save_field_in_field(Field *, bool *, Field *, bool);
> -
> +const Item_bool_static Item_false("FALSE", 0);
> +const Item_bool_static Item_true("TRUE", 1);

The maain question, that Bar asked in his email - we have tons of
modifiable Item members. How do you know none of them will modified in
your Item_bool_static objects?

>  /**
>    Compare two Items for List<Item>::add_unique()
> @@ -412,7 +413,6 @@ Item::Item(THD *thd):
>    is_expensive_cache(-1), rsize(0), name(null_clex_str), orig_name(0),
>    common_flags(IS_AUTO_GENERATED_NAME)
>  {
> -  DBUG_ASSERT(thd);
>    marker= 0;
>    maybe_null= null_value= with_window_func= with_field= false;
>    in_rollup= 0;
> @@ -421,6 +421,9 @@ Item::Item(THD *thd):
>     /* Initially this item is not attached to any JOIN_TAB. */
>    join_tab_idx= MAX_TABLES;
>  
> +   /* thd is NULL in case of static items like Item_true */
> +  if (!thd)
> +    return;

I don't like that. thd can be NULL only twice, for statically
initialized Item_true and Item_false. For that you removed a useful
assert and added an if() that is completely unnecessary for billions of
items created runtime.

Better to have special contructor just for these two very special items.
Or to create a THD temporarily when constructing them.

>    /* Put item in free list so that we can free all items at end */
>    next= thd->free_list;
>    thd->free_list= this;
> @@ -3614,6 +3618,19 @@ void Item_int::print(String *str, enum_query_type query_type)
>  }
>  
>  
> +/*
> +  This function is needed to ensure that Item_bool_static doesn't change
> +  the value of the member str_value.
> +*/
> +
> +void Item_bool::print(String *str, enum_query_type query_type)
> +{
> +  // my_charset_bin is good enough for numbers
> +  String tmp(value ? (char*) "1" : (char*) "0" , 1, &my_charset_bin);
> +  str->append(tmp);
> +}

This is not needed anymore. Item_bool inherits from Item_int,
and Item_int::print is

  void Item_int::print(String *str, enum_query_type query_type)
  {
    StringBuffer<LONGLONG_BUFFER_SIZE> buf;
    // my_charset_bin is good enough for numbers
    buf.set_int(value, unsigned_flag, &my_charset_bin);
    str->append(buf);
  }

You removed str_value from Item_int::print in July.

> +
> +
>  Item *Item_bool::neg_transformer(THD *thd)
>  {
>    value= !value;
> diff --git a/sql/item.h b/sql/item.h
> index c4fbf8f9c0a..4cdee637415 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -1969,7 +1969,7 @@ class Item: public Value_source,
>    virtual bool limit_index_condition_pushdown_processor(void *arg) { return 0; }
>    virtual bool exists2in_processor(void *arg) { return 0; }
>    virtual bool find_selective_predicates_list_processor(void *arg) { return 0; }
> -  bool cleanup_is_expensive_cache_processor(void *arg)
> +  virtual bool cleanup_is_expensive_cache_processor(void *arg)

You've added a new virtual method? You tried so hard to remove virtual
methods that it's a bit strange to see a new virtual method added
specifically for these two static items. Why does it matter what
cleanup_is_expensive_cache_processor will do with them? You overwrite
is_expensive() anyway.

>    {
>      is_expensive_cache= (int8)(-1);
>      return 0;
> @@ -3225,6 +3225,8 @@ class Item_literal: public Item_basic_constant
>    bool check_partition_func_processor(void *int_arg) { return false;}
>    bool const_item() const { return true; }
>    bool basic_const_item() const { return true; }
> +  bool is_expensive() { return false; }
> +  bool cleanup_is_expensive_cache_processor(void *arg) { return 0; }
>  };
>  
> diff --git a/sql/opt_index_cond_pushdown.cc b/sql/opt_index_cond_pushdown.cc
> index 360ae028f36..d66f2f2e14e 100644
> --- a/sql/opt_index_cond_pushdown.cc
> +++ b/sql/opt_index_cond_pushdown.cc
> @@ -185,8 +185,8 @@ bool uses_index_fields_only(Item *item, TABLE *tbl, uint keyno,
>  static Item *make_cond_for_index(THD *thd, Item *cond, TABLE *table, uint keyno,
>                                   bool other_tbls_ok)
>  {
> -  if (!cond)
> -    return NULL;
> +  if (!cond || cond->basic_const_item())
> +    return cond;

The effect of this change is that you don't set
cond->marker= ICP_COND_USES_INDEX_ONLY for basic_const_item's anymore.

This looked suspicious at first. But then I noticed (and Sergey Petrunia
confirmed) that ICP_COND_USES_INDEX_ONLY is not used at all. Since 5.3.

So here I'd suggest to revert this your change and completely remove any
ICP_COND_USES_INDEX_ONLY manipulations together with the
ICP_COND_USES_INDEX_ONLY definition.

>    if (cond->type() == Item::COND_ITEM)
>    {
>      uint n_marked= 0;
> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> index 144b86e8fc9..1140f32c9c9 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -2313,6 +2313,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
>      thd->update_server_status();
>      thd->protocol->end_statement();
>      query_cache_end_of_result(thd);
> +    /* Check that the query didn't change const items */

what does that mean? Is it a TODO comment? Or it documents what happens
below?

>    }
>    if (drop_more_results)
>      thd->server_status&= ~SERVER_MORE_RESULTS_EXISTS;
> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index 22bf2eb37b5..fdd609a96ce 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -22560,6 +22560,8 @@ make_cond_for_table_from_pred(THD *thd, Item *root_cond, Item *cond,
>        return new_cond;
>      }
>    }
> +  else if (cond->basic_const_item())
> +    return cond;

this is rather fragile, isn't it?
You skip item->set_join_tab_idx(join_tab_idx_arg),
quite understandably.

And currently, as far as I can see, item->get_join_tab_idx()
is always used under if (!item->const_item()), so what you did is safe.

But can it change in the future? Or is join_tab_idx something that
conceptually doesn't make sense for const items?

>  
>    if (is_top_and_level && used_table == rand_table_bit &&
>        (cond->used_tables() & ~OUTER_REF_TABLE_BIT) != rand_table_bit)

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx