MDEV-20225 BF aborting SP execution (#1394)

* MDEV-20225 BF aborting SP execution

When stored procedure execution was chosen as victim for a BF abort, the old implemnetationn called for rollback immediately
when execution was inside SP isntruction. Technically this happened in wsrep_after_statement() call, which identified the
need for a rollback.
The problem was that MariaDB does not accept rollback (nor commit) inside sub statement, there are several asserts about it,
checking for THD::in_sub_stmt.

This patch contains a fix, which skips calling wsrep_after_statement() for SP execution, which is marked as BF must abort. Instead,
we return error code to upper level, where rollback will eventually happen, ouside of SP execution.
Also, appending the affected trigger table (dropped or created) in the populated key set for the write set,
which prevents parallel applying of other transactions working on the same table.

* MDEV-20225 BF aborting SP execution, second patch

First PR missed 4 commits, which are now squashed in this patch:
- Added galera_sp_bf_abort test.
  A MTR test case which will reproduce BF-BF conflict if all keys
  corresponding to affected tables are not assigned for DROP TRIGGER.
- Fixed incorrect use of sync pointsin MDEV-20225
- Added condition for SQLCOM_DROP_TRIGGER in wsrep_can_run_in_toi()
  to make it replicate.

* MDEV-20225 BF aborting SP execution, third patch

The galera_trigger.test caused a situation, where SP invocation caused a trigger
to fire, and the trigger executed as sub statement SP, and was BF aborted by applier.
because of wsrep_after_statement() was called for the sub-statement level, it ended up
in exeuting rollback and asserted there.
Thus fix will catch sub-statement level SP execution, and avoids calling wsrep_after_statement()

mysql-test/suite/galera/r/MDEV-20225.result

+connection node_2;
+connection node_1;
+CREATE TABLE t1 (f1 INT NOT NULL PRIMARY KEY AUTO_INCREMENT) ENGINE=InnoDB;
+CREATE TABLE t2 (f1 INT NOT NULL PRIMARY KEY AUTO_INCREMENT, f2 INT) ENGINE=InnoDB;
+CREATE TRIGGER tr1 BEFORE INSERT ON t1 FOR EACH ROW INSERT INTO t2 VALUES (NULL, NEW.f1);
+connection node_2;
+SET SESSION wsrep_sync_wait = 0;
+SET GLOBAL wsrep_slave_threads = 2;
+SET GLOBAL debug_dbug = 'd,sync.mdev_20225';
+DROP TRIGGER tr1;
+connection node_2;
+connection node_1;
+INSERT INTO t1 VALUES (NULL);
+connection node_2;
+SET GLOBAL debug_dbug = 'RESET';
+SET DEBUG_SYNC = 'now SIGNAL signal.mdev_20225_continue';
+SET DEBUG_SYNC = 'RESET';
+SET GLOBAL wsrep_slave_threads = 1;
+SHOW TRIGGERS;
+Trigger	Event	Table	Statement	Timing	Created	sql_mode	Definer	character_set_client	collation_connection	Database Collation
+DROP TABLE t1;
+DROP TABLE t2;

mysql-test/suite/galera/t/MDEV-20225.test

+#
+# MDEV-20225 - Verify that DROP TRIGGER gets keys assigned corresponding
+#              to all affected tables.
+#
+
+--source include/galera_cluster.inc
+--source include/have_innodb.inc
+
+CREATE TABLE t1 (f1 INT NOT NULL PRIMARY KEY AUTO_INCREMENT) ENGINE=InnoDB;
+CREATE TABLE t2 (f1 INT NOT NULL PRIMARY KEY AUTO_INCREMENT, f2 INT) ENGINE=InnoDB;
+
+CREATE TRIGGER tr1 BEFORE INSERT ON t1 FOR EACH ROW INSERT INTO t2 VALUES (NULL, NEW.f1);
+
+--connection node_2
+SET SESSION wsrep_sync_wait = 0;
+SET GLOBAL wsrep_slave_threads = 2;
+SET GLOBAL debug_dbug = 'd,sync.mdev_20225';
+
+--let $galera_connection_name = node_1a
+--let $galera_server_number = 1
+--source include/galera_connect.inc
+DROP TRIGGER tr1;
+
+--connection node_2
+--let $wait_condition = SELECT COUNT(*) = 1 FROM INFORMATION_SCHEMA.PROCESSLIST WHERE STATE = 'debug sync point: now'
+--source include/wait_condition.inc
+
+
+--connection node_1
+INSERT INTO t1 VALUES (NULL);
+# We must rely on sleep here. If the bug is fixed, the second applier
+# is not allowed to go past apply monitor which would trigger the bug,
+# so there is no sync point or condition to wait.
+--sleep 1
+
+--connection node_2
+SET GLOBAL debug_dbug = 'RESET';
+SET DEBUG_SYNC = 'now SIGNAL signal.mdev_20225_continue';
+SET DEBUG_SYNC = 'RESET';
+SET GLOBAL wsrep_slave_threads = 1;
+
+# Trigger should now be dropped on node_2.
+SHOW TRIGGERS;
+
+DROP TABLE t1;
+DROP TABLE t2;

mysql-test/suite/galera/t/galera_sp_bf_abort.inc

+
+#
+# Issue an INSERT for gap between 1 and 3 to node_2 and wait until it hits
+# apply monitor sync point on node_1
+#
+
+--connection node_1a
+--let $galera_sync_point = apply_monitor_slave_enter_sync
+--source include/galera_set_sync_point.inc
+
+--connection node_2
+--eval $galera_sp_bf_abort_conflict
+
+--connection node_1a
+--source include/galera_wait_sync_point.inc
+--source include/galera_clear_sync_point.inc
+
+# Send a procedure to node_1 which should take a gap lock between
+# rows 1 and 3. It does not conflict with INSERT from node_2 in
+# certification. Park the UPDATE after replicate and let INSERT to
+# continue applying, generating a BF abort.
+
+--let $galera_sync_point = after_replicate_sync
+--source include/galera_set_sync_point.inc
+
+--connection node_1
+--send_eval CALL $galera_sp_bf_abort_proc
+
+--connection node_1a
+--let $galera_sync_point = after_replicate_sync apply_monitor_slave_enter_sync
+--source include/galera_wait_sync_point.inc
+--source include/galera_clear_sync_point.inc
+
+--let $galera_sync_point = apply_monitor_slave_enter_sync
+--source include/galera_signal_sync_point.inc
+--let $galera_sync_point = after_replicate_sync
+--source include/galera_signal_sync_point.inc
+

sql/sp_head.cc

@@ -1345,6 +1345,69 @@ sp_head::execute(THD *thd, bool merge_da_on_success)
 #endif /* WITH_WSREP */
     err_status= i->execute(thd, &ip);
 
+#ifdef WITH_WSREP
+    if (WSREP(thd))
+    {
+      if (((thd->wsrep_trx().state() == wsrep::transaction::s_executing || thd->in_sub_stmt) &&
+           (thd->is_fatal_error || thd->killed)))
+      {
+        WSREP_DEBUG("SP abort err status %d in sub %d trx state %d",
+                    err_status, thd->in_sub_stmt, thd->wsrep_trx().state());
+        err_status= 1;
+        thd->is_fatal_error= 1;
+        /*
+          SP was killed, and it is not due to a wsrep conflict.
+          We skip after_command hook at this point because
+          otherwise it clears the error, and cleans up the
+          whole transaction. For now we just return and finish
+          our handling once we are back to mysql_parse.
+
+          Same applies to a SP execution, which was aborted due
+          to wsrep related conflict, but which is executing as sub statement.
+          SP in sub statement level should not commit not rollback,
+          we have to call for rollback is up-most SP level.
+        */
+        WSREP_DEBUG("Skipping after_command hook for killed SP");
+      }
+      else
+      {
+        const bool must_replay= wsrep_must_replay(thd);
+        if (must_replay)
+        {
+          WSREP_DEBUG("MUST_REPLAY set after SP, err_status %d trx state: %d",
+                      err_status, thd->wsrep_trx().state());
+        }
+        (void) wsrep_after_statement(thd);
+
+        /*
+          Reset the return code to zero if the transaction was
+          replayed succesfully.
+        */
+        if (must_replay && !wsrep_current_error(thd))
+        {
+          err_status= 0;
+          thd->get_stmt_da()->reset_diagnostics_area();
+        }
+        /*
+          Final wsrep error status for statement is known only after
+          wsrep_after_statement() call. If the error is set, override
+          error in thd diagnostics area and reset wsrep client_state error
+          so that the error does not get propagated via client-server protocol.
+        */
+        if (wsrep_current_error(thd))
+        {
+          wsrep_override_error(thd, wsrep_current_error(thd),
+                               wsrep_current_error_status(thd));
+          thd->wsrep_cs().reset_error();
+          /* Reset also thd->killed if it has been set during BF abort. */
+          if (thd->killed == KILL_QUERY)
+            thd->killed= NOT_KILLED;
+          /* if failed transaction was not replayed, must return with error from here */
+          if (!must_replay) err_status = 1;
+        }
+      }
+    }
+#endif /* WITH_WSREP */
     thd->m_digest= parent_digest;
 
     if (i->free_list)
@@ -3605,49 +3668,6 @@ sp_instr_stmt::exec_core(THD *thd, uint *nextp)
                          (char *)thd->security_ctx->host_or_ip,
                          3);
   int res= mysql_execute_command(thd);
-#ifdef WITH_WSREP
-  if (WSREP(thd))
-  {
-    if ((thd->is_fatal_error || thd->killed_errno()) &&
-        (thd->wsrep_trx().state() == wsrep::transaction::s_executing))
-    {
-      /*
-        SP was killed, and it is not due to a wsrep conflict.
-        We skip after_statement hook at this point because
-        otherwise it clears the error, and cleans up the
-        whole transaction. For now we just return and finish
-        our handling once we are back to mysql_parse.
-      */
-      WSREP_DEBUG("Skipping after_command hook for killed SP");
-    }
-    else
-    {
-      const bool must_replay= wsrep_must_replay(thd);
-      (void) wsrep_after_statement(thd);
-      /*
-        Reset the return code to zero if the transaction was
-        replayed succesfully.
-      */
-      if (res && must_replay && !wsrep_current_error(thd))
-        res= 0;
-      /*
-        Final wsrep error status for statement is known only after
-        wsrep_after_statement() call. If the error is set, override
-        error in thd diagnostics area and reset wsrep client_state error
-        so that the error does not get propagated via client-server protocol.
-      */
-      if (wsrep_current_error(thd))
-      {
-        wsrep_override_error(thd, wsrep_current_error(thd),
-                             wsrep_current_error_status(thd));
-        thd->wsrep_cs().reset_error();
-        /* Reset also thd->killed if it has been set during BF abort. */
-        if (thd->killed == KILL_QUERY)
-          thd->reset_killed();
-      }
-    }
-  }
-#endif /* WITH_WSREP */
   MYSQL_QUERY_EXEC_DONE(res);
   *nextp= m_ip+1;
   return res;

