Bug#16587369: HANDLER FOR SQLEXCEPTION NOT CATCHING ERROR

1452 (FK CONSTRAINT) FOR UPDATE

Overview
--------
This is a bug regarding the handling of fatal errors. A fatal
error is an error that should not be caught by a condition
handler in a stored procedure. The issues described in the bug
report are that:

1. The same error is handled differently for different
   statements.
2. Seemingly trivial errors are treated as fatal.

It should be noted that in the context of this bug report (and
bug fix), only handler errors are relevant to consider. There are
two important issues regarding handler errors in the source code:

1. Deciding whether a handler error is to be considered fatal.
2. Ensuring that the handler error is indeed treated as fatal.

For the first issue, "is_fatal_error()" is provided by the
handler api, and returns true for handler errors that are
fatal. The second issue can be handled in various ways; the most
common is to call "print_error()" with a flag indicating that the
handler error is to be considered fatal. Inspecting how the
source code handles these two issues, in light of the problems
reported, reveals three shortcomings in the implementation:

1. Handler errors are in fact fatal by default, as defined by the
   "is_fatal_error()" function.
2. When an error is returned from the handler api, the caller
   does not always check if the error is fatal.
3. The presence of a fatal error is not always flagged when
   calling "print_error()".


Suggested fix
-------------
The scope and ambition for the fix is to treat handler errors
consistently for DML statements (update, insert, delete) by
making sure "is_fatal_error()" is called before calling
"print_error()", and to provide the "ME_FATALERROR" flag as
appopriate. This alone does not fix the bug that is reported,
since handler errors are fatal by default, hence, we also need to
extend the function "is_fatal_error()" to recognize more handler
errors as being non-fatal.

An attempt was made to consider handler errors non-fatal by
default, but this made a large number of tests fail in not very
obvious ways, so this solution was not pursued further. Instead,
a switch for catching non-fatal errors was added to
"is_fatal_error()", currently catching:

- HA_ERR_NO_REFERENCED_ROW: The error originally addressed by the
  bug report considered here.
- HA_ERR_ROW_IS_REFERENCED: Another FK constraint violation
  error, an obvious extension of the bug we are addressing.
- HA_ERR_LOCK_WAIT_TIMEOUT: See below.

Changing the implementation of "is_fatal_error()" has a potential
impact on the non-DML source code, which also calls this
function; however, no damage has been observed in the testing
that has been done. On the other hand, tests failed for other
reasons:

1. Some tests expected a fatal outcome in situations where the
   error is now non-fatal (e.g. for foreign key constraint
   violations). The fix for this was to modify the test.
2. Some tests expected a non-fatal outcome where the error is now
   fatal due to the changed implementation of DML statements
   where we ensure that errors are treated the way they ought
   to. The fix for this was to extend "is_fatal_error()" to
   consider the error in question as non fatal (the error here
   was lock wait timeout).

Re-considering which handler errors should be considered fatal is
still a relevant task, but is considered beyond the scope of this
bug fix. Additionally, such a task might be the responsibility of
the storage engine teams.

Author: ssorumgard

sql/handler.cc

@@ -3581,6 +3581,41 @@ void print_keydup_error(TABLE *table, KEY *key, myf errflag)
 }
 
 
+/**
+  This method is used to analyse the error to see whether the error
+  is ignorable or not. Further comments in header file. 
+*/
+
+bool handler::is_fatal_error(int error, uint flags)
+{
+  DBUG_ENTER("is_fatal_error");
+  // Error code 0 is not fatal, dup key errors may be explicitly ignored
+  if (!error || ((flags & HA_CHECK_DUP_KEY) &&
+       (error == HA_ERR_FOUND_DUPP_KEY ||
+        error == HA_ERR_FOUND_DUPP_UNIQUE)))
+    DBUG_RETURN(false);
+  
+  // Catch errors that are not fatal
+  switch (error)
+  {
+    /*
+      Lock wait timeout was treated as 'non-fatal' by e.g. insert code,
+      and as 'fatal' by update code prior to fix of bug#16587369. 
+      In order to change current behavior as little as possible, we 
+      for now treat lock wait timeout as non-fatal rather than fatal.
+    */
+    case HA_ERR_LOCK_WAIT_TIMEOUT:
+    // Foreign key constraint violations are not fatal:
+    case HA_ERR_ROW_IS_REFERENCED:
+    case HA_ERR_NO_REFERENCED_ROW:
+      DBUG_RETURN(false);
+  }
+  
+  // Default is that an error is fatal
+  DBUG_RETURN(true);
+}
+
+
 /**
   Print error that we got from handler function.
 

sql/handler.h

@@ -2193,24 +2193,26 @@ class handler :public Sql_alloc
   virtual uint extra_rec_buf_length() const { return 0; }
 
   /**
-    This method is used to analyse the error to see whether the error
-    is ignorable or not, certain handlers can have more error that are
-    ignorable than others. E.g. the partition handler can get inserts
-    into a range where there is no partition and this is an ignorable
-    error.
+    @brief Determine whether an error is fatal or not. 
+    
+    @details This method is used to analyze the error to see whether the 
+    error is fatal or not. Handlers can have different sets of fatal errors, 
+    e.g. the partition handler can get inserts into a range where there 
+    is no partition, and this is an ignorable error.
+    
     HA_ERR_FOUND_DUP_UNIQUE is a special case in MyISAM that means the
-    same thing as HA_ERR_FOUND_DUP_KEY but can in some cases lead to
+    same thing as HA_ERR_FOUND_DUP_KEY, but can in some cases lead to
     a slightly different error message.
+     
+    @param error  error code received from the handler interface (HA_ERR_...)
+    @param flags  indicate whether duplicate key errors should be ignorable
+    
+    @return   whether the error is fatal or not
+      @retval true  the error is fatal
+      @retval false the error is not fatal
   */
-  virtual bool is_fatal_error(int error, uint flags)
-  {
-    if (!error ||
-        ((flags & HA_CHECK_DUP_KEY) &&
-         (error == HA_ERR_FOUND_DUPP_KEY ||
-          error == HA_ERR_FOUND_DUPP_UNIQUE)))
-      return FALSE;
-    return TRUE;
-  }
+  
+  virtual bool is_fatal_error(int error, uint flags);
 
   /**
     Number of rows in table. It will only be called if

sql/sql_delete.cc

@@ -49,6 +49,7 @@
 bool mysql_delete(THD *thd, TABLE_LIST *table_list, Item *conds,
                   SQL_I_List<ORDER> *order_list, ha_rows limit, ulonglong options)
 {
+  myf           error_flags= MYF(0);            /**< Flag for fatal errors */
   bool          will_batch;
   int		error, loc_error;
   TABLE		*table;
@@ -205,7 +206,10 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, Item *conds,
     }
     if (error != HA_ERR_WRONG_COMMAND)
     {
-      table->file->print_error(error,MYF(0));
+      if (table->file->is_fatal_error(error, HA_CHECK_DUP_KEY))
+        error_flags|= ME_FATALERROR;
+
+      table->file->print_error(error, error_flags);
       error=0;
       goto cleanup;
     }
@@ -383,7 +387,10 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, Item *conds,
     /* If quick select is used, initialize it before retrieving rows. */
     if (select && select->quick && (error= select->quick->reset()))
     {
-      table->file->print_error(error, MYF(0));
+      if (table->file->is_fatal_error(error, HA_CHECK_DUP_KEY))
+        error_flags|= ME_FATALERROR;
+
+      table->file->print_error(error, error_flags);
       err= true;
       goto exit_without_my_ok;
     }
@@ -458,7 +465,10 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, Item *conds,
         }
         else
         {
-          table->file->print_error(error,MYF(0));
+          if (table->file->is_fatal_error(error, HA_CHECK_DUP_KEY))
+            error_flags|= ME_FATALERROR;
+          
+          table->file->print_error(error, error_flags);
           /*
             In < 4.0.14 we set the error number to 0 here, but that
             was not sensible, because then MySQL would not roll back the
@@ -487,7 +497,12 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, Item *conds,
     {
       /* purecov: begin inspected */
       if (error != 1)
-        table->file->print_error(loc_error,MYF(0));
+      {
+        if (table->file->is_fatal_error(loc_error, HA_CHECK_DUP_KEY))
+          error_flags|= ME_FATALERROR;
+
+        table->file->print_error(loc_error, error_flags);
+      }
       error=1;
       /* purecov: end */
     }
