MDEV-28445 Secondary index locking invokes costly trx_sys.get_min_trx_id()

lock_sec_rec_read_check_and_lock(): Remove a redundant check
for trx_sys.get_min_trx_id(). This would be checked in
lock_sec_rec_some_has_impl() anyway. Also, test the
cheapest conditions first.

lock_sec_rec_some_has_impl(): Replace trx_sys.get_min_trx_id()
with trx_sys.find_same_or_older() that is much easier to evaluate.

Inspired by mysql/mysql-server@0a0c0db97e9c9705f4dffc629a770fb87a60cb22

storage/innobase/include/trx0sys.h

@@ -925,20 +925,16 @@ class trx_sys_t
 
 
   /**
-    Returns the minimum trx id in rw trx list.
+    Determine if the specified transaction or any older one might be active.
 
-    This is the smallest id for which the trx can possibly be active. (But, you
-    must look at the trx->state to find out if the minimum trx id transaction
-    itself is active, or already committed.)
-
-    @return the minimum trx id, or m_max_trx_id if the trx list is empty
+    @param caller_trx  used to get/set pins
+    @param id          transaction identifier
+    @return whether any transaction not newer than id might be active
   */
 
-  trx_id_t get_min_trx_id()
+  bool find_same_or_older(trx_t *caller_trx, trx_id_t id)
   {
-    trx_id_t id= get_max_trx_id();
-    rw_trx_hash.iterate(get_min_trx_id_callback, &id);
-    return id;
+    return rw_trx_hash.iterate(caller_trx, find_same_or_older_callback, &id);
   }
 
 
@@ -1173,18 +1169,10 @@ class trx_sys_t
   }
 
 private:
-  static my_bool get_min_trx_id_callback(rw_trx_hash_element_t *element,
-                                         trx_id_t *id)
+  static my_bool find_same_or_older_callback(rw_trx_hash_element_t *element,
+                                             trx_id_t *id)
   {
-    if (element->id < *id)
-    {
-      element->mutex.wr_lock();
-      /* We don't care about read-only transactions here. */
-      if (element->trx && element->trx->rsegs.m_redo.rseg)
-        *id= element->id;
-      element->mutex.wr_unlock();
-    }
-    return 0;
+    return element->id <= *id;
   }
 
 

storage/innobase/lock/lock0lock.cc

@@ -1045,39 +1045,22 @@ lock_sec_rec_some_has_impl(
 	dict_index_t*	index,	/*!< in: secondary index */
 	const rec_offs*	offsets)/*!< in: rec_get_offsets(rec, index) */
 {
-	trx_t*		trx;
-	trx_id_t	max_trx_id;
-	const page_t*	page = page_align(rec);
-
-	lock_sys.assert_unlocked();
-	ut_ad(!dict_index_is_clust(index));
-	ut_ad(page_rec_is_user_rec(rec));
-	ut_ad(rec_offs_validate(rec, index, offsets));
-	ut_ad(!rec_is_metadata(rec, *index));
-
-	max_trx_id = page_get_max_trx_id(page);
-
-	/* Some transaction may have an implicit x-lock on the record only
-	if the max trx id for the page >= min trx id for the trx list, or
-	database recovery is running. */
-
-	if (max_trx_id < trx_sys.get_min_trx_id()) {
-
-		trx = 0;
-
-	} else if (!lock_check_trx_id_sanity(max_trx_id, rec, index, offsets)) {
-
-		/* The page is corrupt: try to avoid a crash by returning 0 */
-		trx = 0;
+  lock_sys.assert_unlocked();
+  ut_ad(!dict_index_is_clust(index));
+  ut_ad(page_rec_is_user_rec(rec));
+  ut_ad(rec_offs_validate(rec, index, offsets));
+  ut_ad(!rec_is_metadata(rec, *index));
 
-	/* In this case it is possible that some transaction has an implicit
-	x-lock. We have to look in the clustered index. */
+  const trx_id_t max_trx_id= page_get_max_trx_id(page_align(rec));
 
-	} else {
-		trx = row_vers_impl_x_locked(caller_trx, rec, index, offsets);
-	}
+  if ((caller_trx->id > max_trx_id &&
+       !trx_sys.find_same_or_older(caller_trx, max_trx_id)) ||
+      !lock_check_trx_id_sanity(max_trx_id, rec, index, offsets))
+    return nullptr;
 
-	return(trx);
+  /* In this case it is possible that some transaction has an implicit
+  x-lock. We have to look in the clustered index. */
+  return row_vers_impl_x_locked(caller_trx, rec, index, offsets);
 }
 
 /*********************************************************************//**
@@ -5413,7 +5396,6 @@ lock_sec_rec_read_check_and_lock(
 	que_thr_t*		thr)	/*!< in: query thread */
 {
 	dberr_t	err;
-	ulint	heap_no;
 
 	ut_ad(!dict_index_is_clust(index));
 	ut_ad(!dict_index_is_online_ddl(index));
@@ -5433,17 +5415,10 @@ lock_sec_rec_read_check_and_lock(
 	const page_id_t id{block->page.id()};
 
 	ut_ad(!rec_is_metadata(rec, *index));
-	heap_no = page_rec_get_heap_no(rec);
-
-	/* Some transaction may have an implicit x-lock on the record only
-	if the max trx id for the page >= min trx id for the trx list or a
-	database recovery is running. */
 
 	trx_t *trx = thr_get_trx(thr);
-	if (!lock_table_has(trx, index->table, LOCK_X)
-	    && !page_rec_is_supremum(rec)
-	    && page_get_max_trx_id(block->page.frame)
-	    >= trx_sys.get_min_trx_id()
+	if (!page_rec_is_supremum(rec)
+	    && !lock_table_has(trx, index->table, LOCK_X)
 	    && lock_rec_convert_impl_to_expl(thr_get_trx(thr), id, rec,
 					     index, offsets)
 	    && gap_mode == LOCK_REC_NOT_GAP) {
@@ -5464,7 +5439,7 @@ lock_sec_rec_read_check_and_lock(
 #endif /* WITH_WSREP */
 
 	err = lock_rec_lock(false, gap_mode | mode,
-			    block, heap_no, index, thr);
+			    block, page_rec_get_heap_no(rec), index, thr);
 
 #ifdef WITH_WSREP
 	if (trx->wsrep == 3) trx->wsrep = 1;