MDEV-33779 InnoDB row operations could be faster

We have quite a few assertions
	ut_a(m_prebuilt->trx == thd_to_trx(ha_thd()));
in low-level functions.
These had better be debug assertions for performance reasons.
It should suffice to check that condition in the less frequently invoked
ha_innobase::change_active_index().

convert_search_mode_to_innobase(): Return whether the mode is
unsupported, and optionally update ha_innobase::m_last_match_mode.

ha_innobase::index_read(): Only branch on find_flag once, and
simplify the error handling after invoking row_search_mvcc().

ha_innobase::rnd_pos(): Remove an assertion that is duplicating one
in ha_innobase::index_read(), which we are calling unconditionally.

ha_innobase::records_in_range(): Check only once whether
min_key, max_key are null pointers.

row_sel_convert_mysql_key_to_innobase(): Declare all parameters
except the conversion buffer pointer (buf) to be nonnull.

Reviewed by: Debarun Banerjee

storage/innobase/handler/ha_innodb.cc

@@ -8883,47 +8883,63 @@ ha_innobase::index_end(void)
 	DBUG_RETURN(0);
 }
 
-/*********************************************************************//**
-Converts a search mode flag understood by MySQL to a flag understood
-by InnoDB. */
-page_cur_mode_t
-convert_search_mode_to_innobase(
-/*============================*/
-	ha_rkey_function	find_flag)
-{
-	switch (find_flag) {
-	case HA_READ_KEY_EXACT:
-		/* this does not require the index to be UNIQUE */
-	case HA_READ_KEY_OR_NEXT:
-		return(PAGE_CUR_GE);
-	case HA_READ_AFTER_KEY:
-		return(PAGE_CUR_G);
-	case HA_READ_BEFORE_KEY:
-		return(PAGE_CUR_L);
-	case HA_READ_KEY_OR_PREV:
-	case HA_READ_PREFIX_LAST:
-	case HA_READ_PREFIX_LAST_OR_PREV:
-		return(PAGE_CUR_LE);
-	case HA_READ_MBR_CONTAIN:
-		return(PAGE_CUR_CONTAIN);
-	case HA_READ_MBR_INTERSECT:
-		return(PAGE_CUR_INTERSECT);
-	case HA_READ_MBR_WITHIN:
-		return(PAGE_CUR_WITHIN);
-	case HA_READ_MBR_DISJOINT:
-		return(PAGE_CUR_DISJOINT);
-	case HA_READ_MBR_EQUAL:
-		return(PAGE_CUR_MBR_EQUAL);
-	case HA_READ_PREFIX:
-		return(PAGE_CUR_UNSUPP);
-	/* do not use "default:" in order to produce a gcc warning:
-	enumeration value '...' not handled in switch
-	(if -Wswitch or -Wall is used) */
-	}
-
-	my_error(ER_CHECK_NOT_IMPLEMENTED, MYF(0), "this functionality");
-
-	return(PAGE_CUR_UNSUPP);
+/** Convert a MariaDB search mode to an InnoDB search mode.
+@tparam last_match whether last_match_mode is to be set
+@param find_flag       MariaDB search mode
+@param mode            InnoDB search mode
+@param last_match_mode pointer to ha_innobase::m_last_match_mode
+@return whether the search mode is unsupported */
+template<bool last_match= false>
+static bool convert_search_mode_to_innobase(ha_rkey_function find_flag,
+                                            page_cur_mode_t &mode,
+                                            uint *last_match_mode= nullptr)
+{
+  mode= PAGE_CUR_LE;
+  if (last_match)
+    *last_match_mode= 0;
+
+  switch (find_flag) {
+  case HA_READ_KEY_EXACT:
+    /* this does not require the index to be UNIQUE */
+    if (last_match)
+      *last_match_mode= ROW_SEL_EXACT;
+    /* fall through */
+  case HA_READ_KEY_OR_NEXT:
+    mode= PAGE_CUR_GE;
+    return false;
+  case HA_READ_AFTER_KEY:
+    mode= PAGE_CUR_G;
+    return false;
+  case HA_READ_BEFORE_KEY:
+    mode= PAGE_CUR_L;
+    return false;
+  case HA_READ_PREFIX_LAST:
+    if (last_match)
+      *last_match_mode= ROW_SEL_EXACT_PREFIX;
+    /* fall through */
+  case HA_READ_KEY_OR_PREV:
+  case HA_READ_PREFIX_LAST_OR_PREV:
+    return false;
+  case HA_READ_MBR_CONTAIN:
+    mode= PAGE_CUR_CONTAIN;
+    return false;
+  case HA_READ_MBR_INTERSECT:
+    mode= PAGE_CUR_INTERSECT;
+    return false;
+  case HA_READ_MBR_WITHIN:
+    mode= PAGE_CUR_WITHIN;
+    return false;
+  case HA_READ_MBR_DISJOINT:
+    mode= PAGE_CUR_DISJOINT;
+    return false;
+  case HA_READ_MBR_EQUAL:
+    mode= PAGE_CUR_MBR_EQUAL;
+    return false;
+  case HA_READ_PREFIX:
+    break;
+  }
+
+  return true;
 }
 
 /*
@@ -9001,8 +9017,7 @@ ha_innobase::index_read(
 	mariadb_set_stats set_stats_temporary(handler_stats);
 	DEBUG_SYNC_C("ha_innobase_index_read_begin");
 
-	ut_a(m_prebuilt->trx == thd_to_trx(m_user_thd));
-	ut_ad(key_len != 0 || find_flag != HA_READ_KEY_EXACT);
+	ut_ad(m_prebuilt->trx == thd_to_trx(m_user_thd));
 
 	dict_index_t*	index = m_prebuilt->index;
 
@@ -9038,7 +9053,8 @@ ha_innobase::index_read(
 		build_template(false);
 	}
 
-	if (key_ptr != NULL) {
+	if (key_len) {
+		ut_ad(key_ptr);
 		/* Convert the search key value to InnoDB format into
 		m_prebuilt->search_tuple */
 
@@ -9048,42 +9064,31 @@ ha_innobase::index_read(
 			m_prebuilt->srch_key_val_len,
 			index,
 			(byte*) key_ptr,
-			(ulint) key_len);
+			key_len);
 
 		DBUG_ASSERT(m_prebuilt->search_tuple->n_fields > 0);
 	} else {
+		ut_ad(find_flag != HA_READ_KEY_EXACT);
 		/* We position the cursor to the last or the first entry
 		in the index */
 
 		dtuple_set_n_fields(m_prebuilt->search_tuple, 0);
 	}
 
