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

[Konstantin Osipov]

* eugene@stripped <eugene@stripped> [07/07/18 17:40]:
> +  /*
> +    Don't allow fatal sub statement errors to be caught in a sub statement.
> +    Appropriate context to handle such errors would be a most inner procedure
> +    which isn't called directly or indirectly from any function/trigger.
> +  */
> +  if (thd->is_fatal_sub_stmt_error && thd->in_sub_stmt)
> +  {
> +    bool ret= FALSE;
> +    sp_rcontext *found_ctx= 0, *ctx= this;
> +    while ((ctx= ctx->m_prev_runtime_ctx))
> +    {
> +      if (!ctx->sp)
> +        continue;
> +      if (ctx->sp->m_type == TYPE_ENUM_PROCEDURE)
> +      {
> +        if (!found_ctx)
> +          found_ctx= ctx;
> +      }
> +      else
> +        found_ctx= 0;
> +    }
> +    if (found_ctx)
> +    {
> +      thd->in_sub_stmt= FALSE;
> +      ret= found_ctx->find_handler(thd, sql_errno, level);
> +      thd->in_sub_stmt= TRUE;
> +    }
> +    return ret;
> +  }

This is incorrect.

I applied your patch, and implemented all the review comments I
had.

The new patch (attached) should be OK to push.

However, the test case that goes with your patch contains
unnecessary sleeps (via get_lock() which simply time outs) and races.

I've spent quite a bit of time trying to eliminate these issues,
but haven't succeeded. In particular, I see no way to eliminate
all races -- I guess the only option is to add the test case to
innodb_mysql-big.test and run only with --big-test option.
I will continue looking at the test case tomorrow.

I have the following comments on the patch:

* comments are not in Doxygen format (fixed) 
* missing comments for new variables (fixed).
* incorrect implementation of the search for the current handler:
  find_handler did not account for the situation when a procedure
  is called from a stored function or trigger:
  f1()
    p1()
      f2()
  With this call stack find_handler would find the continue
  handler of p1 (procedure) while p1() is still part of a
  sub-statement started by f1().
  (fixed)
* no test case for the above (todo)
* redundant select get_lock/release_lock() in the test
  (todo: remove)
* test results are not updated to show results from the last
  change (todo)

Suggestions for the future: 
* Doxygen is our commenting style. If the patch doesn't follow
  the coding style, I shouldn't even look at it.
* please discuss first, then write code. It should not be hard to
  catch me on IRC - after all, you managed to remind me daily
  that you're waiting for a review

