Hi Serg, Please find some cosmetic input below. (additionally, we've found a failing example while chatting) On Tue, Apr 10, 2018 at 02:10:17PM +0200, serg@mariadb.org wrote:
revision-id: a74e3ef17e7a8693d68001290a8d91b5b206804d (mariadb-10.3.5-138-ga74e3ef17e7) parent(s): 9bd3af97dffa411657879bbdfd4dfef38c99f7cc author: Sergei Golubchik committer: Sergei Golubchik timestamp: 2018-04-10 14:09:17 +0200 message:
MDEV-14551 Can't find record in table on multi-table update with ORDER BY
...
diff --git a/mysql-test/main/multi_update.test b/mysql-test/main/multi_update.test index 5feebe87a5a..96e78dab82b 100644 --- a/mysql-test/main/multi_update.test +++ b/mysql-test/main/multi_update.test @@ -914,3 +914,14 @@ update t1 set c1=NULL; update t1, t2 set t1.c1=t2.c3 where t1.c3=t2.c3 order by t1.c3 desc limit 2; select * from t1; drop table t1, t2; + +# +# MDEV-14551 Can't find record in table on multi-table update with ORDER BY +# +CREATE TABLE t1 (i INT) ENGINE=MEMORY; +INSERT t1 VALUES (1),(2); +CREATE TABLE t2 (f INT) ENGINE=MyISAM; +INSERT t2 VALUES (1),(2); +UPDATE t1, t2 SET f = 126 ORDER BY f LIMIT 2; +SELECT * FROM t2; +DROP TABLE t1, t2; diff --git a/sql/item.h b/sql/item.h index 9574bdc63bf..e391e7810c4 100644 --- a/sql/item.h +++ b/sql/item.h @@ -2023,6 +2023,7 @@ class Item: public Value_source, { marker &= ~EXTRACTION_MASK; } + virtual TABLE *rowid_table() const { return 0; }
I am concerned about this approach bloating class Item's vtable. There is (and likely will ever be) only one class with a different implementation, the check for this is made only in one place - why not check Item's type() or functype(), etc?
};
MEM_ROOT *get_thd_memroot(THD *thd); diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc index c6b0ae7fba8..9097ffca867 100644 --- a/sql/item_strfunc.cc +++ b/sql/item_strfunc.cc @@ -5204,3 +5204,23 @@ String *Item_func_dyncol_list::val_str(String *str) my_free(names); return NULL; } + +Item_temptable_rowid::Item_temptable_rowid(TABLE *table_arg) + : Item_str_func(table_arg->in_use), table(table_arg) +{ + max_length= table->file->ref_length; +} + +void Item_temptable_rowid::fix_length_and_dec() +{ + used_tables_cache= table->map; + const_item_cache= false; +} + +String *Item_temptable_rowid::val_str(String *str) +{ + if (!((null_value= table->null_row))) + table->file->position(table->record[0]); + str_value.set((char*)(table->file->ref), max_length, &my_charset_bin); + return &str_value; +} diff --git a/sql/item_strfunc.h b/sql/item_strfunc.h index 18cda491efd..49de5568696 100644 --- a/sql/item_strfunc.h +++ b/sql/item_strfunc.h @@ -1748,5 +1748,20 @@ class Item_func_dyncol_list: public Item_str_func { return get_item_copy<Item_func_dyncol_list>(thd, this); } };
-#endif /* ITEM_STRFUNC_INCLUDED */
Please add a note that this is not the "_rowid" that we support in the parser.
+class Item_temptable_rowid :public Item_str_func +{ + TABLE *table; +public: + Item_temptable_rowid(TABLE *table_arg); + const Type_handler *type_handler() const { return &type_handler_string; } + Field *create_tmp_field(bool group, TABLE *table) + { return create_table_field_from_handler(table); } + String *val_str(String *str); + const char *func_name() const { return "<rowid>"; } + void fix_length_and_dec(); + Item *get_copy(THD *thd) + { return get_item_copy<Item_temptable_rowid>(thd, this); } + TABLE *rowid_table() const { return table; } +};
+#endif /* ITEM_STRFUNC_INCLUDED */ ...
diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 796ea569e64..22301319680 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -2650,6 +2650,21 @@ bool JOIN::add_having_as_table_cond(JOIN_TAB *tab) }
+bool JOIN::add_fields_for_current_rowid(JOIN_TAB *cur, List<Item> *table_fields)
I think ths is actually "current rowids", in plural. As the loop shows, there are multiple current rowids (from different tables).
+{ + for (JOIN_TAB *tab=join_tab; tab < cur; tab++)
Please add a comment: this will not walk into semi-join materialization nests but this is ok because we will never need to save current rowids for those.
+ { + if (!tab->keep_current_rowid) + continue; + Item *item= new (thd->mem_root) Item_temptable_rowid(tab->table); + item->fix_fields(thd, 0); + table_fields->push_back(item, thd->mem_root); + cur->tmp_table_param->func_count++; + } + return 0; +} + + /** Set info for aggregation tables
diff --git a/sql/sql_select.h b/sql/sql_select.h index f8911fbba01..1da87bb9d50 100644 --- a/sql/sql_select.h +++ b/sql/sql_select.h @@ -1438,6 +1438,9 @@ class JOIN :public Sql_alloc
enum { QEP_NOT_PRESENT_YET, QEP_AVAILABLE, QEP_DELETED} have_query_plan;
+ // if keep_current_rowid=true, whether they should be saved in temporary table + bool tmp_table_keep_current_rowid; Same input as with add_fields_for_current_rowid - use plural.
+ /* Additional WHERE and HAVING predicates to be considered for IN=>EXISTS subquery transformation of a JOIN object.
diff --git a/sql/sql_union.cc b/sql/sql_union.cc index 0149c2848c2..0e29f817e56 100644 --- a/sql/sql_union.cc +++ b/sql/sql_union.cc @@ -507,14 +507,14 @@ void select_union_recursive::cleanup() bool select_union_direct::change_result(select_result *new_result) { result= new_result; - return (result->prepare(unit->types, unit) || result->prepare2()); + return (result->prepare(unit->types, unit) || result->prepare2(0)); Do we really continue to use '0' when 'NULL' should be used? I prefer "NULL".
}
bool select_union_direct::postponed_prepare(List<Item> &types) { if (result != NULL) - return (result->prepare(types, unit) || result->prepare2()); + return (result->prepare(types, unit) || result->prepare2(0)); else return false; } diff --git a/sql/sql_update.cc b/sql/sql_update.cc index 38638d3aa1d..4ae7e33b4e3 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -2205,10 +2195,41 @@ multi_update::initialize_tables(JOIN *join) DBUG_RETURN(1); tmp_tables[cnt]->file->extra(HA_EXTRA_WRITE_CACHE); } + join->tmp_table_keep_current_rowid= TRUE; DBUG_RETURN(0); }
(Follwing explanations on Slack) Please add a comment there, saying that Here we walk through the aggegation temptable to find where rowids are stored. And then for each of the multi-update tables, we get them to copy the data from the rowid field in the aggregation temptable.
+ +int multi_update::prepare2(JOIN *join) +{ + if (!join->need_tmp || !join->tmp_table_keep_current_rowid) + return 0; + + // there cannot be many tmp tables in multi-update + JOIN_TAB *tmptab= join->join_tab + join->exec_join_tab_cnt(); + + for (Item **it= tmptab->tmp_table_param->items_to_copy; *it ; it++) + { + TABLE *tbl= (*it)->rowid_table(); + if (!tbl) + continue; + for (uint i= 0; i < table_count; i++) + { + for (Item **it2= tmp_table_param[i].items_to_copy; *it2; it2++) + { + if ((*it2)->rowid_table() != tbl) + continue; + *it2= new (thd->mem_root) Item_field(thd, (*it)->get_tmp_table_field()); + if (!*it2) + return 1; + } + } + } + return 0; +} + + multi_update::~multi_update() { TABLE_LIST *table;
BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog