MDEV-35661 Assertion `!cache_list ||...' fails in online_alter_close_connection

After MDEV-35182 (88adcbf3) fix, online_alter still wasn't fully ready for `XA PREPARE` to be skipped. In fact, according to XA standard, even xa_commit() call may happen without preceding xa_prepare(), so this stage is completely optional. This commit removes any relying on xa_prepare() call. The only thing that will be done there is an equivalent of "statement-level commit", which basically just flushes a pending rows event to an online alter cache. This is done by always storing an online alter cache in the XA_data object whenever XA is active, rather than in THD::ha_data.
2025-01-16 03:52:35 +01:00 · 2024-12-22 17:25:58 +01:00 · 2024-12-22 17:25:58 +01:00 · 22bbed7416
commit 22bbed7416
parent 9386283180
5 changed files with 162 additions and 59 deletions
--- a/mysql-test/main/alter_table_online_debug.result
+++ b/mysql-test/main/alter_table_online_debug.result
@ -1608,6 +1608,7 @@ connection default;
 select * from t;
 a
 2
+53
 # XA transaction is left uncommitted
 # end then is rollbacked after alter fails
 set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go';
@ -1629,6 +1630,7 @@ xa rollback 'xuncommitted';
 select * from t;
 a
 2
+53
 # Same, but commit
 set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go';
 set statement innodb_lock_wait_timeout=0, lock_wait_timeout= 0
@ -1650,6 +1652,7 @@ select * from t;
 a
 2
 3
+53
 # Commit, but error in statement, and there is some stmt data to rollback
 set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go';
 alter table t force, algorithm=copy, lock=none;
@ -1673,6 +1676,7 @@ a
 2
 3
 5
+53
 connect con1, localhost, root,,;
 connection default;
 drop table t;
@ -1731,9 +1735,9 @@ set debug_sync= 'now signal go';
 connection default;
 select * from t;
 a	c
-1	777
 2	777
 3	777
+10	777
 set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go';
 alter table t drop primary key, algorithm=copy;
 connection con1;
@ -1750,9 +1754,9 @@ set debug_sync= 'now signal go';
 connection default;
 select * from t;
 a	c
-10	777
 2	777
 3	777
+10	777
 # The only statement succeeds (test both commit and rollback)
 set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go';
 alter table t add d text default ('qwe'), algorithm=copy;
@ -1767,9 +1771,9 @@ set debug_sync= 'now signal go';
 connection default;
 select * from t;
 a	c	d
-10	777	qwe
-2	777	qwe
-3	777	qwe
+0	777	qwe
+0	777	qwe
+0	777	qwe
 set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go';
 alter table t drop c, algorithm=copy;
 connection con1;
@ -1835,3 +1839,48 @@ disconnect con2;
 #
 # End of 11.2 tests
 #
+#
+# MDEV-35661 Assertion `!cache_list || cache_list->empty()' fails in
+# online_alter_close_connection
+SET @isolation.save= @@global.innodb_snapshot_isolation;
+SET GLOBAL innodb_snapshot_isolation= ON;
+CREATE TABLE t1 (a INT) ENGINE=InnoDB;
+CREATE TABLE t2 (col_char CHAR(224), id SERIAL) ENGINE=InnoDB;
+CREATE TABLE t3 (id int PRIMARY KEY, c char) ENGINE=innodb;
+INSERT INTO t3 VALUES (1, 'a');
+connect  con1,localhost,root,,;
+connect  con2,localhost,root,,;
+connect  con3,localhost,root,,;
+XA BEGIN 'x1';
+INSERT INTO t2 () VALUES ();
+connection con2;
+XA START 'x2';
+SELECT * FROM t1;
+a
+UPDATE t2 SET col_char = 'b';
+ERROR HY000: Record has changed since last read in table 't2'
+connection con1;
+set debug_sync= 'alter_table_online_before_lock signal downgraded wait_for go';
+ALTER TABLE t3 ADD KEY (c), lock=none, algorithm = copy;
+connection con2;
+set debug_sync= 'now wait_for downgraded';
+DELETE FROM t3 WHERE id = 0;
+UPDATE t3 SET c = '';
+set debug_sync= 'now signal go';
+connection con2;
+XA END 'x2';
+ERROR XA100: XA_RBROLLBACK: Transaction branch was rolled back
+XA ROLLBACK 'x2';
+connection con1;
+disconnect con1;
+disconnect con2;
+connection con3;
+XA END 'x1';
+XA ROLLBACK 'x1';
+disconnect con3;
+connection default;
+DROP TABLE t1, t2, t3;
+SET GLOBAL innodb_snapshot_isolation= @isolation.save;
+#
+# End of 11.7 tests
+#
--- a/mysql-test/main/alter_table_online_debug.test
+++ b/mysql-test/main/alter_table_online_debug.test
@ -2108,3 +2108,59 @@ eval set global default_storage_engine= $default_storage_engine;
 --echo #
 --echo # End of 11.2 tests
 --echo #
