MDEV-15061 TRUNCATE must honor InnoDB table locks

Traditionally, DROP TABLE and TRUNCATE TABLE discarded any locks that
may have been held on the table. This feels like an ACID violation.
Probably most occurrences of it were prevented by meta-data locks (MDL)
which were introduced in MySQL 5.5.

dict_table_t::n_foreign_key_checks_running: Reduce the number of
non-debug checks.

lock_remove_all_on_table(), lock_remove_all_on_table_for_trx(): Remove.

ha_innobase::truncate(): Acquire an exclusive InnoDB table lock
before proceeding. DROP TABLE and DISCARD/IMPORT were already doing
this.

row_truncate_table_for_mysql(): Convert the already started transaction
into a dictionary operation, and do not invoke lock_remove_all_on_table().

row_mysql_table_id_reassign(): Do not call lock_remove_all_on_table().
This function is only used in ALTER TABLE...DISCARD/IMPORT TABLESPACE,
which is already holding an exclusive InnoDB table lock.

TODO: Make n_foreign_key_checks running a debug-only variable.
This would require two fixes:
(1) DROP TABLE: Exclusively lock the table beforehand, to prevent
the possibility of concurrently running foreign key checks (which
would acquire a table IS lock and then record S locks).
(2) RENAME TABLE: Find out if n_foreign_key_checks_running>0 actually
constitutes a potential problem.

sql/share/errmsg-utf8.txt

@@ -6899,8 +6899,8 @@ ER_COL_COUNT_DOESNT_MATCH_CORRUPTED_V2
 ER_SLAVE_SILENT_RETRY_TRANSACTION
   eng "Slave must silently retry current transaction"
 
-ER_DISCARD_FK_CHECKS_RUNNING
-  eng "There is a foreign key check running on table '%-.192s'. Cannot discard the table"
+ER_UNUSED_22
+  eng "You should never see it"
 
 ER_TABLE_SCHEMA_MISMATCH
   eng "Schema mismatch (%s)"

storage/innobase/handler/ha_innodb.cc

@@ -13348,18 +13348,26 @@ ha_innobase::truncate()
 
 	TrxInInnoDB	trx_in_innodb(m_prebuilt->trx);
 
-	if (!trx_is_started(m_prebuilt->trx)) {
-		++m_prebuilt->trx->will_lock;
-	}
-
-	dberr_t	err;
-
-	/* Truncate the table in InnoDB */
-	err = row_truncate_table_for_mysql(m_prebuilt->table, m_prebuilt->trx);
+	m_prebuilt->trx->ddl = true;
+	trx_start_if_not_started(m_prebuilt->trx, true);
 
-	int	error;
+	dberr_t	err = row_mysql_lock_table(m_prebuilt->trx, m_prebuilt->table,
+					   LOCK_X, "truncate table");
+	if (err == DB_SUCCESS) {
+		err = row_truncate_table_for_mysql(m_prebuilt->table,
+						   m_prebuilt->trx);
+	}
 
 	switch (err) {
+	case DB_FORCED_ABORT:
+	case DB_DEADLOCK:
+		thd_mark_transaction_to_rollback(m_user_thd, 1);
+		DBUG_RETURN(HA_ERR_LOCK_DEADLOCK);
+	case DB_LOCK_TABLE_FULL:
+		thd_mark_transaction_to_rollback(m_user_thd, 1);
+		DBUG_RETURN(HA_ERR_LOCK_TABLE_FULL);
+	case DB_LOCK_WAIT_TIMEOUT:
+		DBUG_RETURN(HA_ERR_LOCK_WAIT_TIMEOUT);
 	case DB_TABLESPACE_DELETED:
 	case DB_TABLESPACE_NOT_FOUND:
 		ib_senderrf(
@@ -13368,19 +13376,14 @@ ha_innobase::truncate()
 			ER_TABLESPACE_DISCARDED : ER_TABLESPACE_MISSING),
 			table->s->table_name.str);
 		table->status = STATUS_NOT_FOUND;
-		error = HA_ERR_TABLESPACE_MISSING;
-		break;
-
+		DBUG_RETURN(HA_ERR_TABLESPACE_MISSING);
 	default:
-		error = convert_error_code_to_mysql(
-			err, m_prebuilt->table->flags,
-			m_prebuilt->trx->mysql_thd);
-
 		table->status = STATUS_NOT_FOUND;
+		DBUG_RETURN(convert_error_code_to_mysql(
+				    err, m_prebuilt->table->flags,
+				    m_user_thd));
 		break;
 	}
-
-	DBUG_RETURN(error);
 }
 
 /*****************************************************************//**

storage/innobase/include/dict0mem.h

@@ -2,7 +2,7 @@
 
 Copyright (c) 1996, 2017, Oracle and/or its affiliates. All Rights Reserved.
 Copyright (c) 2012, Facebook Inc.
-Copyright (c) 2013, 2017, MariaDB Corporation.
+Copyright (c) 2013, 2018, MariaDB Corporation.
 
 This program is free software; you can redistribute it and/or modify it under
 the terms of the GNU General Public License as published by the Free Software
@@ -1544,6 +1544,23 @@ struct dict_table_t {
 
 	bool versioned() const { return vers_start || vers_end; }
 
+	void inc_fk_checks()
+	{
+#ifdef UNIV_DEBUG
+		lint fk_checks=
+#endif
+		my_atomic_addlint(&n_foreign_key_checks_running, 1);
+		ut_ad(fk_checks >= 0);
+	}
+	void dec_fk_checks()
+	{
+#ifdef UNIV_DEBUG
+		lint fk_checks=
+#endif
+		my_atomic_addlint(&n_foreign_key_checks_running, -1);
+		ut_ad(fk_checks > 0);
+	}
+
 	/** Id of the table. */
 	table_id_t				id;
 

storage/innobase/include/lock0lock.h

 /*****************************************************************************
 
 Copyright (c) 1996, 2016, Oracle and/or its affiliates. All Rights Reserved.
-Copyright (c) 2017, MariaDB Corporation.
+Copyright (c) 2017, 2018, MariaDB Corporation.
 
 This program is free software; you can redistribute it and/or modify it under
 the terms of the GNU General Public License as published by the Free Software
@@ -511,18 +511,6 @@ void
 lock_trx_release_locks(
 /*===================*/
 	trx_t*	trx);	/*!< in/out: transaction */
