maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #13178
Re: [Commits] 78370ea: MDEV-28965 Assertion failure when preparing UPDATE with derived table in WHERE
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 <igor@xxxxxxxxxxx> wrote:
> 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]