-	page_cur_mode_t	mode = convert_search_mode_to_innobase(find_flag);
-
-	ulint	match_mode = 0;
-
-	if (find_flag == HA_READ_KEY_EXACT) {
+	page_cur_mode_t	mode;
 
-		match_mode = ROW_SEL_EXACT;
-
-	} else if (find_flag == HA_READ_PREFIX_LAST) {
-
-		match_mode = ROW_SEL_EXACT_PREFIX;
+	if (convert_search_mode_to_innobase<true>(find_flag, mode,
+						  &m_last_match_mode)) {
+		table->status = STATUS_NOT_FOUND;
+		DBUG_RETURN(HA_ERR_UNSUPPORTED);
 	}
 
-	m_last_match_mode = (uint) match_mode;
-
-	dberr_t ret = mode == PAGE_CUR_UNSUPP ? DB_UNSUPPORTED
-		: row_search_mvcc(buf, mode, m_prebuilt, match_mode, 0);
+	dberr_t ret =
+		row_search_mvcc(buf, mode, m_prebuilt, m_last_match_mode, 0);
 
 	DBUG_EXECUTE_IF("ib_select_query_failure", ret = DB_ERROR;);
 
-	int	error;
-
-	switch (ret) {
-	case DB_SUCCESS:
-		error = 0;
-		table->status = 0;
+	if (UNIV_LIKELY(ret == DB_SUCCESS)) {
 		if (m_prebuilt->table->is_system_db) {
 			srv_stats.n_system_rows_read.add(
 				thd_get_thread_id(m_prebuilt->trx->mysql_thd), 1);
@@ -9091,48 +9096,33 @@ ha_innobase::index_read(
 			srv_stats.n_rows_read.add(
 				thd_get_thread_id(m_prebuilt->trx->mysql_thd), 1);
 		}
-		break;
-
-	case DB_RECORD_NOT_FOUND:
-		error = HA_ERR_KEY_NOT_FOUND;
-		table->status = STATUS_NOT_FOUND;
-		break;
+		table->status = 0;
+		DBUG_RETURN(0);
+	}
 
-	case DB_END_OF_INDEX:
-		error = HA_ERR_KEY_NOT_FOUND;
-		table->status = STATUS_NOT_FOUND;
-		break;
+	table->status = STATUS_NOT_FOUND;
 
+	switch (ret) {
 	case DB_TABLESPACE_DELETED:
 		ib_senderrf(
 			m_prebuilt->trx->mysql_thd, IB_LOG_LEVEL_ERROR,
 			ER_TABLESPACE_DISCARDED,
 			table->s->table_name.str);
-
-		table->status = STATUS_NOT_FOUND;
-		error = HA_ERR_TABLESPACE_MISSING;
-		break;
-
+		DBUG_RETURN(HA_ERR_TABLESPACE_MISSING);
+	case DB_RECORD_NOT_FOUND:
+	case DB_END_OF_INDEX:
+		DBUG_RETURN(HA_ERR_KEY_NOT_FOUND);
 	case DB_TABLESPACE_NOT_FOUND:
-
 		ib_senderrf(
 			m_prebuilt->trx->mysql_thd, IB_LOG_LEVEL_ERROR,
 			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(
-			ret, m_prebuilt->table->flags, m_user_thd);
-
-		table->status = STATUS_NOT_FOUND;
-		break;
+		DBUG_RETURN(convert_error_code_to_mysql(
+				    ret, m_prebuilt->table->flags,
+				    m_user_thd));
 	}
-
-	DBUG_RETURN(error);
 }
 
 /*******************************************************************//**
@@ -9566,8 +9556,6 @@ ha_innobase::rnd_pos(
 	DBUG_ENTER("rnd_pos");
 	DBUG_DUMP("key", pos, ref_length);
 
-	ut_a(m_prebuilt->trx == thd_to_trx(ha_thd()));
-
 	/* Note that we assume the length of the row reference is fixed
 	for the table, and it is == ref_length */
 
@@ -14301,14 +14289,14 @@ ha_innobase::records_in_range(
 	dict_index_t*	index;
 	dtuple_t*	range_start;
 	dtuple_t*	range_end;
-	ha_rows		n_rows;
+	ha_rows		n_rows = HA_POS_ERROR;
 	page_cur_mode_t	mode1;
 	page_cur_mode_t	mode2;
 	mem_heap_t*	heap;
 
 	DBUG_ENTER("records_in_range");
 
-	ut_a(m_prebuilt->trx == thd_to_trx(ha_thd()));
+	ut_ad(m_prebuilt->trx == thd_to_trx(ha_thd()));
 
 	m_prebuilt->trx->op_info = "estimating records in index range";
 
@@ -14321,12 +14309,7 @@ ha_innobase::records_in_range(
 	/* There exists possibility of not being able to find requested
 	index due to inconsistency between MySQL and InoDB dictionary info.
 	Necessary message should have been printed in innobase_get_index() */
-	if (!m_prebuilt->table->space) {
-		n_rows = HA_POS_ERROR;
-		goto func_exit;
-	}
-	if (!index) {
-		n_rows = HA_POS_ERROR;
+	if (!index || !m_prebuilt->table->space) {
 		goto func_exit;
 	}
 	if (index->is_corrupted()) {
@@ -14342,61 +14325,50 @@ ha_innobase::records_in_range(
 				    + sizeof(dtuple_t)));
 
 	range_start = dtuple_create(heap, key->ext_key_parts);
-	dict_index_copy_types(range_start, index, key->ext_key_parts);
 
 	range_end = dtuple_create(heap, key->ext_key_parts);
-	dict_index_copy_types(range_end, index, key->ext_key_parts);
-
-	row_sel_convert_mysql_key_to_innobase(
-		range_start,
-		m_prebuilt->srch_key_val1,
-		m_prebuilt->srch_key_val_len,
-		index,
-		(byte*) (min_key ? min_key->key : (const uchar*) 0),
-		(ulint) (min_key ? min_key->length : 0));
-
-	DBUG_ASSERT(min_key
-		    ? range_start->n_fields > 0
-		    : range_start->n_fields == 0);
-
-	row_sel_convert_mysql_key_to_innobase(
-		range_end,
-		m_prebuilt->srch_key_val2,
-		m_prebuilt->srch_key_val_len,
-		index,
-		(byte*) (max_key ? max_key->key : (const uchar*) 0),
-		(ulint) (max_key ? max_key->length : 0));
-
-	DBUG_ASSERT(max_key
-		    ? range_end->n_fields > 0
-		    : range_end->n_fields == 0);
-
-	mode1 = convert_search_mode_to_innobase(
-		min_key ? min_key->flag : HA_READ_KEY_EXACT);
-
-	mode2 = convert_search_mode_to_innobase(
-		max_key ? max_key->flag : HA_READ_KEY_EXACT);
-
-	if (mode1 != PAGE_CUR_UNSUPP && mode2 != PAGE_CUR_UNSUPP) {
-
-		if (dict_index_is_spatial(index)) {
-			/*Only min_key used in spatial index. */
-			n_rows = rtr_estimate_n_rows_in_range(
-				index, range_start, mode1);
-		} else {
-                        btr_pos_t tuple1(range_start, mode1, pages->first_page);
-                        btr_pos_t tuple2(range_end,   mode2, pages->last_page);
-			n_rows = btr_estimate_n_rows_in_range(
-                                 index, &tuple1, &tuple2);
-                        pages->first_page= tuple1.page_id.raw();
-                        pages->last_page=  tuple2.page_id.raw();
-		}
+
+	if (!min_key) {
+		mode1 = PAGE_CUR_GE;
+		dtuple_set_n_fields(range_start, 0);
+	} else if (convert_search_mode_to_innobase(min_key->flag, mode1)) {
+		goto unsupported;
 	} else {
+		dict_index_copy_types(range_start, index, key->ext_key_parts);
+		row_sel_convert_mysql_key_to_innobase(
+			range_start,
+			m_prebuilt->srch_key_val1,
+			m_prebuilt->srch_key_val_len,
+			index, min_key->key, min_key->length);
+		DBUG_ASSERT(range_start->n_fields > 0);
+	}
 
-		n_rows = HA_POS_ERROR;
+	if (!max_key) {
+		mode2 = PAGE_CUR_GE;
+		dtuple_set_n_fields(range_end, 0);
+	} else if (convert_search_mode_to_innobase(max_key->flag, mode2)) {
+		goto unsupported;
+	} else {
+		dict_index_copy_types(range_end, index, key->ext_key_parts);
+		row_sel_convert_mysql_key_to_innobase(
+			range_end,
+			m_prebuilt->srch_key_val2,
+			m_prebuilt->srch_key_val_len,
+			index, max_key->key, max_key->length);
+		DBUG_ASSERT(range_end->n_fields > 0);
 	}
 
-	mem_heap_free(heap);
+	if (dict_index_is_spatial(index)) {
+		/*Only min_key used in spatial index. */
+		n_rows = rtr_estimate_n_rows_in_range(
+			index, range_start, mode1);
+	} else {
+		btr_pos_t tuple1(range_start, mode1, pages->first_page);
+		btr_pos_t tuple2(range_end,   mode2, pages->last_page);
+		n_rows = btr_estimate_n_rows_in_range(index, &tuple1, &tuple2);
+		pages->first_page= tuple1.page_id.raw();
+		pages->last_page=  tuple2.page_id.raw();
+	}
 
 	DBUG_EXECUTE_IF(
 		"print_btr_estimate_n_rows_in_range_return_value",
@@ -14407,11 +14379,7 @@ ha_innobase::records_in_range(
                         (longlong) n_rows);
 	);
 
-func_exit:
-
-	m_prebuilt->trx->op_info = (char*)"";
-
-	/* The MySQL optimizer seems to believe an estimate of 0 rows is
+	/* The MariaDB optimizer seems to believe an estimate of 0 rows is
 	always accurate and may return the result 'Empty set' based on that.
 	The accuracy is not guaranteed, and even if it were, for a locking
 	read we should anyway perform the search to set the next-key lock.
@@ -14421,6 +14389,10 @@ ha_innobase::records_in_range(
 		n_rows = 1;
 	}
 
+unsupported:
+	mem_heap_free(heap);
+func_exit:
+	m_prebuilt->trx->op_info = "";
 	DBUG_RETURN((ha_rows) n_rows);
 }
 

storage/innobase/include/row0sel.h

@@ -115,8 +115,8 @@ row_sel_convert_mysql_key_to_innobase(
 	ulint		buf_len,	/*!< in: buffer length */
 	dict_index_t*	index,		/*!< in: index of the key value */
 	const byte*	key_ptr,	/*!< in: MySQL key value */
-	ulint		key_len);	/*!< in: MySQL key value length */
-
+	ulint		key_len)	/*!< in: MySQL key value length */
+	MY_ATTRIBUTE((nonnull(1,4,5)));
 
 /** Search for rows in the database using cursor.
 Function is mainly used for tables that are shared across connections and