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