+
+--echo #
+--echo # MDEV-35661 Assertion `!cache_list || cache_list->empty()' fails in
+--echo # online_alter_close_connection
+
+SET @isolation.save= @@global.innodb_snapshot_isolation;
+SET GLOBAL innodb_snapshot_isolation= ON;
+
+CREATE TABLE t1 (a INT) ENGINE=InnoDB;
+CREATE TABLE t2 (col_char CHAR(224), id SERIAL) ENGINE=InnoDB;
+CREATE TABLE t3 (id int PRIMARY KEY, c char) ENGINE=innodb;
+INSERT INTO t3 VALUES (1, 'a');
+
+--connect (con1,localhost,root,,)
+--connect (con2,localhost,root,,)
+--connect (con3,localhost,root,,)
+XA BEGIN 'x1';
+INSERT INTO t2 () VALUES ();
+
+--connection con2
+XA START 'x2';
+SELECT * FROM t1;
+--error ER_CHECKREAD
+UPDATE t2 SET col_char = 'b';
+
+--connection con1
+set debug_sync= 'alter_table_online_before_lock signal downgraded wait_for go';
+--send
+ALTER TABLE t3 ADD KEY (c), lock=none, algorithm = copy;
+
+--connection con2
+set debug_sync= 'now wait_for downgraded';
+DELETE FROM t3 WHERE id = 0;
+UPDATE t3 SET c = '';
+set debug_sync= 'now signal go';
+
+--connection con2
+--error ER_XA_RBROLLBACK
+XA END 'x2';
+XA ROLLBACK 'x2';
+--connection con1
+--reap
+--disconnect con1
+--disconnect con2
+--connection con3
+XA END 'x1';
+XA ROLLBACK 'x1';
+--disconnect con3
+
+--connection default
+DROP TABLE t1, t2, t3;
+SET GLOBAL innodb_snapshot_isolation= @isolation.save;
+
+--echo #
+--echo # End of 11.7 tests
+--echo #
--- a/sql/online_alter.cc
+++ b/sql/online_alter.cc
@ -29,8 +29,8 @@ static int online_alter_savepoint_rollback(THD *thd, sv_id_t sv_id);
 static int online_alter_commit(THD *thd, bool all);
 static int online_alter_rollback(THD *thd, bool all);
 static int online_alter_prepare(THD *thd, bool all);
-static int online_alter_commit_by_xid(XID *x);
-static int online_alter_rollback_by_xid(XID *x);
+static int online_alter_commit_by_xa(XA_data *xid, bool is_ending_trans);
+static int online_alter_rollback_by_xa(XA_data *xid, bool is_ending_trans);

 static transaction_participant online_alter_tp=
 {
@ -41,8 +41,8 @@ static transaction_participant online_alter_tp=
  NULL,                          /*savepoint_release*/
  online_alter_commit, online_alter_rollback, online_alter_prepare,
  [](XID*, uint){ return 0; },   /*recover*/
-  online_alter_commit_by_xid,
-  online_alter_rollback_by_xid,
+  [](XID *xid)->int { return online_alter_commit_by_xa((XA_data*)xid, true); },
+  [](XID *xid)->int { return online_alter_rollback_by_xa((XA_data*)xid, true);},
  NULL, NULL,
  NULL, NULL, NULL, NULL, NULL /* snapshot, *_ordered, checkpoint, versioned*/
 };
@ -132,6 +132,8 @@ online_alter_cache_data *setup_cache_data(MEM_ROOT *root, TABLE_SHARE *share)
 static Online_alter_cache_list &get_cache_list(transaction_participant *ht,
                                               THD *thd)
 {
+  if (thd->transaction->xid_state.is_explicit_XA())
+    return *thd->transaction->xid_state.get_xid()->online_alter_cache;
  void *data= thd_get_ha_data(thd, ht);
  DBUG_ASSERT(data);
  return *(Online_alter_cache_list*)data;
@ -140,6 +142,14 @@ static Online_alter_cache_list &get_cache_list(transaction_participant *ht,

 static Online_alter_cache_list &get_or_create_cache_list(THD *thd)
 {
+  if (thd->transaction->xid_state.is_explicit_XA())
+  {
+    XA_data *xa= thd->transaction->xid_state.get_xid();
+    if (!xa->online_alter_cache)
+      xa->online_alter_cache= new Online_alter_cache_list();
+    return *xa->online_alter_cache;
+  }
+
  void *data= thd_get_ha_data(thd, &online_alter_tp);
  if (!data)
  {
@ -232,9 +242,11 @@ int online_alter_end_trans(Online_alter_cache_list &cache_list, THD *thd,
  if (cache_list.empty())
    DBUG_RETURN(0);

+  Event_log *binlog= NULL;
+
  for (auto &cache: cache_list)
  {
-    auto *binlog= cache.sink_log;
+    binlog= cache.sink_log;
    DBUG_ASSERT(binlog);
    bool non_trans= cache.hton->flags & HTON_NO_ROLLBACK // Aria
                    || !cache.hton->rollback;
@ -245,7 +257,7 @@ int online_alter_end_trans(Online_alter_cache_list &cache_list, THD *thd,
      // Do not set STMT_END for last event to leave table open in altering thd
      error= binlog_flush_pending_rows_event(thd, false, true, binlog, &cache);
      if (error)
-        do_commit= commit= 0;
+        break;
    }

    if (do_commit)
@ -262,6 +274,9 @@ int online_alter_end_trans(Online_alter_cache_list &cache_list, THD *thd,
        mysql_mutex_unlock(binlog->get_log_lock());
        if (!is_ending_transaction)
          cache.reset();
+
+        if (error)
+          break;
      }
    }
    else if (!commit) // rollback
@ -274,15 +289,10 @@ int online_alter_end_trans(Online_alter_cache_list &cache_list, THD *thd,
      DBUG_ASSERT(!is_ending_transaction);
      cache.store_prev_position();
    }
-
+  }

  if (error)
-    {
-      my_error(ER_ERROR_ON_WRITE, MYF(ME_ERROR_LOG),
-               binlog->get_name(), errno);
-      break;
-    }
-  }
+    my_error(ER_ERROR_ON_WRITE, MYF(ME_ERROR_LOG), binlog->get_name(), errno);

  if (is_ending_transaction)
    cleanup_cache_list(cache_list);
@ -338,27 +348,33 @@ int online_alter_savepoint_rollback(THD *thd, sv_id_t sv_id)
  DBUG_RETURN(0);
 }

-static int online_alter_commit_by_xid(XID *x)
+inline
+static int online_alter_commit_by_xa(XA_data *xid, bool is_ending_trans)
 {
-  auto *xid= static_cast<XA_data*>(x);
-  if (likely(xid->online_alter_cache == NULL))
+  if (unlikely(xid->online_alter_cache == NULL))
    return 1;
  int res= online_alter_end_trans(*xid->online_alter_cache, current_thd,
                                  true, true);
+  if (is_ending_trans)
+  {
    delete xid->online_alter_cache;
    xid->online_alter_cache= NULL;
+  }
  return res;
 };

-static int online_alter_rollback_by_xid(XID *x)
+inline
+static int online_alter_rollback_by_xa(XA_data *xid, bool is_ending_trans)
 {
-  auto *xid= static_cast<XA_data*>(x);
-  if (likely(xid->online_alter_cache == NULL))
+  if (unlikely(xid->online_alter_cache == NULL))
    return 1;
  int res= online_alter_end_trans(*xid->online_alter_cache, current_thd,
-                                  true, false);
+                                  is_ending_trans, false);
+  if (is_ending_trans)
+  {
    delete xid->online_alter_cache;
    xid->online_alter_cache= NULL;
+  }
  return res;
 };

@ -366,18 +382,13 @@ static int online_alter_commit(THD *thd, bool all)
 {
  int res;
  bool is_ending_transaction= ending_trans(thd, all);
-  if (is_ending_transaction 
-      && thd->transaction->xid_state.get_state_code() == XA_PREPARED)
-  {
-    res= online_alter_commit_by_xid(thd->transaction->xid_state.get_xid());
-    // cleanup was already done by prepare()
-  }
+  if (thd->transaction->xid_state.is_explicit_XA())
+    res= online_alter_commit_by_xa(thd->transaction->xid_state.get_xid(),
+                                   is_ending_transaction);
  else
-  {
    res= online_alter_end_trans(get_cache_list(&online_alter_tp, thd), thd,
                                is_ending_transaction, true);
  cleanup_tables(thd);
-  }
  return res;
 };

@ -385,19 +396,13 @@ static int online_alter_rollback(THD *thd, bool all)
 {
  int res;
  bool is_ending_transaction= ending_trans(thd, all);
-  if (is_ending_transaction &&
-      (thd->transaction->xid_state.get_state_code() == XA_PREPARED ||
-       thd->transaction->xid_state.get_state_code() == XA_ROLLBACK_ONLY))
-  {
-    res= online_alter_rollback_by_xid(thd->transaction->xid_state.get_xid());
-    // cleanup was already done by prepare()
-  }
+  if (thd->transaction->xid_state.is_explicit_XA())
+    res= online_alter_rollback_by_xa(thd->transaction->xid_state.get_xid(),
+                                     is_ending_transaction);
  else
-  {
    res= online_alter_end_trans(get_cache_list(&online_alter_tp, thd), thd,
                                is_ending_transaction, false);
  cleanup_tables(thd);
-  }
  return res;
 };

@ -405,15 +410,8 @@ static int online_alter_prepare(THD *thd, bool all)
 {
  auto &cache_list= get_cache_list(&online_alter_tp, thd);
  int res= 0;
-  if (ending_trans(thd, all))
-  {
-    thd->transaction->xid_state.set_online_alter_cache(&cache_list);
-    thd_set_ha_data(thd, &online_alter_tp, NULL);
-  }
-  else
-  {
+  if (!ending_trans(thd, all))
    res= online_alter_end_trans(cache_list, thd, false, true);
-  }

  cleanup_tables(thd);
  return res;
--- a/sql/xa.cc
+++ b/sql/xa.cc
@ -225,7 +225,7 @@ bool XID_STATE::check_has_uncommitted_xa() const
 }


-XID *XID_STATE::get_xid() const
+XA_data *XID_STATE::get_xid() const
 {
  DBUG_ASSERT(is_explicit_XA());
  return &xid_cache_element->xid;
--- a/sql/xa.h
+++ b/sql/xa.h
@ -37,7 +37,7 @@ struct XID_STATE {
  void set_online_alter_cache(Online_alter_cache_list *);
  void set_rollback_only();
  void er_xaer_rmfail() const;
-  XID *get_xid() const;
+  XA_data *get_xid() const;
  enum xa_states get_state_code() const;
 };