-/*********************************************************************//**
-Removes locks on a table to be dropped or truncated.
-If remove_also_table_sx_locks is TRUE then table-level S and X locks are
-also removed in addition to other table-level and record-level locks.
-No lock, that is going to be removed, is allowed to be a wait lock. */
-void
-lock_remove_all_on_table(
-/*=====================*/
-	dict_table_t*	table,			/*!< in: table to be dropped
-						or truncated */
-	ibool		remove_also_table_sx_locks);/*!< in: also removes
-						table S and X locks */
 
 /*********************************************************************//**
 Calculates the fold value of a page file address: used in inserting or

storage/innobase/lock/lock0lock.cc

@@ -5300,113 +5300,6 @@ lock_trx_table_locks_remove(
 	ut_error;
 }
 
-/*********************************************************************//**
-Removes locks of a transaction on a table to be dropped.
-If remove_also_table_sx_locks is TRUE then table-level S and X locks are
-also removed in addition to other table-level and record-level locks.
-No lock that is going to be removed is allowed to be a wait lock. */
-static
-void
-lock_remove_all_on_table_for_trx(
-/*=============================*/
-	dict_table_t*	table,			/*!< in: table to be dropped */
-	trx_t*		trx,			/*!< in: a transaction */
-	ibool		remove_also_table_sx_locks)/*!< in: also removes
-						table S and X locks */
-{
-	lock_t*		lock;
-	lock_t*		prev_lock;
-
-	ut_ad(lock_mutex_own());
-
-	for (lock = UT_LIST_GET_LAST(trx->lock.trx_locks);
-	     lock != NULL;
-	     lock = prev_lock) {
-
-		prev_lock = UT_LIST_GET_PREV(trx_locks, lock);
-
-		if (lock_get_type_low(lock) == LOCK_REC
-		    && lock->index->table == table) {
-			ut_a(!lock_get_wait(lock));
-
-			lock_rec_discard(lock);
-		} else if (lock_get_type_low(lock) & LOCK_TABLE
-			   && lock->un_member.tab_lock.table == table
-			   && (remove_also_table_sx_locks
-			       || !IS_LOCK_S_OR_X(lock))) {
-
-			ut_a(!lock_get_wait(lock));
-
-			lock_trx_table_locks_remove(lock);
-			lock_table_remove_low(lock);
-		}
-	}
-}
-
-/*********************************************************************//**
-Removes locks on a table to be dropped or truncated.
-If remove_also_table_sx_locks is TRUE then table-level S and X locks are
-also removed in addition to other table-level and record-level locks.
-No lock, that is going to be removed, is allowed to be a wait lock. */
-void
-lock_remove_all_on_table(
-/*=====================*/
-	dict_table_t*	table,			/*!< in: table to be dropped
-						or truncated */
-	ibool		remove_also_table_sx_locks)/*!< in: also removes
-						table S and X locks */
-{
-	lock_t*		lock;
-
-	lock_mutex_enter();
-
-	for (lock = UT_LIST_GET_FIRST(table->locks);
-	     lock != NULL;
-	     /* No op */) {
-
-		lock_t*	prev_lock;
-
-		prev_lock = UT_LIST_GET_PREV(un_member.tab_lock.locks, lock);
-
-		/* If we should remove all locks (remove_also_table_sx_locks
-		is TRUE), or if the lock is not table-level S or X lock,
-		then check we are not going to remove a wait lock. */
-		if (remove_also_table_sx_locks
-		    || !(lock_get_type(lock) == LOCK_TABLE
-			 && IS_LOCK_S_OR_X(lock))) {
-
-			ut_a(!lock_get_wait(lock));
-		}
-
-		lock_remove_all_on_table_for_trx(
-			table, lock->trx, remove_also_table_sx_locks);
-
-		if (prev_lock == NULL) {
-			if (lock == UT_LIST_GET_FIRST(table->locks)) {
-				/* lock was not removed, pick its successor */
-				lock = UT_LIST_GET_NEXT(
-					un_member.tab_lock.locks, lock);
-			} else {
-				/* lock was removed, pick the first one */
-				lock = UT_LIST_GET_FIRST(table->locks);
-			}
-		} else if (UT_LIST_GET_NEXT(un_member.tab_lock.locks,
-					    prev_lock) != lock) {
-			/* If lock was removed by
-			lock_remove_all_on_table_for_trx() then pick the
-			successor of prev_lock ... */
-			lock = UT_LIST_GET_NEXT(
-				un_member.tab_lock.locks, prev_lock);
-		} else {
-			/* ... otherwise pick the successor of lock. */
-			lock = UT_LIST_GET_NEXT(
-				un_member.tab_lock.locks, lock);
-		}
-	}
-
-	lock_mutex_exit();
-}
-
 /*===================== VALIDATION AND DEBUGGING ====================*/
 
 /** Print info of a table lock.

storage/innobase/row/row0ins.cc

@@ -1446,15 +1446,11 @@ row_ins_foreign_check_on_constraint(
 #endif /* WITH_WSREP */
 	node->new_upd_nodes->push_back(cascade);
 
-	my_atomic_addlint(&table->n_foreign_key_checks_running, 1);
-
-	ut_ad(foreign->foreign_table->n_foreign_key_checks_running > 0);
+	table->inc_fk_checks();
 
 	/* Release the data dictionary latch for a while, so that we do not
 	starve other threads from doing CREATE TABLE etc. if we have a huge
-	cascaded operation running. The counter n_foreign_key_checks_running
-	will prevent other users from dropping or ALTERing the table when we
-	release the latch. */
+	cascaded operation running. */
 
 	row_mysql_unfreeze_data_dictionary(thr_get_trx(thr));
 
@@ -1553,17 +1549,6 @@ row_ins_set_exclusive_rec_lock(
 	return(err);
 }
 
-/* Decrement a counter in the destructor. */
-class ib_dec_in_dtor {
-public:
-	ib_dec_in_dtor(ulint& c): counter(c) {}
-	~ib_dec_in_dtor() {
-		my_atomic_addlint(&counter, -1);
-	}
-private:
-	ulint&		counter;
-};
-
 /***************************************************************//**
 Checks if foreign key constraint fails for an index entry. Sets shared locks
 which lock either the success or the failure of the constraint. NOTE that
@@ -1911,19 +1896,13 @@ row_ins_check_foreign_constraint(
 
 do_possible_lock_wait:
 	if (err == DB_LOCK_WAIT) {
-		/* An object that will correctly decrement the FK check counter
-		when it goes out of this scope. */
-		ib_dec_in_dtor	dec(check_table->n_foreign_key_checks_running);
-
 		trx->error_state = err;
 
 		que_thr_stop_for_mysql(thr);
 
 		thr->lock_state = QUE_THR_LOCK_ROW;
 
-		/* To avoid check_table being dropped, increment counter */
-		my_atomic_addlint(
-			&check_table->n_foreign_key_checks_running, 1);
+		check_table->inc_fk_checks();
 
 		trx_kill_blocking(trx);
 
@@ -1934,6 +1913,8 @@ row_ins_check_foreign_constraint(
 		err = check_table->to_be_dropped
 			? DB_LOCK_WAIT_TIMEOUT
 			: trx->error_state;
+
+		check_table->dec_fk_checks();
 	}
 
 exit_func:

storage/innobase/row/row0mysql.cc

@@ -1827,15 +1827,10 @@ init_fts_doc_id_for_ref(
 	}
 }
 
-/* A functor for decrementing counters. */
-class ib_dec_counter {
-public:
-	ib_dec_counter() {}
-
+struct dec_foreign_key_checks_running
+{
 	void operator() (upd_node_t* node) {
-		ut_ad(node->table->n_foreign_key_checks_running > 0);
-		my_atomic_addlint(
-			&node->table->n_foreign_key_checks_running, -1);
+		node->table->dec_fk_checks();
 	}
 };
 
@@ -2061,7 +2056,7 @@ row_update_for_mysql(row_prebuilt_t* prebuilt)
 		if (was_lock_wait) {
 			std::for_each(new_upd_nodes->begin(),
 				      new_upd_nodes->end(),
-				      ib_dec_counter());
+				      dec_foreign_key_checks_running());
 			std::for_each(new_upd_nodes->begin(),
 				      new_upd_nodes->end(),
 				      que_graph_free_recursive);
@@ -2093,9 +2088,7 @@ row_update_for_mysql(row_prebuilt_t* prebuilt)
 
 	if (thr->fk_cascade_depth > 0) {
 		/* Processing cascade operation */
-		ut_ad(node->table->n_foreign_key_checks_running > 0);
-		my_atomic_addlint(
-			&node->table->n_foreign_key_checks_running, -1);
+		dec_foreign_key_checks_running()(node);
 		node->processed_cascades->push_back(node);
 	}
 
@@ -2209,20 +2202,17 @@ row_update_for_mysql(row_prebuilt_t* prebuilt)
 	}
 
 	if (thr->fk_cascade_depth > 0) {
-		ut_ad(node->table->n_foreign_key_checks_running > 0);
-		my_atomic_addlint(
-			&node->table->n_foreign_key_checks_running, -1);
+		dec_foreign_key_checks_running()(node);
 		thr->fk_cascade_depth = 0;
 	}
 
-	/* Reset the table->n_foreign_key_checks_running counter */
 	std::for_each(cascade_upd_nodes->begin(),
 		      cascade_upd_nodes->end(),
-		      ib_dec_counter());
+		      dec_foreign_key_checks_running());
 
 	std::for_each(new_upd_nodes->begin(),
 		      new_upd_nodes->end(),
-		      ib_dec_counter());
+		      dec_foreign_key_checks_running());
 
 	std::for_each(cascade_upd_nodes->begin(),
 		      cascade_upd_nodes->end(),
@@ -3092,9 +3082,6 @@ row_mysql_table_id_reassign(
 
 	dict_hdr_get_new_id(new_id, NULL, NULL, table, false);
 
-	/* Remove all locks except the table-level S and X locks. */
-	lock_remove_all_on_table(table, FALSE);
-
 	pars_info_add_ull_literal(info, "old_id", table->id);
 	pars_info_add_ull_literal(info, "new_id", *new_id);
 
@@ -3145,7 +3132,7 @@ row_discard_tablespace_begin(
 	if (table) {
 		dict_stats_wait_bg_to_stop_using_table(table, trx);
 		ut_a(!is_system_tablespace(table->space));
-		ut_a(table->n_foreign_key_checks_running == 0);
+		ut_ad(!table->n_foreign_key_checks_running);
 	}
 
 	return(table);
@@ -3264,10 +3251,7 @@ row_discard_tablespace(
 	their operations.
 
 	3) Insert buffer: we remove all entries for the tablespace in
-	the insert buffer tree.
-
-	4) FOREIGN KEY operations: if table->n_foreign_key_checks_running > 0,
-	we do not allow the discard. */
+	the insert buffer tree. */
 
 	ibuf_delete_for_discarded_space(table->space);
 
@@ -3391,19 +3375,9 @@ row_discard_tablespace_for_mysql(
 
 		err = DB_ERROR;
 
-	} else if (table->n_foreign_key_checks_running > 0) {
-		char	table_name[MAX_FULL_NAME_LEN + 1];
-
-		innobase_format_name(
-			table_name, sizeof(table_name),
-			table->name.m_name);
-
-		ib_senderrf(trx->mysql_thd, IB_LOG_LEVEL_ERROR,
-			    ER_DISCARD_FK_CHECKS_RUNNING, table_name);
-
-		err = DB_ERROR;
-
 	} else {
+		ut_ad(!table->n_foreign_key_checks_running);
+
 		/* Do foreign key constraint checks. */
 
 		err = row_discard_tablespace_foreign_key_checks(trx, table);

storage/innobase/row/row0trunc.cc

 /*****************************************************************************
 
 Copyright (c) 2013, 2017, Oracle and/or its affiliates. All Rights Reserved.
-Copyright (c) 2017, MariaDB Corporation.
+Copyright (c) 2017, 2018, MariaDB Corporation.
 
 This program is free software; you can redistribute it and/or modify it under
 the terms of the GNU General Public License as published by the Free Software
@@ -1651,19 +1651,7 @@ row_truncate_foreign_key_checks(
 		return(DB_ERROR);
 	}
 
-	/* TODO: could we replace the counter n_foreign_key_checks_running
-	with lock checks on the table? Acquire here an exclusive lock on the
-	table, and rewrite lock0lock.cc and the lock wait in srv0srv.cc so that
-	they can cope with the table having been truncated here? Foreign key
-	checks take an IS or IX lock on the table. */
-
-	if (table->n_foreign_key_checks_running > 0) {
-		ib::warn() << "Cannot truncate table " << table->name
-			<< " because there is a foreign key check running on"
-			" it.";
-
-		return(DB_ERROR);
-	}
+	ut_ad(!table->n_foreign_key_checks_running);
 
 	return(DB_SUCCESS);
 }
@@ -1719,9 +1707,6 @@ row_truncate_table_for_mysql(
 	This would include check for tablespace discard status, ibd file
 	missing, etc ....
 
-	Step-2: Start transaction (only for non-temp table as temp-table don't
-	modify any data on disk doesn't need transaction object).
-
 	Step-3: Validate ownership of needed locks (Exclusive lock).
 	Ownership will also ensure there is no active SQL queries, INSERT,
 	SELECT, .....
@@ -1802,11 +1787,8 @@ row_truncate_table_for_mysql(
 
 	}
 
-	/* Step-2: Start transaction (only for non-temp table as temp-table
-	don't modify any data on disk doesn't need transaction object). */
 	if (!dict_table_is_temporary(table)) {
-		/* Avoid transaction overhead for temporary table DDL. */
-		trx_start_for_ddl(trx, TRX_DICT_OP_TABLE);
+		trx_set_dict_operation(trx, TRX_DICT_OP_TABLE);
 	}
 
 	/* Step-3: Validate ownership of needed locks (Exclusive lock).
@@ -1832,8 +1814,6 @@ row_truncate_table_for_mysql(
 				table, trx, fsp_flags, logger, err));
 	}
 
-	/* Remove all locks except the table-level X lock. */
-	lock_remove_all_on_table(table, FALSE);
 	trx->table_id = table->id;
 	trx_set_dict_operation(trx, TRX_DICT_OP_TABLE);