--cut: 24989.diff
===== sql/ha_innodb.cc 1.315 vs edited =====
--- 1.315/sql/ha_innodb.cc	2007-07-10 18:16:49 +04:00
+++ edited/sql/ha_innodb.cc	2007-07-26 17:39:25 +04:00
@@ -455,9 +455,7 @@ convert_error_code_to_mysql(
  		tell it also to MySQL so that MySQL knows to empty the
  		cached binlog for this transaction */
 
- 		if (thd) {
- 			ha_rollback(thd);
- 		}
+                mark_transaction_to_rollback(thd, TRUE);
 
     		return(HA_ERR_LOCK_DEADLOCK);
 
@@ -467,9 +465,7 @@ convert_error_code_to_mysql(
 		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, row_rollback_on_timeout);
 
    		return(HA_ERR_LOCK_WAIT_TIMEOUT);
 
@@ -521,9 +517,7 @@ convert_error_code_to_mysql(
  		tell it also to MySQL so that MySQL knows to empty the
  		cached binlog for this transaction */
 
- 		if (thd) {
- 			ha_rollback(thd);
- 		}
+                mark_transaction_to_rollback(thd, TRUE);
 
     		return(HA_ERR_LOCK_TABLE_FULL);
     	} else {
===== sql/handler.cc 1.235 vs edited =====
--- 1.235/sql/handler.cc	2007-06-20 12:46:11 +04:00
+++ edited/sql/handler.cc	2007-07-26 17:39:03 +04:00
@@ -821,6 +821,9 @@ int ha_rollback_trans(THD *thd, bool all
     }
   }
 #endif /* USING_TRANSACTIONS */
+  if (all)
+    thd->transaction_rollback_request= FALSE;
+
   /*
     If a non-transactional table was updated, warn; don't warn if this is a
     slave thread (because when a slave thread executes a ROLLBACK, it has
@@ -858,6 +861,8 @@ 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);
     else
       (void) ha_rollback_stmt(thd);
 
===== sql/sp_rcontext.cc 1.44 vs edited =====
--- 1.44/sql/sp_rcontext.cc	2006-12-23 22:04:26 +03:00
+++ edited/sql/sp_rcontext.cc	2007-07-26 18:44:40 +04:00
@@ -37,6 +37,7 @@ sp_rcontext::sp_rcontext(sp_pcontext *ro
    m_var_items(0),
    m_return_value_fld(return_value_fld),
    m_return_value_set(FALSE),
+   in_sub_stmt(FALSE),
    m_hcount(0),
    m_hsp(0),
    m_ihsp(0),
@@ -67,6 +68,8 @@ sp_rcontext::~sp_rcontext()
 
 bool sp_rcontext::init(THD *thd)
 {
+  in_sub_stmt= thd->in_sub_stmt;
+
   if (init_var_table(thd) || init_var_items())
     return TRUE;
 
@@ -191,7 +194,7 @@ sp_rcontext::set_return_value(THD *thd, 
 */
 
 bool
-sp_rcontext::find_handler(uint sql_errno,
+sp_rcontext::find_handler(THD *thd, uint sql_errno,
                           MYSQL_ERROR::enum_warning_level level)
 {
   if (m_hfound >= 0)
@@ -200,6 +203,15 @@ sp_rcontext::find_handler(uint sql_errno
   const char *sqlstate= mysql_errno_to_sqlstate(sql_errno);
   int i= m_hcount, found= -1;
 
+  /*
+    If this is a fatal sub-statement error, and this runtime
+    context corresponds to a sub-statement, no CONTINUE/EXIT
+    handlers from this context are applicable: try to locate one
+    in the outer scope.
+  */
+  if (thd->is_fatal_sub_stmt_error && in_sub_stmt)
+    i= 0;
+
   /* Search handlers from the latest (innermost) to the oldest (outermost) */
   while (i--)
   {
@@ -252,7 +264,7 @@ sp_rcontext::find_handler(uint sql_errno
     */
     if (m_prev_runtime_ctx && IS_EXCEPTION_CONDITION(sqlstate) &&
         level == MYSQL_ERROR::WARN_LEVEL_ERROR)
-      return m_prev_runtime_ctx->find_handler(sql_errno, level);
+      return m_prev_runtime_ctx->find_handler(thd, sql_errno, level);
     return FALSE;
   }
   m_hfound= found;
@@ -298,7 +310,7 @@ sp_rcontext::handle_error(uint sql_errno
     elevated_level= MYSQL_ERROR::WARN_LEVEL_ERROR;
   }
 
-  if (find_handler(sql_errno, elevated_level))
+  if (find_handler(thd, sql_errno, elevated_level))
   {
     if (elevated_level == MYSQL_ERROR::WARN_LEVEL_ERROR)
     {
===== sql/sp_rcontext.h 1.34 vs edited =====
--- 1.34/sql/sp_rcontext.h	2006-12-23 22:04:26 +03:00
+++ edited/sql/sp_rcontext.h	2007-07-26 18:38:43 +04:00
@@ -125,7 +125,7 @@ class sp_rcontext : public Sql_alloc
 
   // Returns 1 if a handler was found, 0 otherwise.
   bool
-  find_handler(uint sql_errno,MYSQL_ERROR::enum_warning_level level);
+  find_handler(THD *thd, uint sql_errno,MYSQL_ERROR::enum_warning_level level);
 
   // If there is an error handler for this error, handle it and return TRUE.
   bool
@@ -236,6 +236,10 @@ private:
     during execution.
   */
   bool m_return_value_set;
+  /**
+    TRUE if the context is created for a sub-statement.
+  */
+  bool in_sub_stmt;
 
   sp_handler_t *m_handler;      // Visible handlers
   uint m_hcount;                // Stack pointer for m_handler
===== sql/sql_class.cc 1.274 vs edited =====
--- 1.274/sql/sql_class.cc	2007-07-14 16:58:37 +04:00
+++ edited/sql/sql_class.cc	2007-07-26 19:06:13 +04:00
@@ -173,6 +173,7 @@ THD::THD()
    Open_tables_state(refresh_version),
    lock_id(&main_lock_id),
    user_time(0), in_sub_stmt(0), global_read_lock(0), is_fatal_error(0),
+   transaction_rollback_request(0), is_fatal_sub_stmt_error(0),
    rand_used(0), time_zone_used(0),
    last_insert_id_used(0), last_insert_id_used_bin_log(0), insert_id_used(0),
    clear_next_insert_id(0), in_lock_tables(0), bootstrap(0),
@@ -976,7 +977,7 @@ void select_send::abort()
 {
   DBUG_ENTER("select_send::abort");
   if (status && thd->spcont &&
-      thd->spcont->find_handler(thd->net.last_errno,
+      thd->spcont->find_handler(thd, thd->net.last_errno,
                                 MYSQL_ERROR::WARN_LEVEL_ERROR))
   {
     /*
@@ -2211,6 +2212,13 @@ void THD::restore_sub_statement_state(Su
   limit_found_rows= backup->limit_found_rows;
   sent_row_count=   backup->sent_row_count;
   client_capabilities= backup->client_capabilities;
+  /*
+    If we've left sub-statement mode, reset the fatal error flag.
+    Otherwise keep the current value, to propagate it up the sub-statement
+    stack.
+  */
+  if (!in_sub_stmt)
+    is_fatal_sub_stmt_error= FALSE;
 
   if ((options & OPTION_BIN_LOG) && is_update_query(lex->sql_command))
     mysql_bin_log.stop_union_events(this);
@@ -2224,6 +2232,18 @@ void THD::restore_sub_statement_state(Su
 }
 
 
+/**
+  Mark transaction to rollback and mark error as fatal to a sub-statement.
+
+  @param  thd   Thread handle
+  @param  all   TRUE <=> rollback main transaction.
+*/
+
+void mark_transaction_to_rollback(THD *thd, bool all)
+{
+  thd->is_fatal_sub_stmt_error= TRUE;
+  thd->transaction_rollback_request= all;
+}
 /***************************************************************************
   Handling of XA id cacheing
 ***************************************************************************/
===== sql/sql_class.h 1.342 vs edited =====
--- 1.342/sql/sql_class.h	2007-07-21 17:52:13 +04:00
+++ edited/sql/sql_class.h	2007-07-26 18:36:46 +04:00
@@ -1421,7 +1421,33 @@ public:
   bool       slave_thread, one_shot_set;
   bool	     locked, some_tables_deleted;
   bool       last_cuted_field;
-  bool	     no_errors, password, is_fatal_error;
+  bool	     no_errors, password;
+  /**
+    Set to TRUE if execution of the current compound statement
+    can not continue. In particular, disables activation of
+    CONTINUE or EXIT handlers of stored routines.
+    Reset in the end of processing of the current user request, in
+    @see mysql_reset_thd_for_next_command().
+  */
+  bool is_fatal_error;
+  /**
+    Set by a storage engine to request the entire
+    transaction (that possibly spans multiple engines) to
+    rollback. Reset in ha_rollback.
+  */
+  bool       transaction_rollback_request;
+  /**
+    TRUE if we are in a sub-statement and the current error can
+    not be safely recovered until we left the sub-statement mode.
+    In particular, disables activation of CONTINUE and EXIT
+    handlers inside sub-statements. E.g. if it is a deadlock
+    error and requires a transaction-wide rollback, this flag is
+    raised (traditionally, MySQL first has to close all the reads
+    via @see handler::ha_index_or_rnd_end() and only then perform
+    the rollback).
+    Reset to FALSE when we leave the sub-statement mode.
+  */
+  bool       is_fatal_sub_stmt_error;
   bool	     query_start_used, rand_used, time_zone_used;
 
   /*
@@ -2397,3 +2423,5 @@ public:
 /* Functions in sql_class.cc */
 
 void add_to_status(STATUS_VAR *to_var, STATUS_VAR *from_var);
+void mark_transaction_to_rollback(THD *thd, bool all);
+
--end cut

-- 
-- Konstantin Osipov              Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com   The best DATABASE COMPANY in the GALAXY