MDEV-16675 Unnecessary explicit lock acquisition during UPDATE or DELETE

In InnoDB, an INSERT will not create an explicit lock object. Instead,
the inserted record is initially implicitly locked by the transaction
that wrote its trx_t::id to the hidden system column DB_TRX_ID.
(Other transactions would check if DB_TRX_ID is referring to a
transaction that has not been committed.)

If a record was inserted in the current transaction, it would be
implicitly locked by that transaction. Only if some other transaction
is requesting access to the record, the implicit lock should be
converted to an explicit one, so that the waits-for graph can be
constructed for detecting deadlocks and lock wait timeouts.

Before this fix, InnoDB would convert implicit locks to
explicit ones, even if no conflict exists.

lock_rec_convert_impl_to_expl(): Return whether caller_trx
already holds an explicit lock that covers the record.

row_vers_impl_x_locked_low(): Avoid a lookup if the record matches
caller_trx->id.

lock_trx_has_expl_x_lock(): Renamed from lock_trx_has_rec_x_lock().

row_upd_clust_step(): In a debug assertion, check for implicit lock
before invoking lock_trx_has_expl_x_lock().

rw_trx_hash_t::find(): Make do_ref_count a mandatory parameter.
Assert that trx_id is not 0 (the caller should check it).

trx_sys_t::is_registered(): Only invoke find() if id != 0.

trx_sys_t::find(): Add the optional parameter do_ref_count.

lock_rec_queue_validate(): Avoid lookup for trx_id == 0.

mysql-test/main/xa.result

@@ -296,20 +296,21 @@ DROP TABLE t1;
 #                ASSERTION THD->TRANSACTION.XID_STATE.XID.IS_NULL()
 #                FAILED 
 #
-DROP TABLE IF EXISTS t1, t2;
 CREATE TABLE t1 (a INT) ENGINE=InnoDB;
 CREATE TABLE t2 (a INT) ENGINE=InnoDB;
-START TRANSACTION;
-INSERT INTO t1 VALUES (1);
+INSERT INTO t2 VALUES (1);
+COMMIT;
+BEGIN;
+INSERT INTO t2 VALUES (2);
+UPDATE t2 SET a=a+1;
 connect  con2,localhost,root;
 XA START 'xid1';
+INSERT INTO t1 VALUES (1);
 # Sending:
-INSERT INTO t2 SELECT a FROM t1;
+DELETE FROM t2;
 connection default;
-# Waiting until INSERT ... is blocked
 DELETE FROM t1;
 connection con2;
-# Reaping: INSERT INTO t2 SELECT a FROM t1
 ERROR 40001: Deadlock found when trying to get lock; try restarting transaction
 XA COMMIT 'xid1';
 ERROR XA102: XA_RBDEADLOCK: Transaction branch was rolled back: deadlock was detected

mysql-test/main/xa.test

@@ -393,33 +393,30 @@ DROP TABLE t1;
 --echo #                FAILED 
 --echo #
 
---disable_warnings
-DROP TABLE IF EXISTS t1, t2;
---enable_warnings
-
 CREATE TABLE t1 (a INT) ENGINE=InnoDB;
 CREATE TABLE t2 (a INT) ENGINE=InnoDB;
 
-START TRANSACTION;
-INSERT INTO t1 VALUES (1);
+INSERT INTO t2 VALUES (1); COMMIT;
+BEGIN;
+INSERT INTO t2 VALUES (2);
+UPDATE t2 SET a=a+1;
 
 --connect (con2,localhost,root)
 XA START 'xid1';
+INSERT INTO t1 VALUES (1);
 --echo # Sending:
---send INSERT INTO t2 SELECT a FROM t1
+--send DELETE FROM t2
 
 --connection default
 let $wait_condition=
   SELECT COUNT(*) = 1 FROM information_schema.processlist
-  WHERE state = "Sending data"
-  AND info = "INSERT INTO t2 SELECT a FROM t1";
---echo # Waiting until INSERT ... is blocked
+  WHERE state = "Updating"
+  AND info = "DELETE FROM t2";
 --source include/wait_condition.inc
 --sleep 0.1
-DELETE FROM t1;
+--send DELETE FROM t1
 
 --connection con2
