Re: [Commits] b97452a6204: MDEV-19491 update query stopped working after mariadb upgrade 10.2.23 -> 10.2.24
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
Hi, Sergey! On May 30, Sergey Vojtovich wrote:
+# +# 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?
More. open_tables() starts from has_prelocking_list= thd->lex->requires_prelocking(); when open_tables() was called twice, the first value of has_prelocking_list was lost. Later when triggers were opened in the second open_tables() call, routines were not opened, because has_prelocking_list on the second invocation was true. This could've been fixed by passing has_prelocking_list as an argument to open_tables(). But by looking at this code I've noticed the backoff issue, so I took a different approach, that fixed both bugs.
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().
handle_xxx is called when something happens, to "handle" it. "handle_end" means to handle the end of the one pass over the table list. Say, the table list has tables t1, t2, t3. handle_table is called for each. Then routines are opened, they may add t4 and t5 to the list. But after handling t1, t2, t3 handle_end is called. Then t4 and t5 are opened, handle_table is called for them, then handle_end again.
Also end() sounds multi-update specific. Isn't it "next nesting level dive"? May be level_end()? Or next_level()?
It's not really a level, a table list is a linked list, not a hierarchy. one_pass_end() ? no, not good either. I don't have a good name, sorry.
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?
I've spent a lot of time trying to do it in handle_table(). The idea was to get the last handle_table() call to do the job. That's why the whole interface is a Prelocking_strategy, may be it wouldn't have been, if it hadn't started from handle_table(). The problem was that it's fairly difficult to detect the last handle_table() call. It's the call for the last updatable table which isn't a view, and isn't information_schema table. Then there was some issue with routines that are opened after tables, and it was too late. I was fixing all those problems but finally I've basically given up. I'm not sure anymore what the last problem was, probably the one with information_schema tables (handle_table() isn't called for them).
...skip...
The rest is fixed. Thanks! Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Sergei, On Thu, May 30, 2019 at 09:03:24PM +0200, Sergei Golubchik wrote:
Hi, Sergey!
On May 30, Sergey Vojtovich wrote:
+# +# 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?
More.
open_tables() starts from
has_prelocking_list= thd->lex->requires_prelocking();
when open_tables() was called twice, the first value of has_prelocking_list was lost. Later when triggers were opened in the second open_tables() call, routines were not opened, because has_prelocking_list on the second invocation was true.
This could've been fixed by passing has_prelocking_list as an argument to open_tables().
But by looking at this code I've noticed the backoff issue, so I took a different approach, that fixed both bugs. Ok, it explains why you added Multiupdate_prelocking_strategy::has_prelocking_list.
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().
handle_xxx is called when something happens, to "handle" it. "handle_end" means to handle the end of the one pass over the table list. Say, the table list has tables t1, t2, t3. handle_table is called for each. Then routines are opened, they may add t4 and t5 to the list. But after handling t1, t2, t3 handle_end is called. Then t4 and t5 are opened, handle_table is called for them, then handle_end again.
Ok, It is probably just me not being a fan of handle/handler/handlerton.
Also end() sounds multi-update specific. Isn't it "next nesting level dive"? May be level_end()? Or next_level()?
It's not really a level, a table list is a linked list, not a hierarchy. one_pass_end() ? no, not good either. I don't have a good name, sorry.
Sigh.
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?
I've spent a lot of time trying to do it in handle_table(). The idea was to get the last handle_table() call to do the job. That's why the whole interface is a Prelocking_strategy, may be it wouldn't have been, if it hadn't started from handle_table().
The problem was that it's fairly difficult to detect the last handle_table() call. It's the call for the last updatable table which isn't a view, and isn't information_schema table. Then there was some issue with routines that are opened after tables, and it was too late. I was fixing all those problems but finally I've basically given up. I'm not sure anymore what the last problem was, probably the one with information_schema tables (handle_table() isn't called for them).
I think it implies that we have to open all query tables before evalutation. My second question was: do we actually have to do it this way?
...skip...
The rest is fixed. Thanks!
One question re is_stmt_prepare() is not answered. Ok to push anyway. Regards, Sergey
Hi, Sergey! On May 30, Sergey Vojtovich wrote:
The problem was that it's fairly difficult to detect the last handle_table() call. It's the call for the last updatable table which isn't a view, and isn't information_schema table. Then there was some issue with routines that are opened after tables, and it was too late. I was fixing all those problems but finally I've basically given up. I'm not sure anymore what the last problem was, probably the one with information_schema tables (handle_table() isn't called for them).
I think it implies that we have to open all query tables before evalutation. My second question was: do we actually have to do it this way?
I'm not quite sure what part of the original mysql_multi_update_prepare() really has to be done in handle_end(). Obviously all tables explicitly mentioned in the statement has to be opened and their fields has to be fixed. That's the whole point - we need to know what table the field in `SET a=5` belongs to. To what extent views and derived tables has to be prepared - I cannot say. It looks like we have to merge all mergeable views and derived for the same reason as above, to find what table the field `a` belongs to. Clearly, any table not mentioned in a statement (nor in any views) doesn't have to be opened - tables coming via triggers or stored routines cannot be updated.
One question re is_stmt_prepare() is not answered.
Sorry. Yes, this branch is definitely used with is_stmt_prepare() == true. Any prepare of a multi-update does it. Just to be sure, I've added an assert there and it immediately crashed in multi_update test on UPDATE t1 t1 left join v3 t2 on t1.f4 = t2.fe2 SET t1.f20 = t2.ft6_1, t1.f32 = t2.f32, t1.f33 = t2.f33, t1.f37 = t2.f37 WHERE f5 >= '2015-02-01' Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich