MDEV-27239 Spider: Assertion `thd->transaction->stmt.ha_list == __null || trans == &thd->transaction->stmt' failed in ha_commit_trans on BEGIN WORK after FTWRL

The check on the SQL command type, in ha_spider::external_lock() is deleted
by e954d9d. This resulted in the wrong call of spider_internal_start_trx()
(and thus Ha_trx_info::register_ha()).

I reverted the check and refactored ha_spider::external_lock().

Author: nayuta-yanagisawa

storage/spider/ha_spider.cc

@@ -1182,75 +1182,83 @@ int ha_spider::external_lock(
   int error_num = 0;
   SPIDER_TRX *trx;
   backup_error_status();
+
   DBUG_ENTER("ha_spider::external_lock");
   DBUG_PRINT("info",("spider this=%p", this));
   DBUG_PRINT("info",("spider lock_type=%x", lock_type));
-#if MYSQL_VERSION_ID < 50500
-  DBUG_PRINT("info",("spider thd->options=%x", (int) thd->options));
-#endif
-#ifdef WITH_PARTITION_STORAGE_ENGINE
-  if (
-    wide_handler->stage == SPD_HND_STAGE_EXTERNAL_LOCK &&
-    wide_handler->stage_executor != this)
+  DBUG_PRINT("info", ("spider sql_command=%d", thd_sql_command(thd)));
+
+  if (wide_handler->stage == SPD_HND_STAGE_EXTERNAL_LOCK)
   {
-    DBUG_RETURN(0);
+    /* Only the stage executor deals with table locks. */
+    if (wide_handler->stage_executor != this)
+    {
+      DBUG_RETURN(0);
+    }
+  }
+  else
+  {
+    /* Update the stage executor when the stage changes */
+    wide_handler->stage= SPD_HND_STAGE_EXTERNAL_LOCK;
+    wide_handler->stage_executor= this;
   }
-  wide_handler->stage = SPD_HND_STAGE_EXTERNAL_LOCK;
-  wide_handler->stage_executor = this;
-#endif
-#ifdef HANDLER_HAS_NEED_INFO_FOR_AUTO_INC
-  info_auto_called = FALSE;
-#endif
 
+  info_auto_called = FALSE;
+  wide_handler->external_lock_type= lock_type;
   wide_handler->sql_command = thd_sql_command(thd);
+
+  /* We treat BEGIN as if UNLOCK TABLE. */
   if (wide_handler->sql_command == SQLCOM_BEGIN)
+  {
     wide_handler->sql_command = SQLCOM_UNLOCK_TABLES;
+  }
+  if (lock_type == F_UNLCK &&
+      wide_handler->sql_command != SQLCOM_UNLOCK_TABLES)
+  {
+    DBUG_RETURN(0);
+  }
 
   trx = spider_get_trx(thd, TRUE, &error_num);
   if (error_num)
+  {
     DBUG_RETURN(error_num);
+  }
   wide_handler->trx = trx;
 
-  DBUG_PRINT("info",("spider sql_command=%d", wide_handler->sql_command));
-#ifdef HA_CAN_BULK_ACCESS
-  wide_handler->external_lock_cnt++;
-#endif
-  if (
-    lock_type == F_UNLCK &&
-    wide_handler->sql_command != SQLCOM_UNLOCK_TABLES
-  )
-    DBUG_RETURN(0);
+  /* Question: Why the following if block is necessary? Why here? */
   if (store_error_num)
+  {
     DBUG_RETURN(store_error_num);
-  wide_handler->external_lock_type = lock_type;
-#if defined(HS_HAS_SQLCOM) && defined(HAVE_HANDLERSOCKET)
-  if ((conn_kinds & SPIDER_CONN_KIND_MYSQL))
+  }
+
+  DBUG_ASSERT(wide_handler->sql_command != SQLCOM_RENAME_TABLE &&
+              wide_handler->sql_command != SQLCOM_DROP_DB);
+
+  if (wide_handler->sql_command == SQLCOM_DROP_TABLE ||
+      wide_handler->sql_command == SQLCOM_ALTER_TABLE)
   {
-#endif
-    if (
-      /* SQLCOM_RENAME_TABLE and SQLCOM_DROP_DB don't come here */
-      wide_handler->sql_command == SQLCOM_DROP_TABLE ||
-      wide_handler->sql_command == SQLCOM_ALTER_TABLE
-    ) {
-      if (trx->locked_connections)
-      {
-        my_message(ER_SPIDER_ALTER_BEFORE_UNLOCK_NUM,
-          ER_SPIDER_ALTER_BEFORE_UNLOCK_STR, MYF(0));
-        DBUG_RETURN(ER_SPIDER_ALTER_BEFORE_UNLOCK_NUM);
-      }
-      DBUG_RETURN(0);
+    if (trx->locked_connections)
+    {
+      my_message(ER_SPIDER_ALTER_BEFORE_UNLOCK_NUM,
+                 ER_SPIDER_ALTER_BEFORE_UNLOCK_STR, MYF(0));
+      DBUG_RETURN(ER_SPIDER_ALTER_BEFORE_UNLOCK_NUM);
     }
-    if (unlikely((error_num = spider_internal_start_trx(this))))
+    DBUG_RETURN(0);
+  }
+
+  if (lock_type != F_UNLCK)
+  {
+    if (unlikely((error_num= spider_internal_start_trx(this))))
     {
       DBUG_RETURN(error_num);
     }
-#if defined(HS_HAS_SQLCOM) && defined(HAVE_HANDLERSOCKET)
-  } else {
-    trans_register_ha(trx->thd, FALSE, spider_hton_ptr);
-    if (thd_test_options(trx->thd, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
-      trans_register_ha(trx->thd, TRUE, spider_hton_ptr);
+    if (wide_handler->sql_command != SQLCOM_SELECT &&
+        wide_handler->sql_command != SQLCOM_HA_READ)
+    {
+      trx->updated_in_this_trx= TRUE;
+      DBUG_PRINT("info", ("spider trx->updated_in_this_trx=TRUE"));
+    }
   }
-#endif
 
   if (wide_handler->lock_table_type > 0 ||
     wide_handler->sql_command == SQLCOM_UNLOCK_TABLES)
@@ -1263,67 +1271,24 @@ int ha_spider::external_lock(
     }
 
     /* lock/unlock tables */
-#ifdef WITH_PARTITION_STORAGE_ENGINE
     if (partition_handler && partition_handler->handlers)
     {
-      uint roop_count;
-      for (roop_count = 0; roop_count < partition_handler->no_parts;
-        ++roop_count)
+      for (uint roop_count= 0; roop_count < partition_handler->no_parts;
+           ++roop_count)
       {
         if (unlikely((error_num =
           partition_handler->handlers[roop_count]->lock_tables())))
         {
           DBUG_RETURN(error_num);
         }
       }
-    } else {
-#endif
-      if (unlikely((error_num = lock_tables())))
-      {
-        DBUG_RETURN(error_num);
-      }
-#ifdef WITH_PARTITION_STORAGE_ENGINE
     }
-#endif
-  }
-
-  DBUG_PRINT("info",("spider trx_start=%s",
-    trx->trx_start ? "TRUE" : "FALSE"));
-  /* need to check after spider_internal_start_trx() */
-  if (trx->trx_start)
-  {
-    switch (wide_handler->sql_command)
+    else if (unlikely((error_num= lock_tables())))
     {
-      case SQLCOM_SELECT:
-      case SQLCOM_HA_READ:
-#ifdef HS_HAS_SQLCOM
-      case SQLCOM_HS_READ:
-#endif
-        /* nothing to do */
-        break;
-      case SQLCOM_UPDATE:
-      case SQLCOM_UPDATE_MULTI:
-#ifdef HS_HAS_SQLCOM
-      case SQLCOM_HS_UPDATE:
-#endif
-      case SQLCOM_CREATE_TABLE:
-      case SQLCOM_INSERT:
-      case SQLCOM_INSERT_SELECT:
-      case SQLCOM_DELETE:
-      case SQLCOM_LOAD:
-      case SQLCOM_REPLACE:
-      case SQLCOM_REPLACE_SELECT:
-      case SQLCOM_DELETE_MULTI:
-#ifdef HS_HAS_SQLCOM
-      case SQLCOM_HS_INSERT:
-      case SQLCOM_HS_DELETE:
-#endif
-      default:
-        trx->updated_in_this_trx = TRUE;
-        DBUG_PRINT("info",("spider trx->updated_in_this_trx=TRUE"));
-        break;
+      DBUG_RETURN(error_num);
     }
   }
+
   DBUG_RETURN(0);
 }
 

storage/spider/mysql-test/spider/bugfix/r/mdev_27239.result

@@ -0,0 +1,20 @@
+#
+# MDEV-27239 Spider: Assertion `thd->transaction->stmt.ha_list == __null || trans == &thd->transaction->stmt' failed in ha_commit_trans on BEGIN WORK after FTWRL
+#
+for master_1
+for child2
+for child3
+CREATE DATABASE auto_test_local;
+USE auto_test_local;
+CREATE TABLE tbl_a (a INT) ENGINE=SPIDER;
+FLUSH TABLE tbl_a WITH READ LOCK;
+Warnings:
+Error	1429	Unable to connect to foreign data source: localhost
+Error	1429	Unable to connect to foreign data source: localhost
+Error	1429	Unable to connect to foreign data source: localhost
+Error	1429	Unable to connect to foreign data source: localhost
+BEGIN;
+DROP DATABASE auto_test_local;
+for master_1
+for child2
+for child3

storage/spider/mysql-test/spider/bugfix/t/mdev_27239.cnf

@@ -0,0 +1,2 @@
+!include include/default_mysqld.cnf
+!include ../my_1_1.cnf

storage/spider/mysql-test/spider/bugfix/t/mdev_27239.test

@@ -0,0 +1,24 @@
+--echo #
+--echo # MDEV-27239 Spider: Assertion `thd->transaction->stmt.ha_list == __null || trans == &thd->transaction->stmt' failed in ha_commit_trans on BEGIN WORK after FTWRL
+--echo #
+
+--disable_query_log
+--disable_result_log
+--source ../../t/test_init.inc
+--enable_result_log
+--enable_query_log
+
+CREATE DATABASE auto_test_local;
+USE auto_test_local;
+
+CREATE TABLE tbl_a (a INT) ENGINE=SPIDER;
+FLUSH TABLE tbl_a WITH READ LOCK;
+BEGIN;
+
+DROP DATABASE auto_test_local;
+
+--disable_query_log
+--disable_result_log
+--source ../../t/test_deinit.inc
+--enable_result_log
+--enable_query_log