---echo # Reaping: INSERT INTO t2 SELECT a FROM t1
 --error ER_LOCK_DEADLOCK
 --reap
 --error ER_XA_RBDEADLOCK
@@ -427,6 +424,7 @@ XA COMMIT 'xid1';
 
 connection default;
 
+reap;
 COMMIT;
 
 connection con2;

mysql-test/suite/innodb/r/innodb_information_schema.result

@@ -45,7 +45,7 @@ trx_last_foreign_key_error	varchar(256)	YES		NULL
 trx_is_read_only	int(1)	NO		0	
 trx_autocommit_non_locking	int(1)	NO		0	
 trx_state	trx_weight	trx_tables_in_use	trx_tables_locked	trx_rows_locked	trx_rows_modified	trx_concurrency_tickets	trx_isolation_level	trx_unique_checks	trx_foreign_key_checks
-RUNNING	4	0	1	7	1	0	REPEATABLE READ	1	1
+RUNNING	3	0	1	5	1	0	REPEATABLE READ	1	1
 trx_isolation_level	trx_unique_checks	trx_foreign_key_checks
 SERIALIZABLE	0	0
 trx_state	trx_isolation_level	trx_last_foreign_key_error

mysql-test/suite/innodb/r/monitor.result

@@ -662,7 +662,21 @@ SELECT NAME, COUNT > 0 FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME
 LIKE 'buffer_page_written_index_leaf';
 NAME	COUNT > 0
 buffer_page_written_index_leaf	1
+DROP TABLE t1;
+CREATE TABLE t1(id INT PRIMARY KEY, a INT, b CHAR(1), UNIQUE KEY u(a,b))
+ENGINE=InnoDB;
+SET @start = (SELECT COUNT FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME
+= 'lock_rec_lock_created');
+BEGIN;
+INSERT INTO t1 VALUES(1,1,'a'),(2,9999,'b'),(3,10000,'c'),(4,4,'d');
+DELETE FROM t1 WHERE a = 9999 AND b='b';
+COMMIT;
+SET @end = (SELECT COUNT FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME
+= 'lock_rec_lock_created');
+SELECT @end - @start;
+@end - @start
+0
+DROP TABLE t1;
 SET GLOBAL innodb_monitor_enable=default;
 SET GLOBAL innodb_monitor_disable=default;
 SET GLOBAL innodb_monitor_reset_all=default;
-DROP TABLE t1;

mysql-test/suite/innodb/t/monitor.test

@@ -422,10 +422,27 @@ UNLOCK TABLES;
 SELECT NAME, COUNT > 0 FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME
 LIKE 'buffer_page_written_index_leaf';
 
+DROP TABLE t1;
+
+CREATE TABLE t1(id INT PRIMARY KEY, a INT, b CHAR(1), UNIQUE KEY u(a,b))
+ENGINE=InnoDB;
+
+SET @start = (SELECT COUNT FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME
+= 'lock_rec_lock_created');
+
+BEGIN;
+INSERT INTO t1 VALUES(1,1,'a'),(2,9999,'b'),(3,10000,'c'),(4,4,'d');
+DELETE FROM t1 WHERE a = 9999 AND b='b';
+COMMIT;
+
+SET @end = (SELECT COUNT FROM INFORMATION_SCHEMA.INNODB_METRICS WHERE NAME
+= 'lock_rec_lock_created');
+SELECT @end - @start;
+
+DROP TABLE t1;
+
 --disable_warnings
 SET GLOBAL innodb_monitor_enable=default;
 SET GLOBAL innodb_monitor_disable=default;
 SET GLOBAL innodb_monitor_reset_all=default;
 --enable_warnings
-
-DROP TABLE t1;

storage/innobase/include/lock0lock.h

@@ -789,19 +789,21 @@ const lock_t*
 lock_trx_has_sys_table_locks(
 /*=========================*/
 	const trx_t*	trx)	/*!< in: transaction to check */
-	MY_ATTRIBUTE((warn_unused_result));
+	MY_ATTRIBUTE((nonnull, warn_unused_result));
 
-/*******************************************************************//**
-Check if the transaction holds an exclusive lock on a record.
-@return whether the locks are held */
+/** Check if the transaction holds an explicit exclusive lock on a record.
+@param[in]	trx	transaction
+@param[in]	table	table
+@param[in]	block	leaf page
+@param[in]	heap_no	heap number identifying the record
+@return whether an explicit X-lock is held */
 bool
