[Commits] b97452a6204: MDEV-19491 update query stopped working after mariadb upgrade 10.2.23 -> 10.2.24

Sergey Vojtovich svoj at mariadb.org
Thu May 30 13:51:59 EEST 2019


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 at mariadb.com>
> committer: Sergei Golubchik <serg at 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


More information about the commits mailing list