Hi Sergei, On Fri, May 24, 2019 at 06:06:43PM +0200, Sergei Golubchik wrote:
revision-id: b97452a6204 (mariadb-5.5.64-11-gb97452a6204) parent(s): 7fceef405ae author: Sergei Golubchik <serg@mariadb.com> committer: Sergei Golubchik <serg@mariadb.com> timestamp: 2019-05-24 18:00:34 +0200 message:
MDEV-19491 update query stopped working after mariadb upgrade 10.2.23 -> 10.2.24 ...skip...
diff --git a/mysql-test/t/multi_update.test b/mysql-test/t/multi_update.test index 14c5574f61c..a3b64ffde8c 100644 --- a/mysql-test/t/multi_update.test +++ b/mysql-test/t/multi_update.test @@ -1081,6 +1081,18 @@ select * from t2; drop table t1,t2, t3; drop user foo;
+# +# MDEV-19521 Update Table Fails with Trigger and Stored Function +# +create table t1 (a int, b varchar(50), c varchar(50)); +insert t1 (a,b) values (1,'1'), (2,'2'), (3,'3'); +create function f1() returns varchar(50) return 'result'; +create trigger tr before update on t1 for each row set new.c = (select f1()); +create table t2 select a, b from t1; +update t1 join t2 using (a) set t1.b = t2.b; +drop table t1, t2; +drop function f1; I don't completely understand why this test was failing. It doesn't request backoff action, right? Or is this patch fixing more than just backoff retry?
diff --git a/mysql-test/t/multi_update_debug.test b/mysql-test/t/multi_update_debug.test new file mode 100644 index 00000000000..ccd7791f029 --- /dev/null +++ b/mysql-test/t/multi_update_debug.test @@ -0,0 +1,30 @@ +# +# test MDL backoff-and-retry during multi-update +# +source include/have_debug_sync.inc; +create table t1 (a int, b int); +create table t2 (c int, d int); +insert t1 values (1,2),(3,4); +insert t2 values (5,6),(7,8); +create table t0 (x int); +insert t0 values (11), (22); +create trigger tr1 before update on t1 for each row insert t0 values (new.b); + +set debug_sync='open_tables_after_open_and_process_table WAIT_FOR cont'; +send update t1 join t2 on (a=c+4) set b=d; + +connect con1, localhost, root; You should wait here until update reaches open_tables_after_open_and_process_table.
Also you should be able to do something like set debug_sync='mdl_acquire_lock_wait SIGNAL cont'; So that you don't need con2 at all. ...skip...
diff --git a/sql/sql_base.h b/sql/sql_base.h index ea92e880db7..439052a28f5 100644 --- a/sql/sql_base.h +++ b/sql/sql_base.h @@ -426,6 +426,7 @@ class Prelocking_strategy public: virtual ~Prelocking_strategy() { }
+ virtual void reset(THD *thd) { }; virtual bool handle_routine(THD *thd, Query_tables_list *prelocking_ctx, Sroutine_hash_entry *rt, sp_head *sp, bool *need_prelocking) = 0; @@ -433,6 +434,7 @@ class Prelocking_strategy TABLE_LIST *table_list, bool *need_prelocking) = 0; virtual bool handle_view(THD *thd, Query_tables_list *prelocking_ctx, TABLE_LIST *table_list, bool *need_prelocking)= 0; + virtual bool handle_end(THD *thd) { return 0; }; }; May be just end()? It looks inconsistent with reset(). Also end() sounds multi-update specific. Isn't it "next nesting level dive"? May be level_end()? Or next_level()?
I wonder if handle_table() alone can handle this? Or do we need to open all tables before we can tell which ones are go ing to be updated? ...skip...
+int mysql_multi_update_prepare(THD *thd) +{ + LEX *lex= thd->lex; + TABLE_LIST *table_list= lex->query_tables; + TABLE_LIST *tl; + Multiupdate_prelocking_strategy prelocking_strategy; + uint table_count= lex->table_count; Bad spacing.
+ bool original_multiupdate= (thd->lex->sql_command == SQLCOM_UPDATE_MULTI); Do you really need this variable?
+ DBUG_ENTER("mysql_multi_update_prepare"); + + /* + Open tables and create derived ones, but do not lock and fill them yet. + + During prepare phase acquire only S metadata locks instead of SW locks to + keep prepare of multi-UPDATE compatible with concurrent LOCK TABLES WRITE + and global read lock. + */ + if (original_multiupdate) + { + if (open_tables(thd, &table_list, &table_count, + thd->stmt_arena->is_stmt_prepare() ? MYSQL_OPEN_FORCE_SHARED_MDL : 0, + &prelocking_strategy)) + DBUG_RETURN(TRUE); Wrong indentation. Do we ever enter this branch with is_stmt_prepare()?
+ } + else + { + /* following need for prepared statements, to run next time multi-update */ + thd->lex->sql_command= SQLCOM_UPDATE_MULTI; + prelocking_strategy.reset(thd); + if (prelocking_strategy.handle_end(thd)) DBUG_RETURN(TRUE); }
In 10.2 we got MDL_SHARED_READ_ONLY, which is acquired by LOCK TABLES ... READ for InnoDB. I guess it is going to be incompatible with multi update tables opened for reading. I don't see this approach making fix for this issue impossible, but still worth to double check. Looks good otherwise. Regards, Sergey