MDEV-29013 ER_KEY_NOT_FOUND/lock timeout upon online alter with long uniquebb-10.10-MDEV-29021

1. ER_KEY_NOT_FOUND
Some virtual columns were not updated because were not included in read_set
to the moment of calculation.

In Row_logs_event::do_apply_event some fields are excluded based on m_cols
value, the number of replicated rows.

We can't rely on this. Basically, we'd be satisfied, if all columns were
just set, at least for now.

2. ER_LOCK_WAIT_TIMEOUT
This is a long unique specific problem.

Sometimes, lookup_handler is created for to->file. To properly free it,
ha_reset should be called. It is usually done by calling
close_thread_table, but ALTER TABLE makes it differently. Hence, a single
ha_reset call is added to mysql_alter_table.

Also, the lifetime of lookup_handler is corrected: it's been allocated on
thd->mem_root, which in case of online alter
is event's mem_root, and can vary other ways.
It is changed to table->mem_root, which is more appropriate:
ha_reset is called only when table is closed, or in a few additional cases,
which correspond to a statement end. So memory leaks are unlikely here.

5 files changed, 126 insertions, 27 deletions

diff --git a/mysql-test/main/alter_table_online_debug.result b/mysql-test/main/alter_table_online_debug.result
index ef2aba2eb39..bc6aac0f3f6 100644
--- a/mysql-test/main/alter_table_online_debug.result
+++ b/mysql-test/main/alter_table_online_debug.result
@@ -723,5 +723,39 @@ connection default;
 drop table t1;
 set debug_sync= reset;
 #
+# MDEV-29013 ER_KEY_NOT_FOUND/lock timeout upon online alter
+# with long unique indexes
+#
+create table t1 (b text not null, unique(b));
+insert into t1 values ('foo'),('bar');
+set debug_sync= 'now wait_for downgraded';
+connection con2;
+set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for goforit';
+alter table t1 add c int, algorithm=copy, lock=none;
+connection default;
+delete from t1;
+set debug_sync= 'now signal goforit';
+connection con2;
+connection default;
+drop table t1;
+set debug_sync= reset;
+###
+create table t1 (a text, unique(a)) engine=innodb;
+create table t2 (b text, unique(b)) engine=innodb;
+insert into t2 values (null),(null);
+set debug_sync= 'now wait_for downgraded';
+connection con2;
+set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for goforit';
+alter table t2 add column c int, algorithm=copy, lock=none;
+connection default;
+delete from t2;
+set debug_sync= 'now signal goforit';
+connection con2;
+connection default;
+alter table t2 force;
+alter table t1 force;
+drop table t1, t2;
+set debug_sync= reset;
+#
 # End of 10.10 tests
 #
diff --git a/mysql-test/main/alter_table_online_debug.test b/mysql-test/main/alter_table_online_debug.test
index 8cd50d02e26..a6cb7bb7667 100644
--- a/mysql-test/main/alter_table_online_debug.test
+++ b/mysql-test/main/alter_table_online_debug.test
@@ -901,5 +901,59 @@ drop table t1;
 set debug_sync= reset;
 
 --echo #
+--echo # MDEV-29013 ER_KEY_NOT_FOUND/lock timeout upon online alter
+--echo # with long unique indexes
+--echo #
+create table t1 (b text not null, unique(b));
+insert into t1 values ('foo'),('bar');
+--send
+set debug_sync= 'now wait_for downgraded';
+
+--connection con2
+set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for goforit';
+--send
+alter table t1 add c int, algorithm=copy, lock=none;
+
+--connection default
+--reap
+delete from t1;
+set debug_sync= 'now signal goforit';
+
+--connection con2
+--reap
+
+--connection default
+drop table t1;
+set debug_sync= reset;
+
+--echo ###
+
+create table t1 (a text, unique(a)) engine=innodb;
+create table t2 (b text, unique(b)) engine=innodb;
+insert into t2 values (null),(null);
+--send
+set debug_sync= 'now wait_for downgraded';
+
+--connection con2
+set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for goforit';
+--send
+alter table t2 add column c int, algorithm=copy, lock=none;
+
+--connection default
+--reap
+delete from t2;
+set debug_sync= 'now signal goforit';
+
+--connection con2
+--reap
+
+--connection default
+alter table t2 force;
+alter table t1 force;
+
+drop table t1, t2;
+set debug_sync= reset;
+
+--echo #
 --echo # End of 10.10 tests
 --echo #
diff --git a/sql/handler.cc b/sql/handler.cc
index d71c0fc83a3..fbebfd27203 100644
--- a/sql/handler.cc
+++ b/sql/handler.cc
@@ -3212,7 +3212,7 @@ int handler::create_lookup_handler()
   handler *tmp;
   if (lookup_handler != this)
     return 0;