-lock_trx_has_rec_x_lock(
-/*====================*/
+lock_trx_has_expl_x_lock(
 	const trx_t*		trx,	/*!< in: transaction to check */
 	const dict_table_t*	table,	/*!< in: table to check */
 	const buf_block_t*	block,	/*!< in: buffer block of the record */
 	ulint			heap_no)/*!< in: record heap number */
-	MY_ATTRIBUTE((warn_unused_result));
+	MY_ATTRIBUTE((nonnull, warn_unused_result));
 #endif /* UNIV_DEBUG */
 
 /**

storage/innobase/include/trx0sys.h

@@ -616,7 +616,7 @@ class rw_trx_hash_t
       @retval pointer to trx
   */
 
-  trx_t *find(trx_t *caller_trx, trx_id_t trx_id, bool do_ref_count= false)
+  trx_t *find(trx_t *caller_trx, trx_id_t trx_id, bool do_ref_count)
   {
     /*
       In MariaDB 10.3, purge will reset DB_TRX_ID to 0
@@ -624,9 +624,10 @@ class rw_trx_hash_t
       always have a nonzero trx_t::id; there the value 0 is
       reserved for transactions that did not write or lock
       anything yet.
+
+      The caller should already have handled trx_id==0 specially.
     */
-    if (!trx_id)
-      return NULL;
+    ut_ad(trx_id);
     if (caller_trx && caller_trx->id == trx_id)
     {
       if (do_ref_count)
@@ -1044,13 +1045,13 @@ class trx_sys_t
 
   bool is_registered(trx_t *caller_trx, trx_id_t id)
   {
-    return rw_trx_hash.find(caller_trx, id);
+    return id && find(caller_trx, id, false);
   }
 
 
-  trx_t *find(trx_t *caller_trx, trx_id_t id)
+  trx_t *find(trx_t *caller_trx, trx_id_t id, bool do_ref_count= true)
   {
-    return rw_trx_hash.find(caller_trx, id, true);
+    return rw_trx_hash.find(caller_trx, id, do_ref_count);
   }
 
 

storage/innobase/lock/lock0lock.cc

@@ -4937,8 +4937,12 @@ lock_rec_queue_validate(
 		/* Unlike the non-debug code, this invariant can only succeed
 		if the check and assertion are covered by the lock mutex. */
 
-		const trx_t *impl_trx = trx_sys.rw_trx_hash.find(current_trx(),
-			lock_clust_rec_some_has_impl(rec, index, offsets));
+		const trx_id_t impl_trx_id = lock_clust_rec_some_has_impl(
+			rec, index, offsets);
+
+		const trx_t *impl_trx = impl_trx_id
+			? trx_sys.find(current_trx(), impl_trx_id, false)
+			: 0;
 
 		ut_ad(lock_mutex_own());
 		/* impl_trx cannot be committed until lock_mutex_exit()
@@ -5547,18 +5551,31 @@ static void lock_rec_other_trx_holds_expl(trx_t *caller_trx, trx_t *trx,
 #endif /* UNIV_DEBUG */
 
 
-/*********************************************************************//**
-If a transaction has an implicit x-lock on a record, but no explicit x-lock
-set on the record, sets one for it. */
+/** If an implicit x-lock exists on a record, convert it to an explicit one.
+
+Often, this is called by a transaction that is about to enter a lock wait
+due to the lock conflict. Two explicit locks would be created: first the
+exclusive lock on behalf of the lock-holder transaction in this function,
+and then a wait request on behalf of caller_trx, in the calling function.
+
+This may also be called by the same transaction that is already holding
+an implicit exclusive lock on the record. In this case, no explicit lock
+should be created.
+
+@param[in,out]	caller_trx	current transaction
+@param[in]	block		index tree leaf page
+@param[in]	rec		record on the leaf page
+@param[in]	index		the index of the record
+@param[in]	offsets		rec_get_offsets(rec,index)
+@return	whether caller_trx already holds an exclusive lock on rec */
 static
-void
+bool
 lock_rec_convert_impl_to_expl(
-/*==========================*/
-	trx_t*			caller_trx,/*!<in/out: trx of current thread */
-	const buf_block_t*	block,	/*!< in: buffer block of rec */
-	const rec_t*		rec,	/*!< in: user record on page */
-	dict_index_t*		index,	/*!< in: index of record */
-	const ulint*		offsets)/*!< in: rec_get_offsets(rec, index) */
+	trx_t*			caller_trx,
+	const buf_block_t*	block,
+	const rec_t*		rec,
+	dict_index_t*		index,
+	const ulint*		offsets)
 {
 	trx_t*		trx;
 
@@ -5574,12 +5591,23 @@ lock_rec_convert_impl_to_expl(
 
 		trx_id = lock_clust_rec_some_has_impl(rec, index, offsets);
 
+		if (trx_id == 0) {
+			return false;
+		}
+		if (UNIV_UNLIKELY(trx_id == caller_trx->id)) {
+			return true;
+		}
+
 		trx = trx_sys.find(caller_trx, trx_id);
 	} else {
 		ut_ad(!dict_index_is_online_ddl(index));
 
 		trx = lock_sec_rec_some_has_impl(caller_trx, rec, index,
 						 offsets);
+		if (trx == caller_trx) {
+			trx->release_reference();
+			return true;
+		}
 
 		ut_d(lock_rec_other_trx_holds_expl(caller_trx, trx, rec,
 						   block));
@@ -5597,6 +5625,8 @@ lock_rec_convert_impl_to_expl(
 		lock_rec_convert_impl_to_expl_for_trx(
 			block, rec, index, trx, heap_no);
 	}
+
+	return false;
 }
 
 /*********************************************************************//**
@@ -5641,8 +5671,11 @@ lock_clust_rec_modify_check_and_lock(
 	/* If a transaction has no explicit x-lock set on the record, set one
 	for it */
 
-	lock_rec_convert_impl_to_expl(thr_get_trx(thr), block, rec, index,
-				      offsets);
+	if (lock_rec_convert_impl_to_expl(thr_get_trx(thr), block, rec, index,
+					  offsets)) {
+		/* We already hold an implicit exclusive lock. */
+		return DB_SUCCESS;
+	}
 
 	err = lock_rec_lock(TRUE, LOCK_X | LOCK_REC_NOT_GAP,
 			    block, heap_no, index, thr);
@@ -5786,10 +5819,11 @@ lock_sec_rec_read_check_and_lock(
 	database recovery is running. */
 
 	if (!page_rec_is_supremum(rec)
-	    && page_get_max_trx_id(block->frame) >= trx_sys.get_min_trx_id()) {
-
-		lock_rec_convert_impl_to_expl(thr_get_trx(thr), block, rec,
-					      index, offsets);
+	    && page_get_max_trx_id(block->frame) >= trx_sys.get_min_trx_id()
+	    && lock_rec_convert_impl_to_expl(thr_get_trx(thr), block, rec,
+					     index, offsets)) {
+		/* We already hold an implicit exclusive lock. */
+		return DB_SUCCESS;
 	}
 
 	err = lock_rec_lock(FALSE, ulint(mode) | gap_mode,
@@ -5850,10 +5884,11 @@ lock_clust_rec_read_check_and_lock(
 
 	heap_no = page_rec_get_heap_no(rec);
 
-	if (heap_no != PAGE_HEAP_NO_SUPREMUM) {
-
-		lock_rec_convert_impl_to_expl(thr_get_trx(thr), block, rec,
-					      index, offsets);
+	if (heap_no != PAGE_HEAP_NO_SUPREMUM
+	    && lock_rec_convert_impl_to_expl(thr_get_trx(thr), block, rec,
+					     index, offsets)) {
+		/* We already hold an implicit exclusive lock. */
+		return DB_SUCCESS;
 	}
 
 	err = lock_rec_lock(FALSE, ulint(mode) | gap_mode,
@@ -6560,12 +6595,14 @@ lock_trx_has_sys_table_locks(
 	return(strongest_lock);
 }
 
-/*******************************************************************//**
-Check if the transaction holds an exclusive lock on a record.
-@return whether the locks are held */
+/** Check if the transaction holds an explicit exclusive lock on a record.
+@param[in]	trx	transaction
+@param[in]	table	table
+@param[in]	block	leaf page
+@param[in]	heap_no	heap number identifying the record
+@return whether an explicit X-lock is held */
 bool
-lock_trx_has_rec_x_lock(
-/*====================*/
+lock_trx_has_expl_x_lock(
 	const trx_t*		trx,	/*!< in: transaction to check */
 	const dict_table_t*	table,	/*!< in: table to check */
 	const buf_block_t*	block,	/*!< in: buffer block of the record */
@@ -6574,11 +6611,9 @@ lock_trx_has_rec_x_lock(
 	ut_ad(heap_no > PAGE_HEAP_NO_SUPREMUM);
 
 	lock_mutex_enter();
-	ut_a(lock_table_has(trx, table, LOCK_IX)
-	     || table->is_temporary());
-	ut_a(lock_rec_has_expl(LOCK_X | LOCK_REC_NOT_GAP,
-			       block, heap_no, trx)
-	     || table->is_temporary());
+	ut_ad(lock_table_has(trx, table, LOCK_IX));
+	ut_ad(lock_rec_has_expl(LOCK_X | LOCK_REC_NOT_GAP, block, heap_no,
+				trx));
 	lock_mutex_exit();
 	return(true);
 }

storage/innobase/row/row0upd.cc

@@ -3093,9 +3093,7 @@ row_upd_clust_step(
 
 	ulint	mode;
 
-	DEBUG_SYNC_C_IF_THD(
-		thr_get_trx(thr)->mysql_thd,
-		"innodb_row_upd_clust_step_enter");
+	DEBUG_SYNC_C_IF_THD(trx->mysql_thd, "innodb_row_upd_clust_step_enter");
 
 	if (dict_index_is_online_ddl(index)) {
 		ut_ad(node->table->id != DICT_INDEXES_ID);
@@ -3157,10 +3155,11 @@ row_upd_clust_step(
 		}
 	}
 
-	ut_ad(index->table->no_rollback()
-	      || lock_trx_has_rec_x_lock(thr_get_trx(thr), index->table,
-					 btr_pcur_get_block(pcur),
-					 page_rec_get_heap_no(rec)));
+	ut_ad(index->table->no_rollback() || index->table->is_temporary()
+	      || row_get_rec_trx_id(rec, index, offsets) == trx->id
+	      || lock_trx_has_expl_x_lock(trx, index->table,
+					  btr_pcur_get_block(pcur),
+					  page_rec_get_heap_no(rec)));
 
 	/* NOTE: the following function calls will also commit mtr */
 

storage/innobase/row/row0vers.cc

@@ -126,14 +126,22 @@ row_vers_impl_x_locked_low(
 		DBUG_RETURN(0);
 	}
 
-	trx_t*	trx = trx_sys.find(caller_trx, trx_id);
+	trx_t*	trx;
 
-	if (trx == 0) {
-		/* The transaction that modified or inserted clust_rec is no
-		longer active, or it is corrupt: no implicit lock on rec */
-		lock_check_trx_id_sanity(trx_id, clust_rec, clust_index, clust_offsets);
-		mem_heap_free(heap);
-		DBUG_RETURN(0);
+	if (trx_id == caller_trx->id) {
+		trx = caller_trx;
+		trx->reference();
+	} else {
+		trx = trx_sys.find(caller_trx, trx_id);
+		if (trx == 0) {
+			/* The transaction that modified or inserted
+			clust_rec is no longer active, or it is
+			corrupt: no implicit lock on rec */
+			lock_check_trx_id_sanity(trx_id, clust_rec,
+						 clust_index, clust_offsets);
+			mem_heap_free(heap);
+			DBUG_RETURN(0);
+		}
 	}
 
 	comp = page_rec_is_comp(rec);

storage/innobase/trx/trx0trx.cc

@@ -761,7 +761,7 @@ trx_lists_init_at_db_start()
 		for (undo = UT_LIST_GET_FIRST(rseg->undo_list);
 		     undo != NULL;
 		     undo = UT_LIST_GET_NEXT(undo_list, undo)) {
-			trx_t *trx = trx_sys.rw_trx_hash.find(0, undo->trx_id);
+			trx_t *trx = trx_sys.find(0, undo->trx_id, false);
 			if (!trx) {
 				trx_resurrect(undo, rseg, start_time,
 					      &rows_to_undo, false);