Fix that BACKUP STAGE BLOCK_COMMIT blocks commit to the Aria engine

MDEV-22468 BACKUP STAGE BLOCK_COMMIT should block commit in the Aria engine This is needed to ensure that mariabackup works properly with Aria tables This code ads new calls to ha_maria::implicit_commit(). These will be deleted by MDEV-22531 Remove maria::implicit_commit().

Author: montywi

mysql-test/main/backup_lock.result

@@ -192,8 +192,10 @@ BACKUP STAGE END;
 SET GLOBAL lock_wait_timeout=0;
 CREATE TABLE t_permanent_innodb (col1 INT) ENGINE = InnoDB;
 CREATE TABLE t_permanent_myisam (col1 INT) ENGINE = MyISAM;
+CREATE TABLE t_permanent_aria (col1 INT) ENGINE = Aria transactional=1;
 INSERT INTO t_permanent_innodb SET col1 = 1;
 INSERT INTO t_permanent_myisam SET col1 = 1;
+INSERT INTO t_permanent_aria SET col1 = 1;
 CREATE TABLE t_con1_innodb (col1 INT) ENGINE = InnoDB;
 CREATE TABLE t_con1_myisam (col1 INT) ENGINE = MyISAM;
 connect con1,localhost,root,,;
@@ -207,13 +209,23 @@ connection con1;
 UPDATE t_permanent_innodb SET col1 = 8;
 UPDATE t_permanent_myisam SET col1 = 8;
 ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+UPDATE t_permanent_aria SET col1 = 8;
 DROP TABLE t_con1_innodb;
 ERROR HY000: Lock wait timeout exceeded; try restarting transaction
 DROP TABLE t_con1_myisam;
 ERROR HY000: Lock wait timeout exceeded; try restarting transaction
 connection default;
 BACKUP STAGE END;
-DROP TABLE t_permanent_myisam, t_permanent_innodb;
+select * from t_permanent_innodb;
+col1
+1
+select * from t_permanent_myisam;
+col1
+1
+select * from t_permanent_aria;
+col1
+8
+DROP TABLE t_permanent_myisam, t_permanent_innodb, t_permanent_aria;
 DROP TABLE t_con1_innodb, t_con1_myisam;
 disconnect con1;
 set global lock_wait_timeout=default;

mysql-test/main/backup_lock.test

@@ -251,9 +251,11 @@ BACKUP STAGE END;
 SET GLOBAL lock_wait_timeout=0;
 CREATE TABLE t_permanent_innodb (col1 INT) ENGINE = InnoDB;
 CREATE TABLE t_permanent_myisam (col1 INT) ENGINE = MyISAM;
+CREATE TABLE t_permanent_aria (col1 INT) ENGINE = Aria transactional=1;
 INSERT INTO t_permanent_innodb SET col1 = 1;
-
 INSERT INTO t_permanent_myisam SET col1 = 1;
+INSERT INTO t_permanent_aria SET col1 = 1;
+
 CREATE TABLE t_con1_innodb (col1 INT) ENGINE = InnoDB;
 CREATE TABLE t_con1_myisam (col1 INT) ENGINE = MyISAM;
 
@@ -270,6 +272,8 @@ BACKUP STAGE BLOCK_COMMIT;
 UPDATE t_permanent_innodb SET col1 = 8;
 --error ER_LOCK_WAIT_TIMEOUT
 UPDATE t_permanent_myisam SET col1 = 8;
+UPDATE t_permanent_aria SET col1 = 8;
+
 --error ER_LOCK_WAIT_TIMEOUT
 DROP TABLE t_con1_innodb;
 
@@ -278,7 +282,12 @@ DROP TABLE t_con1_myisam;
 
 --connection default
 BACKUP STAGE END;
-DROP TABLE t_permanent_myisam, t_permanent_innodb;
+
+select * from t_permanent_innodb;
+select * from t_permanent_myisam;
+select * from t_permanent_aria;
+
+DROP TABLE t_permanent_myisam, t_permanent_innodb, t_permanent_aria;
 DROP TABLE t_con1_innodb, t_con1_myisam;
 --disconnect con1
 set global lock_wait_timeout=default;

sql/handler.cc

@@ -1531,16 +1531,6 @@ int ha_commit_trans(THD *thd, bool all)
  DBUG_RETURN(2);
  }
 
-#ifdef WITH_ARIA_STORAGE_ENGINE
- if ((error= ha_maria::implicit_commit(thd, TRUE)))
- {
- my_error(ER_ERROR_DURING_COMMIT, MYF(0), error);
- ha_rollback_trans(thd, all);
- DBUG_RETURN(1);
- }
-
-#endif
-
  if (!ha_info)
  {
  /*
@@ -1556,6 +1546,16 @@ int ha_commit_trans(THD *thd, bool all)
  if (wsrep_is_active(thd) && is_real_trans && !error)
  wsrep_commit_empty(thd, all);
 #endif /* WITH_WSREP */
+
+#if defined(WITH_ARIA_STORAGE_ENGINE)
+ /* This is needed to ensure that repair commits properly */
+ if ((error= ha_maria::implicit_commit(thd, TRUE)))
+ {
+ my_error(ER_ERROR_DURING_COMMIT, MYF(0), error);
+ ha_rollback_trans(thd, all);
+ DBUG_RETURN(1);
+ }
+#endif
  DBUG_RETURN(0);
  }
 
@@ -1570,10 +1570,16 @@ int ha_commit_trans(THD *thd, bool all)
  bool rw_trans= is_real_trans &&
  (rw_ha_count > (thd->is_current_stmt_binlog_disabled()?0U:1U));
  MDL_request mdl_request;