-  if (!(tmp= clone(table->s->normalized_path.str, table->in_use->mem_root)))
+  if (!(tmp= clone(table->s->normalized_path.str, &table->mem_root)))
     return 1;
   lookup_handler= tmp;
   return lookup_handler->ha_external_lock(table->in_use, F_RDLCK);
diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc
index 89ca2a41937..2c19bf67586 100644
--- a/sql/log_event_server.cc
+++ b/sql/log_event_server.cc
@@ -6007,34 +6007,41 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi)
      if (m_width == table->s->fields && bitmap_is_set_all(&m_cols))
       set_flags(COMPLETE_ROWS_F);
 
-    /* 
-      Set tables write and read sets.
-      
-      Read_set contains all slave columns (in case we are going to fetch
-      a complete record from slave)
-      
-      Write_set equals the m_cols bitmap sent from master but it can be 
-      longer if slave has extra columns. 
-     */ 
+    Rpl_table_data rpl_data{};
+    if (rgi) rgi->get_table_data(table, &rpl_data);
 
-    DBUG_PRINT_BITSET("debug", "Setting table's read_set from: %s", &m_cols);
-    
-    bitmap_set_all(table->read_set);
-    if (get_general_type_code() == DELETE_ROWS_EVENT ||
-        get_general_type_code() == UPDATE_ROWS_EVENT)
-      bitmap_intersect(table->read_set,&m_cols);
+    if (!rpl_data.copy_fields)
+    {
+      /*
+        Set tables write and read sets.
 
-    bitmap_set_all(table->write_set);
-    table->rpl_write_set= table->write_set;
+        Read_set contains all slave columns (in case we are going to fetch
+        a complete record from slave)
 
-    /* WRITE ROWS EVENTS store the bitmap in m_cols instead of m_cols_ai */
-    MY_BITMAP *after_image= ((get_general_type_code() == UPDATE_ROWS_EVENT) ?
-                             &m_cols_ai : &m_cols);
-    bitmap_intersect(table->write_set, after_image);
+        Write_set equals the m_cols bitmap sent from master but it can be
+        longer if slave has extra columns.
+       */
+
+      DBUG_PRINT_BITSET("debug", "Setting table's read_set from: %s", &m_cols);
+
+      bitmap_set_all(table->read_set);
+      if (get_general_type_code() == DELETE_ROWS_EVENT ||
+          get_general_type_code() == UPDATE_ROWS_EVENT)
+        bitmap_intersect(table->read_set, &m_cols);
+
+      bitmap_set_all(table->write_set);
+
+      /* WRITE ROWS EVENTS store the bitmap in m_cols instead of m_cols_ai */
+      MY_BITMAP *after_image= ((get_general_type_code() == UPDATE_ROWS_EVENT) ?
+                               &m_cols_ai : &m_cols);
+      bitmap_intersect(table->write_set, after_image);
 
-    this->slave_exec_mode= slave_exec_mode_options; // fix the mode
+      this->slave_exec_mode= slave_exec_mode_options; // fix the mode
+    }
+
+    table->rpl_write_set= table->write_set;
 
-    // Do event specific preparations 
+    // Do event specific preparations
     error= do_before_row_operations(rli);
 
     /*
@@ -6045,8 +6052,6 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi)
       extra columns on the slave. In that case, do not force
       MODE_NO_AUTO_VALUE_ON_ZERO.
     */
-    Rpl_table_data rpl_data{};
-    if (rgi) rgi->get_table_data(table, &rpl_data);
     sql_mode_t saved_sql_mode= thd->variables.sql_mode;
     if (!is_auto_inc_in_extra_columns())
       thd->variables.sql_mode= (rpl_data.copy_fields ? saved_sql_mode : 0)
diff --git a/sql/sql_table.cc b/sql/sql_table.cc
index 29c4d09cd39..a3efcdd9785 100644
--- a/sql/sql_table.cc
+++ b/sql/sql_table.cc
@@ -11011,6 +11011,9 @@ do_continue:;
                    !(table->file->ha_table_flags() & HA_REUSES_FILE_NAMES) &&
                    !(new_table->file->ha_table_flags() &
                      HA_REUSES_FILE_NAMES));
+
+  // Close lookup_handler.
+  new_table->file->ha_reset();
   /*
     Close the intermediate table that will be the new table, but do
     not delete it! Even though MERGE tables do not have their children
@@ -11854,8 +11857,11 @@ copy_data_between_tables(THD *thd, TABLE *from, TABLE *to,
 
     // We'll be filling from->record[0] from row events
     bitmap_set_all(from->write_set);
-    // We restore bitmaps, because update event is going to mess up with them.
+    // Use all columns. UPDATE/DELETE events reset read_set and write_set to
+    // def_*_set after each row operation, so using all_set won't work.
     to->default_column_bitmaps();
+    bitmap_set_all(&to->def_read_set);
+    bitmap_set_all(&to->def_write_set);
 
     end_read_record(&info);
     init_read_record_done= false;