Bug#49534: multitable IGNORE update with sql_safe_updates

error causes debug assertion

The IGNORE option of the multiple-table UPDATE command was
not intended to suppress errors caused by the
sql_safe_updates mode. This flag will raise an error if the
execution of UPDATE does not use a key for row retrieval,
and should continue do so regardless of the IGNORE option.

However the implementation of IGNORE does not support
exceptions to the rule; it always converts errors to
warnings and cannot be extended. The Internal_error_handler
interface offers the infrastructure to handle individual
errors, making sure that the error raised by
sql_safe_updates is not silenced.

Fixed by implementing an Internal_error_handler and using it
for UPDATE IGNORE commands.

mysql-test/r/multi_update.result

@@ -634,4 +634,15 @@ select count(*) from t3 /* must be 1 */;
 count(*)
 1
 drop table t1, t2, t3;
+#
+# Bug#49534: multitable IGNORE update with sql_safe_updates error
+# causes debug assertion
+#
+CREATE TABLE t1( a INT, KEY( a ) );
+INSERT INTO t1 VALUES (1), (2), (3);
+SET SESSION sql_safe_updates = 1;
+# Must not cause failed assertion
+UPDATE IGNORE t1, t1 t1a SET t1.a = 1 WHERE t1a.a = 1;
+ERROR HY000: You are using safe update mode and you tried to update a table without a WHERE that uses a KEY column
+DROP TABLE t1;
 end of tests

mysql-test/t/multi_update.test

@@ -637,5 +637,17 @@ drop table t1, t2, t3;
 # Add further tests from here
 #
 
+--echo #
+--echo # Bug#49534: multitable IGNORE update with sql_safe_updates error
+--echo # causes debug assertion
+--echo #
+CREATE TABLE t1( a INT, KEY( a ) );
+INSERT INTO t1 VALUES (1), (2), (3);
+SET SESSION sql_safe_updates = 1;
+--echo # Must not cause failed assertion
+--error ER_UPDATE_WITHOUT_KEY_IN_SAFE_MODE
+UPDATE IGNORE t1, t1 t1a SET t1.a = 1 WHERE t1a.a = 1;
+DROP TABLE t1;
+
 
 --echo end of tests

sql/sql_class.cc

@@ -740,10 +740,12 @@ bool THD::handle_error(uint sql_errno, const char *message,
 }
 
 
-void THD::pop_internal_handler()
+Internal_error_handler *THD::pop_internal_handler()
 {
   DBUG_ASSERT(m_internal_handler != NULL);
+  Internal_error_handler *popped_handler= m_internal_handler;
   m_internal_handler= m_internal_handler->m_prev_internal_handler;
+  return popped_handler;
 }
 
 extern "C"

sql/sql_class.h

@@ -2313,7 +2313,7 @@ public:
   /**
     Remove the error handler last pushed.
   */
-  void pop_internal_handler();
+  Internal_error_handler *pop_internal_handler();
 
   /** Overloaded to guard query/query_length fields */
   virtual void set_statement(Statement *stmt);

sql/sql_update.cc

@@ -1188,6 +1188,56 @@ reopen_tables:
 }
 
 
+/**
+   Implementation of the safe update options during UPDATE IGNORE. This syntax
+   causes an UPDATE statement to ignore all errors. In safe update mode,
+   however, we must never ignore the ER_UPDATE_WITHOUT_KEY_IN_SAFE_MODE. There
+   is a special hook in my_message_sql that will otherwise delete all errors
+   when the IGNORE option is specified. 
+
+   In the future, all IGNORE handling should be used with this class and all
+   traces of the hack outlined below should be removed.
+
+   - The parser detects IGNORE option and sets thd->lex->ignore= 1
+   
+   - In JOIN::optimize, if this is set, then 
+     thd->lex->current_select->no_error gets set.
+
+   - In my_message_sql(), if the flag above is set then any error is
+     unconditionally converted to a warning.
+
+   We are moving in the direction of using Internal_error_handler subclasses
+   to do all such error tweaking, please continue this effort if new bugs
+   appear.
+ */
+class Safe_dml_handler : public Internal_error_handler {
+
+private:
+  bool m_handled_error;
+
+public:
+  explicit Safe_dml_handler() : m_handled_error(FALSE) {}
+
+  bool handle_error(uint sql_errno,
+                    const char *message,
+                    MYSQL_ERROR::enum_warning_level level,
+                    THD *thd)
+  {
+    if (level == MYSQL_ERROR::WARN_LEVEL_ERROR &&
+        sql_errno == ER_UPDATE_WITHOUT_KEY_IN_SAFE_MODE)
+        
+    {      
+      thd->main_da.set_error_status(thd, sql_errno, message);
+      m_handled_error= TRUE;
+      return TRUE;
+    }
+    return FALSE;
+  }
+
+  bool handled_error() { return m_handled_error; }
+
+};
+
 /*
   Setup multi-update handling and call SELECT to do the join
 */
@@ -1216,6 +1266,12 @@ bool mysql_multi_update(THD *thd,
                                MODE_STRICT_ALL_TABLES));
 
   List<Item> total_list;
+
+  Safe_dml_handler handler;
+  bool using_handler= thd->options & OPTION_SAFE_UPDATES;
+  if (using_handler)
+    thd->push_internal_handler(&handler);
+
   res= mysql_select(thd, &select_lex->ref_pointer_array,
                     table_list, select_lex->with_wild,
                     total_list,
@@ -1224,10 +1280,21 @@ bool mysql_multi_update(THD *thd,
                     options | SELECT_NO_JOIN_CACHE | SELECT_NO_UNLOCK |
                     OPTION_SETUP_TABLES_DONE,
                     result, unit, select_lex);
-  DBUG_PRINT("info",("res: %d  report_error: %d", res,
-                     (int) thd->is_error()));
+
+  if (using_handler)
+  {
+    Internal_error_handler *top_handler= thd->pop_internal_handler();
+    DBUG_ASSERT(&handler == top_handler);
+  }
+
+  DBUG_PRINT("info",("res: %d  report_error: %d", res, (int) thd->is_error()));
   res|= thd->is_error();
-  if (unlikely(res))
+  /*
+    Todo: remove below code and make Safe_dml_handler do error processing
+    instead. That way we can return the actual error instead of
+    ER_UNKNOWN_ERROR.
+  */
+  if (unlikely(res) && (!using_handler || !handler.handled_error()))
   {
     /* If we had a another error reported earlier then this will be ignored */
     result->send_error(ER_UNKNOWN_ERROR, ER(ER_UNKNOWN_ERROR));