@@ -891,7 +906,11 @@ bool multi_delete::send_data(List<Item> &values)
           If the IGNORE option is used errors caused by ha_delete_row don't
           have to stop the iteration.
         */
-        table->file->print_error(error,MYF(0));
+        myf error_flags= MYF(0);
+        if (table->file->is_fatal_error(error, HA_CHECK_DUP_KEY))
+          error_flags|= ME_FATALERROR;
+
+        table->file->print_error(error, error_flags);
         DBUG_RETURN(1);
       }
     }
@@ -1035,6 +1054,7 @@ int multi_delete::do_deletes()
 */
 int multi_delete::do_table_deletes(TABLE *table, bool ignore)
 {
+  myf error_flags= MYF(0);                      /**< Flag for fatal errors */
   int local_error= 0;
   READ_RECORD info;
   ha_rows last_deleted= deleted;
@@ -1060,7 +1080,10 @@ int multi_delete::do_table_deletes(TABLE *table, bool ignore)
     local_error= table->file->ha_delete_row(table->record[0]);
     if (local_error && !ignore)
     {
-      table->file->print_error(local_error, MYF(0));
+      if (table->file->is_fatal_error(local_error, HA_CHECK_DUP_KEY))
+        error_flags|= ME_FATALERROR;
+
+      table->file->print_error(local_error, error_flags);
       break;
     }
 
@@ -1087,7 +1110,10 @@ int multi_delete::do_table_deletes(TABLE *table, bool ignore)
     if (tmp_error && !local_error)
     {
       local_error= tmp_error;
-      table->file->print_error(local_error, MYF(0));
+      if (table->file->is_fatal_error(local_error, HA_CHECK_DUP_KEY))
+        error_flags|= ME_FATALERROR;
+
+      table->file->print_error(local_error, error_flags);
     }
   }
   if (last_deleted != deleted && !table->file->has_transactions())

sql/sql_insert.cc

@@ -783,11 +783,14 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list,
   */
   {
     table->file->ha_release_auto_increment();
-    if (thd->locked_tables_mode <= LTM_LOCK_TABLES &&
-        table->file->ha_end_bulk_insert() && !error)
+    if (thd->locked_tables_mode <= LTM_LOCK_TABLES && !error &&
+        (error= table->file->ha_end_bulk_insert()))
     {
-      table->file->print_error(my_errno,MYF(0));
-      error=1;
+      myf error_flags= MYF(0);
+      if (table->file->is_fatal_error(error, HA_CHECK_DUP_KEY))
+        error_flags|= ME_FATALERROR;
+
+      table->file->print_error(error, error_flags);
     }
     if (duplic != DUP_ERROR || ignore)
       table->file->extra(HA_EXTRA_NO_IGNORE_DUP_KEY);
@@ -1613,11 +1616,17 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, COPY_INFO *update)
   DBUG_RETURN(trg_error);
 
 err:
-  info->last_errno= error;
-  DBUG_ASSERT(thd->lex->current_select() != NULL);
+  {
+    myf error_flags= MYF(0);                      /**< Flag for fatal errors */
+    info->last_errno= error;
+    DBUG_ASSERT(thd->lex->current_select() != NULL);
     thd->lex->current_select()->no_error= 0;        // Give error
-  table->file->print_error(error,MYF(0));
-  
+    if (table->file->is_fatal_error(error, HA_CHECK_DUP_KEY))
+      error_flags|= ME_FATALERROR;
+
+    table->file->print_error(error, error_flags);
+  }
+
 before_trg_err:
   table->file->restore_auto_increment(prev_insert_id);
   if (key)
@@ -2089,7 +2098,11 @@ bool select_insert::send_eof()
 
   if (error)
   {
-    table->file->print_error(error,MYF(0));
+    myf error_flags= MYF(0);
+    if (table->file->is_fatal_error(my_errno, HA_CHECK_DUP_KEY))
+      error_flags|= ME_FATALERROR;
+
+    table->file->print_error(my_errno, error_flags);
     DBUG_RETURN(1);
   }