Hi, Igor!
Generally all looks good, but see my notes and couple of questions below.
On Thu, Jul 7, 2022 at 6:57 AM IgorBabaev
revision-id: 78370ea0fd21f66a4084d488ddcebac541249241 (mariadb-10.6.1-477-g78370ea) parent(s): 2db18fdb3d68d906fbd188ec570a64502ba55849 author: Igor Babaev committer: Igor Babaev timestamp: 2022-07-06 21:57:23 -0700 message:
MDEV-28965 Assertion failure when preparing UPDATE with derived table in WHERE
This patch fixes not only the assertion failure in the function Field_iterator_table_ref::set_field_iterator() but also: - fixes the problem of forced materialization of derived tables used in subqueries contained in WHERE clauses of single-table and multi-table UPDATE and DELETE statements - fixes the problem of MDEV-17954 that prevented execution of multi-table DELETE statements if they use in their WHERE clauses references to the tables that are updated.
The patch must be considered a complement to the patch for MDEV-28883.
--- mysql-test/main/delete_use_source.result | 184 ++++++++++- mysql-test/main/delete_use_source.test | 120 ++++++++ mysql-test/main/derived.result | 336 +++++++++++++++++++++ mysql-test/main/derived.test | 210 +++++++++++++ mysql-test/main/derived_cond_pushdown.result | 27 +- mysql-test/main/derived_cond_pushdown.test | 2 +- mysql-test/main/multi_update.result | 2 - mysql-test/main/multi_update.test | 2 - mysql-test/main/subselect.result | 1 - mysql-test/main/subselect.test | 1 - mysql-test/main/subselect_no_exists_to_in.result | 1 - mysql-test/main/subselect_no_mat.result | 1 - mysql-test/main/subselect_no_opts.result | 1 - mysql-test/main/subselect_no_scache.result | 1 - mysql-test/main/subselect_no_semijoin.result | 1 - .../engines/iuds/r/update_delete_number.result | 22 +- .../suite/engines/iuds/t/update_delete_number.test | 20 +- sql/opt_subselect.cc | 56 +++- sql/sql_base.cc | 41 ++- sql/sql_delete.cc | 63 ++-- sql/sql_delete.h | 14 +- sql/sql_lex.cc | 28 +- sql/sql_lex.h | 1 + sql/sql_update.cc | 21 +- sql/sql_update.h | 16 +- sql/sql_yacc.yy | 13 + sql/table.cc | 10 +- 27 files changed, 1106 insertions(+), 89 deletions(-)
[skip]
--- a/mysql-test/suite/engines/iuds/t/update_delete_number.test +++ b/mysql-test/suite/engines/iuds/t/update_delete_number.test @@ -285,8 +285,18 @@ SELECT * FROM t1,t2 WHERE t2.c1=t1.c2; DELETE FROM a1, a2 USING t1 AS a1 INNER JOIN t2 AS a2 WHERE a2.c1=a1.c2; --sorted_result SELECT * FROM t1,t2 WHERE t2.c1=t1.c2; ---error ER_UPDATE_TABLE_USED -DELETE FROM t1,t2 using t1,t2 where t1.c1=(select c1 from t1); +TRUNCATE TABLE t1; +TRUNCATE TABLE t2; +INSERT INTO t1 VALUES(254,127,1),(0,-128,2),(1,127,3),(3,NULL,5); +INSERT INTO t2 VALUES(127,255,1),(127,1,2),(-128,0,3),(-1,NULL,5); +# After the patch for MDEV-28883 this should not report +# --error ER_UPDATE_TABLE_USED anymore
Why is the above left as a comment? (the same in other cases) [skip]
diff --git a/sql/opt_subselect.cc b/sql/opt_subselect.cc index fa338f0..ac47566 100644 --- a/sql/opt_subselect.cc +++ b/sql/opt_subselect.cc @@ -30,6 +30,8 @@ #include "sql_base.h" #include "sql_const.h" #include "sql_select.h" +#include "sql_update.h" +#include "sql_delete.h"
it would be nice to have comment why you include it as it only for one function as I can see, something like: #include "sql_update.h" // processing_as_multitable_update_prohibited (the same for delete and for including in #include "sql_update.h" & "sql_base.cc") #include "filesort.h"
#include "opt_subselect.h" #include "sql_test.h" @@ -532,6 +534,48 @@ bool is_materialization_applicable(THD *thd, Item_in_subselect *in_subs, return FALSE; }
[skip]
diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 2b41b78..447573b 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -47,6 +47,8 @@ #include "sql_prepare.h" #include "sql_statistics.h" #include "sql_cte.h" +#include "sql_update.h" +#include "sql_delete.h"
see above
#include
#include #include @@ -1164,7 +1166,7 @@ TABLE_LIST* find_dup_table(THD *thd, TABLE_LIST *table, TABLE_LIST *table_list, /* If we found entry of this table or table of SELECT which already processed in derived table or top select of multi-update/multi-delete - (exclude_from_table_unique_test) or prelocking placeholder. + (exclude_from_table_unique_test) or prelocking placeholder.
Please fix formatting above
*/ DBUG_PRINT("info", ("found same copy of table or table which we should skip")); @@ -1175,16 +1177,43 @@ TABLE_LIST* find_dup_table(THD *thd, TABLE_LIST *table, TABLE_LIST *table_list, We come here for queries of type: INSERT INTO t1 (SELECT tmp.a FROM (select * FROM t1) as tmp);
- Try to fix by materializing the derived table + Try to fix by materializing the derived table if one can't do without it. */ TABLE_LIST *derived= res->belong_to_derived; if (derived->is_merged_derived() && !derived->derived->is_excluded()) { - DBUG_PRINT("info", + bool materialize= true; + if (thd->lex->sql_command == SQLCOM_UPDATE) + { + Sql_cmd_update *cmd= (Sql_cmd_update *) (thd->lex->m_sql_cmd); + if (cmd->is_multitable()) + materialize= false; + else if (!cmd->processing_as_multitable_update_prohibited(thd)) + { + cmd->set_as_multitable(); + materialize= false; + } + } + else if (thd->lex->sql_command == SQLCOM_DELETE) + { + Sql_cmd_delete *cmd= (Sql_cmd_delete *) (thd->lex->m_sql_cmd); + if (cmd->is_multitable()) + materialize= false; + if (!cmd->processing_as_multitable_delete_prohibited(thd)) + { + cmd->set_as_multitable(); + materialize= false; + } + } + if (materialize) + { + DBUG_PRINT("info", ("convert merged to materialization to resolve the conflict")); - derived->change_refs_to_fields(); - derived->set_materialized_derived(); - goto retry; + derived->change_refs_to_fields(); + derived->set_materialized_derived(); + // derived->field_translation= 0;
above comment has no big sens, looks not cleaned up some chenges?
+ goto retry; + } } } DBUG_RETURN(res);
[skip]
diff --git a/sql/sql_delete.h b/sql/sql_delete.h index e1d5044..ffb8173 100644 --- a/sql/sql_delete.h +++ b/sql/sql_delete.h @@ -44,11 +44,12 @@ class Sql_cmd_delete final : public Sql_cmd_dml { public: Sql_cmd_delete(bool multitable_arg) - : multitable(multitable_arg), save_protocol(NULL) {} + : orig_multitable(multitable_arg), save_protocol(NULL) + { multitable= orig_multitable; }
why do not use the same initializers? : multitable(multitable_arg), rig_multitable(multitable_arg), save_protocol(NULL) {}
enum_sql_command sql_command_code() const override { - return multitable ? SQLCOM_DELETE_MULTI : SQLCOM_DELETE; + return orig_multitable ? SQLCOM_DELETE_MULTI : SQLCOM_DELETE; }
DML_prelocking_strategy *get_dml_prelocking_strategy()
[skip]
diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 78c067a..4059c0e 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -37,6 +37,8 @@ #include "sql_partition.h" #include "sql_partition_admin.h" // Sql_cmd_alter_table_*_part #include "event_parse_data.h" +#include "sql_update.h" +#include "sql_delete.h"
see above
void LEX::parse_error(uint err_number) { @@ -4037,9 +4039,8 @@ bool LEX::can_use_merged() SYNOPSIS LEX::can_not_use_merged()
- @param no_update_or_delete Set to 1 if we can't use merge with multiple-table - updates, like when used from - TALE_LIST::init_derived() + @param forced_no_merge_for_update_delete Set to 1 if we can't use merge with + multiple-table updates/deletes
DESCRIPTION Temporary table algorithm will be used on all SELECT levels for queries @@ -4050,7 +4051,7 @@ bool LEX::can_use_merged() TRUE - VIEWs with MERGE algorithms can be used */
-bool LEX::can_not_use_merged(bool no_update_or_delete) +bool LEX::can_not_use_merged(bool forced_no_merge_for_update_delete) { switch (sql_command) { case SQLCOM_CREATE_VIEW: @@ -4064,18 +4065,29 @@ bool LEX::can_not_use_merged(bool no_update_or_delete) return TRUE;
case SQLCOM_UPDATE_MULTI: - case SQLCOM_DELETE_MULTI: - if (no_update_or_delete) + if (forced_no_merge_for_update_delete) return TRUE; /* Fall through */
case SQLCOM_UPDATE: - if (no_update_or_delete && m_sql_cmd && - (m_sql_cmd->sql_command_code() == SQLCOM_UPDATE_MULTI || + if (forced_no_merge_for_update_delete && + (((Sql_cmd_update *) m_sql_cmd)->is_multitable() || query_tables->is_multitable())) return TRUE; + return FALSE; + + case SQLCOM_DELETE_MULTI: + if (forced_no_merge_for_update_delete) + return TRUE; /* Fall through */
+ case SQLCOM_DELETE: + if (forced_no_merge_for_update_delete && + (((Sql_cmd_delete *) m_sql_cmd)->is_multitable() || + query_tables->is_multitable())) + return TRUE; + return FALSE; + default: return FALSE; } diff --git a/sql/sql_lex.h b/sql/sql_lex.h index e733b4f..402e3bd 100644
[skip]
diff --git a/sql/sql_update.cc b/sql/sql_update.cc index 6b14c4f..7769777 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -2807,6 +2807,23 @@ bool multi_update::send_eof()
/** + @brief Check whether conversion to multi-table update is prohibited + + @param thd global context the processed statement + @returns true if conversion is prohibited, false otherwise + + @todo + Introduce handler level flag for storage engines that would prohibit + such conversion for any single-table update. +*/ + +bool Sql_cmd_update::processing_as_multitable_update_prohibited(THD *thd) +{ + return false; +} + + +/** @brief Perform precheck of table privileges for update statements
@param thd global context the processed statement @@ -2889,7 +2906,9 @@ bool Sql_cmd_update::prepare_inner(THD *thd) "updating and querying the same temporal periods table"); DBUG_RETURN(TRUE); } - multitable= true; + if (!table_list->is_multitable() && + !processing_as_multitable_update_prohibited(thd)) + multitable= true; } }
diff --git a/sql/sql_update.h b/sql/sql_update.h index d0fc7cb..4aff77a 100644 --- a/sql/sql_update.h +++ b/sql/sql_update.h @@ -46,12 +46,12 @@ class Sql_cmd_update final : public Sql_cmd_dml { public: Sql_cmd_update(bool multitable_arg) - : multitable(multitable_arg) - { } + : orig_multitable(multitable_arg) + { multitable= orig_multitable; }
why do not use construction for both? Sql_cmd_update(bool multitable_arg) : multitable(multitable_arg) : orig_multitable(multitable_arg) {}
enum_sql_command sql_command_code() const override { - return multitable ? SQLCOM_UPDATE_MULTI : SQLCOM_UPDATE; + return orig_multitable ? SQLCOM_UPDATE_MULTI : SQLCOM_UPDATE; }
DML_prelocking_strategy *get_dml_prelocking_strategy() @@ -59,6 +59,12 @@ class Sql_cmd_update final : public Sql_cmd_dml return &multiupdate_prelocking_strategy; }
+ bool processing_as_multitable_update_prohibited(THD *thd); + + bool is_multitable() { return multitable; } + + void set_as_multitable() { multitable= true; } + protected: /** @brief Perform precheck of table privileges for update statements @@ -89,13 +95,15 @@ class Sql_cmd_update final : public Sql_cmd_dml */ bool multitable;
+ /* Original value of the 'multitable' flag set by constructor */ + const bool orig_multitable; + /* The prelocking strategy used when opening the used tables */ Multiupdate_prelocking_strategy multiupdate_prelocking_strategy;
public: /* The list of the updating expressions used in the set clause */ List<Item> *update_value_list; - };
#endif /* SQL_UPDATE_INCLUDED */ diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index a587a37..13dc602 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -13370,8 +13370,21 @@ delete_single_table: YYPS->m_lock_type, YYPS->m_mdl_type, NULL, + 0))) + MYSQL_YYABORT; + Select->table_list.save_and_clear(&Lex->auxiliary_table_list); + Lex->table_count= 1;
Why it is not enough to have this counter (table_count) set by open_tables? May be it would be better to use mysql_init_select() to set everything needed by SELECT for sure?
+ Lex->query_tables= 0; + Lex->query_tables_last= &Lex->query_tables; + if (unlikely(!Select-> + add_table_to_list(thd, $2, NULL, TL_OPTION_UPDATING, + YYPS->m_lock_type, + YYPS->m_mdl_type, + NULL, $3))) MYSQL_YYABORT; + Lex->auxiliary_table_list.first->correspondent_table= + Lex->query_tables; YYPS->m_lock_type= TL_READ_DEFAULT; YYPS->m_mdl_type= MDL_SHARED_READ; } [skip]