sql/sql_trigger.cc

@@ -34,6 +34,7 @@
 #include "sql_handler.h"                        // mysql_ha_rm_tables
 #include "sp_cache.h"                     // sp_invalidate_cache
 #include <mysys_err.h>
+#include "debug_sync.h"
 
 /*************************************************************************/
 
@@ -507,7 +508,7 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create)
   }
 
 #ifdef WITH_WSREP
-  WSREP_TO_ISOLATION_BEGIN(WSREP_MYSQL_DB, NULL, NULL);
+  WSREP_TO_ISOLATION_BEGIN(WSREP_MYSQL_DB, NULL, tables);
 #endif
 
   /* We should have only one table in table list. */
@@ -569,6 +570,17 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create)
       goto end;
   }
 
+#ifdef WITH_WSREP
+  DBUG_EXECUTE_IF("sync.mdev_20225",
+                  {
+                    const char act[]=
+                      "now "
+                      "wait_for signal.mdev_20225_continue";
+                    DBUG_ASSERT(!debug_sync_set_action(thd,
+                                                       STRING_WITH_LEN(act)));
+                  };);
+#endif /* WITH_WSREP */
+
   result= (create ?
            table->triggers->create_trigger(thd, tables, &stmt_query):
            table->triggers->drop_trigger(thd, tables, &stmt_query));

sql/wsrep_mysqld.cc

@@ -1669,7 +1669,6 @@ static bool wsrep_can_run_in_toi(THD *thd, const char *db, const char *table,
 
   case SQLCOM_CREATE_TRIGGER:
 
-    DBUG_ASSERT(!table_list);
     DBUG_ASSERT(first_table);
 
     if (thd->find_temporary_table(first_table))
@@ -1678,6 +1677,14 @@ static bool wsrep_can_run_in_toi(THD *thd, const char *db, const char *table,
     }
     return true;
 
+  case SQLCOM_DROP_TRIGGER:
+    DBUG_ASSERT(table_list);
+    if (thd->find_temporary_table(table_list))
+    {
+      return false;
+    }
+    return true;
+
   default:
     if (table && !thd->find_temporary_table(db, table))
     {