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

[Sergei Golubchik]

Hi!

On Jul 04, eugene@stripped wrote:
> ChangeSet@stripped, 2007-07-04 23:23:58+04:00, evgen@stripped +9 -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.621/sql/sql_parse.cc	2007-05-15 13:56:04 +04:00
> +++ 1.622/sql/sql_parse.cc	2007-07-04 22:52:54 +04:00
> @@ -5107,6 +5107,8 @@ create_sp_error:
>    if (thd->one_shot_set && lex->sql_command != SQLCOM_SET_OPTION)
>      reset_one_shot_variables(thd);
>  
> +  if (!thd->spcont)
> +    thd->is_fatal_sub_stmt_error= FALSE;

No, this is wrong.
Why to check thd->spcont after every statement ?
It can never change here. It changes inside the statement and is
restored immediately after executing the routine - in
sp_head::execute_function and sp_head::execute_trigger.

Also, note that it's also set and reset in sp_head::execute_procedure,
but in the precedure we're *not* in the sub-statement, even if
thd->spcont is set.

You should not need to check thd->spcont. The flag you want to reset is
about *sub_stmt*, so you need to check thd->in_sub_stmt. And do it where
it's changed (THD::restore_sub_statement_state), not after every statement.

>    /*
>      The return value for ROW_COUNT() is "implementation dependent" if the
>      statement is not DELETE, INSERT or UPDATE, but -1 is what JDBC and ODBC
> @@ -5812,6 +5814,7 @@ void mysql_reset_thd_for_next_command(TH
>    thd->query_start_used= thd->insert_id_used=0;
>    thd->last_insert_id_used_bin_log= FALSE;
>    thd->is_fatal_error= thd->time_zone_used= 0;
> +  thd->transaction_rollback_request= FALSE;

Do you need that line ? You reset thd->transaction_rollback_request
in ha_rollback_trans(), it should be enough.

>    thd->server_status&= ~ (SERVER_MORE_RESULTS_EXISTS | 
> --- 1.21/mysql-test/r/innodb_mysql.result	2007-05-16 00:10:55 +04:00
> +++ 1.22/mysql-test/r/innodb_mysql.result	2007-07-04 22:50:58 +04:00
> @@ -487,7 +487,6 @@ ERROR HY000: Lock wait timeout exceeded;
>  select * from t1;
>  a
>  1
> -5

Wrong. Why would this test result change ?

>  commit;
>  select * from t1;
>  a
> --- 1.313/sql/ha_innodb.cc	2007-04-29 17:40:41 +04:00
> +++ 1.314/sql/ha_innodb.cc	2007-07-04 23:02:38 +04:00
> @@ -451,25 +451,12 @@ 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 */

don't remove the comment, it's still relevant.

> -
> - 		if (thd) {
> - 			ha_rollback(thd);
> - 		}
> +                mark_transaction_to_rollback(thd, TRUE);
>      		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);
> -		}
> +                mark_transaction_to_rollback(thd, TRUE);

No, you've undone the feature - row_rollback_on_timeout.
You should have transaction rollback if row_rollback_on_timeout==true,
and statement rollback otherwise. See what the comment above says.
You could do it as

         mark_transaction_to_rollback(thd, row_rollback_on_timeout);

And don't remove the comment, it's still relevant.
>  
>     		return(HA_ERR_LOCK_WAIT_TIMEOUT);
>  
> @@ -517,13 +504,7 @@ 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 */

Don't remove the comment, it's still relevant.

> -
> - 		if (thd) {
> - 			ha_rollback(thd);
> - 		}
> +                mark_transaction_to_rollback(thd, TRUE);
>  
>      		return(HA_ERR_LOCK_TABLE_FULL);
>      	} else {
> 
> --- 1.246/sql/sp_head.cc	2007-05-16 09:51:57 +04:00
> +++ 1.247/sql/sp_head.cc	2007-07-04 22:58:46 +04:00
> @@ -1070,6 +1070,13 @@ sp_head::execute(THD *thd)
>      thd->cleanup_after_query();
>      free_root(&execute_mem_root, MYF(0));    
>  
> +    if (!err_status && thd->is_fatal_sub_stmt_error)
> +    {
> +      /* Restore error flags after continue handler. */
> +      err_status= TRUE;
> +      thd->net.report_error= TRUE;
> +      break;
> +    }
>      /*
>        Check if an exception has occurred and a handler has been found
>        Note: We have to check even if err_status == FALSE, since warnings (and
> @@ -1090,13 +1097,22 @@ sp_head::execute(THD *thd)
>          ctx->push_hstack(i->get_cont_dest());
>          // Fall through
>        default:
> +        /*
> +          To allow continue handler to catch fatal errors reset error flag
> +          but don't clear the error itself, we will need it to report to
> +          a user.

eh, why ?
If there was a handler, we don't report an error to the user.

> +        */
>          ip= hip;
> -        err_status= FALSE;
>          ctx->clear_handler();
>          ctx->enter_handler(hip);
> +        err_status= FALSE;
> +        thd->net.report_error= 0;
> +        if (!thd->is_fatal_sub_stmt_error)
> +        {
>          thd->clear_error();
>          thd->killed= THD::NOT_KILLED;
>          thd->mysys_var->abort= 0;
> +        }
>          continue;
>        }
>      }
> 
> --- 1.44/sql/sp_rcontext.cc	2006-12-23 22:04:26 +03:00
> +++ 1.45/sql/sp_rcontext.cc	2007-07-04 22:56:14 +04:00
> @@ -194,8 +194,14 @@ bool
>  sp_rcontext::find_handler(uint sql_errno,
>                            MYSQL_ERROR::enum_warning_level level)
>  {
> +  THD *thd;
>    if (m_hfound >= 0)
>      return 1;			// Already got one
> +  thd= current_thd;

don't use current_thd, pass THD as an argument.

> +  /* Don't allow fatal sub statement errors to be caught in a sub statement */
> +  if (thd->is_fatal_sub_stmt_error &&
> +      thd->spcont->sp->m_type != TYPE_ENUM_PROCEDURE)

oh, my.
it's is_fatal_sub_stmt_error, why do you invent complex conditions
instead of writing

       thd->is_fatal_sub_stmt_error && thd->in_sub_stmt

> +    return FALSE;
>  
>    const char *sqlstate= mysql_errno_to_sqlstate(sql_errno);
>    int i= m_hcount, found= -1;
> @@ -313,7 +319,8 @@ sp_rcontext::handle_error(uint sql_errno
>        */
>        thd->net.report_error= 1;
>      }
> -    handled= TRUE;
> +    if (!thd->is_fatal_sub_stmt_error)
> +      handled= TRUE;

why is that ?

>    }
>  
>    return handled;
 
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