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@gmail.com> committer: Michael Widenius <michael.widenius@gmail.com> 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@mariadb.org