+ mdl_request.ticket= 0;
  DBUG_PRINT("info", ("is_real_trans: %d rw_trans: %d rw_ha_count: %d",
  is_real_trans, rw_trans, rw_ha_count));
 
- if (rw_trans)
+ /*
+ We need to test maria_hton because of plugin_innodb.test that changes
+ the plugin table to innodb and thus plugin_load will call
+ mysql_close_tables() which calls trans_commit_trans() with maria_hton = 0
+ */
+ if (rw_trans || (likely(maria_hton) && thd_get_ha_data(thd, maria_hton)))
  {
  /*
  Acquire a metadata lock which will ensure that COMMIT is blocked
@@ -1587,15 +1593,22 @@ int ha_commit_trans(THD *thd, bool all)
  MDL_EXPLICIT);
 
  if (!WSREP(thd) &&
- thd->mdl_context.acquire_lock(&mdl_request,
- thd->variables.lock_wait_timeout))
+  thd->mdl_context.acquire_lock(&mdl_request,
+  thd->variables.lock_wait_timeout))
  {
  ha_rollback_trans(thd, all);
  DBUG_RETURN(1);
  }
 
  DEBUG_SYNC(thd, "ha_commit_trans_after_acquire_commit_lock");
  }
+#if defined(WITH_ARIA_STORAGE_ENGINE)
+ if ((error= ha_maria::implicit_commit(thd, TRUE)))
+ {
+ my_error(ER_ERROR_DURING_COMMIT, MYF(0), error);
+ goto err;
+ }
+#endif
 
  if (rw_trans &&
  opt_readonly &&
@@ -1797,7 +1810,7 @@ int ha_commit_trans(THD *thd, bool all)
  thd->rgi_slave->is_parallel_exec);
  }
 end:
- if (rw_trans && mdl_request.ticket)
+ if (mdl_request.ticket)
  {
  /*
  We do not always immediately release transactional locks

sql/handler.h

@@ -70,6 +70,7 @@ class Column_definition;
 #define HA_ADMIN_NEEDS_UPGRADE -10
 #define HA_ADMIN_NEEDS_ALTER -11
 #define HA_ADMIN_NEEDS_CHECK -12
+#define HA_ADMIN_COMMIT_ERROR -13
 
 /**
  Return values for check_if_supported_inplace_alter().

sql/sql_parse.cc

@@ -1508,6 +1508,67 @@ uint maria_multi_check(THD *thd, char *packet, size_t packet_length)
 }
 
 
+#if defined(WITH_ARIA_STORAGE_ENGINE)
+class Silence_all_errors : public Internal_error_handler
+{
+ char m_message[MYSQL_ERRMSG_SIZE];
+ int error;
+public:
+ Silence_all_errors():error(0) {}
+ virtual ~Silence_all_errors() {}
+
+ virtual bool handle_condition(THD *thd,
+ uint sql_errno,
+ const char* sql_state,
+ Sql_condition::enum_warning_level *level,
+ const char* msg,
+ Sql_condition ** cond_hdl)
+ {
+ error= sql_errno;
+ *cond_hdl= NULL;
+ strmake_buf(m_message, msg);
+ return true; // Error handled
+ }
+};
+#endif
+
+/*
+ Do an implict commit into the Aria storage engine
+*/
+
+static inline my_bool aria_implicit_commit(THD *thd)
+{
+#if defined(WITH_ARIA_STORAGE_ENGINE)
+ if (thd_get_ha_data(thd, maria_hton))
+ {
+ MDL_request mdl_request;
+ bool locked;
+ int res;
+ Silence_all_errors error_handler;
+ DBUG_ASSERT(maria_hton);
+
+ MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT,
+ MDL_EXPLICIT);
+ /*
+ We have to ignore any errors from acquire_lock and continue even if we
+ don't get the lock as Aria can't roll back!
+ This function is also called in some cases when the message is already
+ sent to the user, so we can't even send a warning.
+ */
+ thd->push_internal_handler(& error_handler);
+ locked= !thd->mdl_context.acquire_lock(&mdl_request,
+ thd->variables.lock_wait_timeout);
+ thd->pop_internal_handler();
+ res= ha_maria::implicit_commit(thd, FALSE);
+ if (locked)
+ thd->mdl_context.release_lock(mdl_request.ticket);
+ return res;
+ }
+#endif
+ return 0;
+}
+
+
 /**
  Perform one connection-level (COM_XXXX) command.
 
@@ -1860,9 +1921,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
  */
  char *beginning_of_next_stmt= (char*) parser_state.m_lip.found_semicolon;
 
-#ifdef WITH_ARIA_STORAGE_ENGINE
- ha_maria::implicit_commit(thd, FALSE);
-#endif
+ aria_implicit_commit(thd);
 
  /* Finalize server status flags after executing a statement. */
  thd->update_server_status();
@@ -5990,9 +6049,7 @@ mysql_execute_command(THD *thd)
  trans_commit_stmt(thd);
  thd->get_stmt_da()->set_overwrite_status(false);
  }
-#ifdef WITH_ARIA_STORAGE_ENGINE
- ha_maria::implicit_commit(thd, FALSE);
-#endif
+ aria_implicit_commit(thd);
  }
 
  /* Free tables. Set stage 'closing tables' */

storage/maria/ha_maria.cc

@@ -1477,6 +1477,13 @@ int ha_maria::repair(THD * thd, HA_CHECK_OPT *check_opt)
  }
  break;
  }
+ /*
+ Commit is needed in the case of tables are locked to ensure that repair
+ is registered in the recovery log
+ */
+ if (implicit_commit(thd, TRUE))
+ error= HA_ADMIN_COMMIT_ERROR;
+
  if (!error && start_records != file->state->records &&
  !(check_opt->flags & T_VERY_SILENT))
  {