errors after altering a table has finished aren't fatal

We cannot revert the ALTER, so anything happening after
the point of no return should not be treated as an error. A
very unfortunate condition that a user needs to be warned about - yes,
but we cannot say "ALTER TABLE has failed" if the table was successfully
altered.

mysql-test/suite/s3/alter.result

@@ -102,7 +102,9 @@ drop table t1;
 create table t1 (a int, b int) engine=aria select seq as a,seq+10 as b from seq_1_to_10;
 lock table t1 write;
 alter table t1 add column c int, engine=s3;
-ERROR HY000: Table 't1' is read only
+Warnings:
+Warning	1036	Table 't1' is read only
+Warning	1213	Deadlock found when trying to get lock; try restarting transaction
 unlock tables;
 select count(*), sum(a), sum(b), sum(c) from t1;
 count(*)	sum(a)	sum(b)	sum(c)

mysql-test/suite/s3/alter.test

@@ -69,7 +69,6 @@ drop table t1;
 
 create table t1 (a int, b int) engine=aria select seq as a,seq+10 as b from seq_1_to_10;
 lock table t1 write;
---error ER_OPEN_AS_READONLY
 alter table t1 add column c int, engine=s3;
 unlock tables;
 select count(*), sum(a), sum(b), sum(c) from t1;

sql/sql_class.h

@@ -2015,6 +2015,25 @@ class MDL_deadlock_and_lock_abort_error_handler: public Internal_error_handler
 };
 
 
+class Turn_errors_to_warnings_handler : public Internal_error_handler
+{
+public:
+  Turn_errors_to_warnings_handler() {}
+  bool handle_condition(THD *thd,
+                        uint sql_errno,
+                        const char* sqlstate,
+                        Sql_condition::enum_warning_level *level,
+                        const char* msg,
+                        Sql_condition ** cond_hdl)
+  {
+    *cond_hdl= NULL;
+    if (*level == Sql_condition::WARN_LEVEL_ERROR)
+      *level= Sql_condition::WARN_LEVEL_WARN;
+    return(0);
+  }
+};
+
+
 /**
   Tables that were locked with LOCK TABLES statement.
 

sql/sql_table.cc

@@ -9286,10 +9286,11 @@ bool mysql_alter_table(THD *thd, const LEX_CSTRING *new_db,
                        uint order_num, ORDER *order, bool ignore,
                        bool if_exists)
 {
-  bool engine_changed, error, frm_is_created= false;
+  bool engine_changed, error, frm_is_created= false, error_handler_pushed= false;
   bool no_ha_table= true;  /* We have not created table in storage engine yet */
   TABLE *table, *new_table;
   DDL_LOG_STATE ddl_log_state;
+  Turn_errors_to_warnings_handler errors_to_warnings;
 
 #ifdef WITH_PARTITION_STORAGE_ENGINE
   bool partition_changed= false;
@@ -10610,19 +10611,21 @@ do_continue:;
   }
 
   // ALTER TABLE succeeded, delete the backup of the old table.
-  error= quick_rm_table(thd, old_db_type, &alter_ctx.db, &backup_name,
-                        FN_IS_TMP |
-                        (engine_changed ? NO_HA_TABLE | NO_PAR_TABLE: 0));
+  // a failure to delete isn't an error, as we cannot rollback ALTER anymore
+  thd->push_internal_handler(&errors_to_warnings);
+  error_handler_pushed=1;
+
+  quick_rm_table(thd, old_db_type, &alter_ctx.db, &backup_name,
+                        FN_IS_TMP | (engine_changed ? NO_HA_TABLE | NO_PAR_TABLE: 0));
 
   debug_crash_here("ddl_log_alter_after_delete_backup");
   if (engine_changed)
   {
     /* the .frm file was removed but not the original table */
-    error|= quick_rm_table(thd, old_db_type, &alter_ctx.db,
-                           &alter_ctx.table_name,
-                           NO_FRM_RENAME |
-                           (engine_changed ? 0 : FN_IS_TMP));
+    quick_rm_table(thd, old_db_type, &alter_ctx.db, &alter_ctx.table_name,
+                           NO_FRM_RENAME | (engine_changed ? 0 : FN_IS_TMP));
   }
+
   debug_crash_here("ddl_log_alter_after_drop_original_table");
   if (binlog_as_create_select)
   {
@@ -10637,21 +10640,15 @@ do_continue:;
     thd->binlog_xid= 0;
   }
 
-  if (error)
-  {
-    /*
-      The fact that deletion of the backup failed is not critical
-      error, but still worth reporting as it might indicate serious
-      problem with server.
-    */
-    goto err_with_mdl_after_alter;
-  }
-
 end_inplace:
   thd->variables.option_bits&= ~OPTION_BIN_COMMIT_OFF;
 
-  if (thd->locked_tables_list.reopen_tables(thd, false))
-    goto err_with_mdl_after_alter;
+  if (!error_handler_pushed)
+    thd->push_internal_handler(&errors_to_warnings);
+
+  thd->locked_tables_list.reopen_tables(thd, false);
+
+  thd->pop_internal_handler();
 
   THD_STAGE_INFO(thd, stage_end);
   DEBUG_SYNC(thd, "alter_table_before_main_binlog");
@@ -10763,19 +10760,6 @@ do_continue:;
   }
   DBUG_RETURN(true);
 
-err_with_mdl_after_alter:
-  DBUG_PRINT("error", ("err_with_mdl_after_alter"));
-  /* the table was altered. binlog the operation */
-  DBUG_ASSERT(!(mysql_bin_log.is_open() &&
-                thd->is_current_stmt_binlog_format_row() &&
-                (create_info->tmp_table())));
-  /*
-    We can't reset error as we will return 'true' below and the server
-    expects that error is set
-  */
-  if (!binlog_as_create_select)
-    write_bin_log_with_if_exists(thd, FALSE, FALSE, log_if_exists);
-
 err_with_mdl:
   ddl_log_complete(&ddl_log_state);
   /*

sql/table.cc

@@ -8574,25 +8574,6 @@ bool is_simple_order(ORDER *order)
   return TRUE;
 }
 
-class Turn_errors_to_warnings_handler : public Internal_error_handler
-{
-public:
-  Turn_errors_to_warnings_handler() {}
-  bool handle_condition(THD *thd,
-                        uint sql_errno,
-                        const char* sqlstate,
-                        Sql_condition::enum_warning_level *level,
-                        const char* msg,
-                        Sql_condition ** cond_hdl)
-  {
-    *cond_hdl= NULL;
-    if (*level == Sql_condition::WARN_LEVEL_ERROR)
-      *level= Sql_condition::WARN_LEVEL_WARN;
-    return(0);
-  }
-};
-
-
 /*
   to satisfy marked_for_write_or_computed() Field's assert we temporarily
   mark field for write before storing the generated value in it