Re: bk commit into 5.0 tree (evgen:1.2500) BUG#24989

[Sergei Golubchik]

Hi!

On Jun 20, eugene@stripped wrote:
> ChangeSet@stripped, 2007-06-20 21:33:53+04:00, evgen@stripped +8 -0
>   Bug#24989: The DEADLOCK error is improperly handled by InnoDB.
>   
>   When innodb detects a deadlock it calls ha_rollback_trans() to rollback the 
>   main transaction. But such action isn't allowed from inside of triggers and
>   functions. When it happen the 'Explicit or implicit commit' error is thrown
>   even if there is no commit/rollback statements in the trigger/function. This
>   leads to the user confusion.
>   
> --- 1.233/sql/handler.cc      2007-05-07 23:12:13 +04:00
> +++ 1.234/sql/handler.cc      2007-06-20 21:23:12 +04:00
> @@ -858,6 +858,11 @@ int ha_autocommit_or_rollback(THD *thd, 
>        if (ha_commit_stmt(thd))
>       error=1;
>      }
> +    else if (thd->transaction_rollback_request && !thd->in_sub_stmt)
> +    {
> +      (void) ha_rollback(thd);
> +      thd->transaction_rollback_request= FALSE;

why not to reset thd->transaction_rollback_request in ha_rollback() ?
Looks like the most robust solution to me.

Now you do it in two places, and one could still wonder whether it's
enough.

> +    }
>      else
>        (void) ha_rollback_stmt(thd);
>  
> --- 1.21/mysql-test/t/innodb_mysql.test       2007-05-16 00:10:55 +04:00
> +++ 1.22/mysql-test/t/innodb_mysql.test       2007-06-20 19:48:33 +04:00
> @@ -597,4 +597,52 @@ EXPLAIN SELECT COUNT(*) FROM t2 WHERE st
>  
>  DROP TABLE t1,t2; 
>  
> +#
> +# Bug#24989: The DEADLOCK error is improperly handled by InnoDB.
> +#
> +CREATE TABLE t1 (f1 int NOT NULL) ENGINE=InnoDB;
> +CREATE TABLE t2 (f2 int(11) NOT NULL PRIMARY KEY AUTO_INCREMENT) ENGINE=InnoDB;
> +DELIMITER |;
> +CREATE TRIGGER t1_bi before INSERT
> +    ON t1 FOR EACH ROW
> +BEGIN
> +  DECLARE CONTINUE HANDLER FOR SQLSTATE '40001' SET @a:= 'deadlock';
> +  DECLARE CONTINUE HANDLER FOR SQLEXCEPTION SET @a:= 'exception';
> +  INSERT INTO t2 (f2) VALUES (1);
> +  DELETE FROM t2 WHERE f2 = 1;
> +END;|
> +DELIMITER ;|

There should be a test with the stored procedure, where a handler
catches the deadlock.

> +connect (con1,localhost,root,,);
> +connect (con2,localhost,root,,);
> +
> --- 1.313/sql/ha_innodb.cc    2007-04-29 17:40:41 +04:00
> +++ 1.314/sql/ha_innodb.cc    2007-06-20 21:24:53 +04:00
> @@ -451,25 +451,22 @@ convert_error_code_to_mysql(
>               return(-1); /* unspecified error */
>  
>       } else if (error == (int) DB_DEADLOCK) {
> -             /* Since we rolled back the whole transaction, we must
> -             tell it also to MySQL so that MySQL knows to empty the
> -             cached binlog for this transaction */
> -
> -             if (thd) {
> -                     ha_rollback(thd);
> -             }
> +                if (thd)
> +                {
> +                  if (thd->in_sub_stmt)
> +                    thd->is_fatal_sub_stmt_error= TRUE;
> +                  thd->transaction_rollback_request= TRUE;

No, we don't want a storage engine to access THD directly (this is only
true in 5.1, but you'd better to a correct fix in 5.0, otherwise you'd
have to fix 5.1 after the merge).

Make a function, say,

  void mark_transaction_to_rollback(THD *thd, bool all)
  {
    if (thd->in_sub_stmt)
      thd->is_fatal_sub_stmt_error= TRUE;
    thd->transaction_rollback_request= all;
  }

and use it here. 'all' means, as usual, whether to rollback a
transaction or only a statement.

> +                }
>  
>               return(HA_ERR_LOCK_DEADLOCK);
>  
>       } else if (error == (int) DB_LOCK_WAIT_TIMEOUT) {
> -
> -             /* Starting from 5.0.13, we let MySQL just roll back the
> -             latest SQL statement in a lock wait timeout. Previously, we
> -             rolled back the whole transaction. */
> -
> -             if (thd && row_rollback_on_timeout) {
> -                     ha_rollback(thd);
> -             }
> +                if (thd)
> +                {
> +                  if (thd->in_sub_stmt)
> +                    thd->is_fatal_sub_stmt_error= TRUE;
> +                  thd->transaction_rollback_request= TRUE;
> +                }
>  
>               return(HA_ERR_LOCK_WAIT_TIMEOUT);
>  
> @@ -517,14 +514,12 @@ convert_error_code_to_mysql(
>  
>               return(HA_ERR_NO_SAVEPOINT);
>       } else if (error == (int) DB_LOCK_TABLE_FULL) {
> -             /* Since we rolled back the whole transaction, we must
> -             tell it also to MySQL so that MySQL knows to empty the
> -             cached binlog for this transaction */
> -
> -             if (thd) {
> -                     ha_rollback(thd);
> -             }
> -
> +             if (thd)
> +                {
> +                  if (thd->in_sub_stmt)
> +                    thd->is_fatal_sub_stmt_error= TRUE;
> +                  thd->transaction_rollback_request= TRUE;
> +                }
>               return(HA_ERR_LOCK_TABLE_FULL);
>       } else {
>               return(-1);                     // Unknown error
> 
> --- 1.44/sql/sp_rcontext.cc   2006-12-23 22:04:26 +03:00
> +++ 1.45/sql/sp_rcontext.cc   2007-06-20 21:32:51 +04:00
> @@ -196,6 +196,9 @@ sp_rcontext::find_handler(uint sql_errno
>  {
>    if (m_hfound >= 0)
>      return 1;                        // Already got one
> +  /* Don't allow fatal sub statement errors to be caught */
> +  if (current_thd->is_fatal_sub_stmt_error)
> +    return FALSE;

I don't see where you check that you're in sub statement. If you're not,
the error should be possible to catch.

If you mean that not in sub statement is_fatal_sub_stmt_error won't even
be set, then I think you're resetting it in the wrong place.
  
>    const char *sqlstate= mysql_errno_to_sqlstate(sql_errno);
>    int i= m_hcount, found= -1;
> 
Regards / Mit vielen Grüssen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Developer
/_/  /_/\_, /___/\___\_\___/  MySQL GmbH, Radlkoferstr. 2, D-81373 München
       <___/                  Geschäftsführer: Kaj Arnö - HRB
München 162140