maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Monty!

On Mar 25, Michael Widenius wrote:
> revision-id: 2be9b69f4ff (mariadb-10.5.2-510-g2be9b69f4ff)
> parent(s): cb545f11169
> author: Michael Widenius <michael.widenius@xxxxxxxxx>
> committer: Michael Widenius <michael.widenius@xxxxxxxxx>
> timestamp: 2021-03-24 14:05:15 +0200
> 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:
> - 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

agree

> - Less and faster code

I don't quite agree with that, it's not less code:

>  7 files changed, 65 insertions(+), 43 deletions(-)

and I doubt that speedup can be measured in any benchmarks whatsoever.

> diff --git a/sql/item.cc b/sql/item.cc
> index 1a86b8b3114..0f160fa257b 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 main question - we have tons of modifiable Item members. How do you
know none of them will modified in your Item_bool_static objects?

> @@ -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;

No, please, don't.
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.

>    /* Put item in free list so that we can free all items at end */
>    next= thd->free_list;
>    thd->free_list= this;
> @@ -3637,6 +3641,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 a1c288ab1f0..1087c08869e 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -1976,7 +1976,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? First you try so hard to remove
virtual methods, then you add a new one 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;
> @@ -3220,9 +3220,11 @@ class Item_literal: public Item_basic_constant
>    Item_literal(THD *thd): Item_basic_constant(thd)
>    { }
>    Type type() const override { return CONST_ITEM; }
> -  bool check_partition_func_processor(void *) override { return false;}
> +  bool check_partition_func_processor(void *int_arg) override { return false;}
>    bool const_item() const override { return true; }
>    bool basic_const_item() const override { return true; }
> +  bool is_expensive() override { return false; }
> +  bool cleanup_is_expensive_cache_processor(void *arg) override { return 0; }
>  };
>  
>  
> diff --git a/sql/opt_index_cond_pushdown.cc b/sql/opt_index_cond_pushdown.cc
> index 15bc2074e1f..e950ad1b7ca 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 119c7360f07..e63e120a02b 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -2395,6 +2395,7 @@ dispatch_command_return dispatch_command(enum enum_server_command command, 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 93f5d3591ed..894d511adf4 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -2364,7 +2364,7 @@ int JOIN::optimize_stage2()
>    if (!conds && outer_join)
>    {
>      /* Handle the case where we have an OUTER JOIN without a WHERE */
> -    conds= new (thd->mem_root) Item_bool(thd, true); // Always true
> +    conds= (Item*) &Item_true;

Hmm, does casting const away even work in the Intel compiler?

>    }
>  
>    if (impossible_where)
> @@ -22743,6 +22743,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;
>  
>    if (is_top_and_level && used_table == rand_table_bit &&
>        (cond->used_tables() & ~OUTER_REF_TABLE_BIT) != rand_table_bit)

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?

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