Generally all looks good, but see my notes and couple of questions below.
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 <m_ctype.h>
#include <my_dir.h>
#include <hash.h>
@@ -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]