[MDEV-28747] Possible problem with ha_innobase::build_template Created: 2022-06-04  Updated: 2022-06-08  Resolved: 2022-06-08

Status: Closed
Project: MariaDB Server
Component/s: Storage Engine - InnoDB
Affects Version/s: 10.4, 10.5, 10.6, 10.7, 10.8, 10.9
Fix Version/s: 10.4.26, 10.5.17, 10.6.9, 10.7.5, 10.8.4, 10.9.2

Type: Bug Priority: Major
Reporter: Oleg Smirnov Assignee: Oleg Smirnov
Resolution: Fixed Votes: 0
Labels: None

Issue Links:
Relates
relates to MDEV-27366 SIGSEGV in handler_index_cond_check o... In Review

 Description   

While working on MDEV-27366 a potentially problematic piece of code was found in /storage/innobase/handler/ha_innodb.cc:

             if ((active_index != MAX_KEY
 
	            && active_index == pushed_idx_cond_keyno)
 
	            || (pushed_rowid_filter && rowid_filter_is_active)) {
 
		       /* Push down an index condition or an end_range check. */
 
        ...
 
		if (active_index == pushed_idx_cond_keyno) {
 
			m_prebuilt->idx_cond = this;
 
		}
 
	}

The last condition satisfies when 

active_index == pushed_idx_cond_keyno == MAX_KEY

which doesn't look correct. After fixing MDEV-27366 there is no test case to reproduce this scenario.

 Comments   
Comment by Oleg Smirnov [ 2022-06-04 ]

marko, could you please review the branch bb-10.4-MDEV-28747?

Comment by Marko Mäkelä [ 2022-06-06 ]

The commit message should mention both MDEVs.

Could the assignment be moved at the start of the outer if block? It would be easier to follow the logic in that way? Even a goto could be helpful for both the human reader and the processor:

	if (active_index != MAX_KEY
 
	    && active_index == pushed_idx_cond_keyno) {
 
		m_prebuilt->idx_cond = this;
 
		goto icp;
 
	} else if (pushed_rowid_filter && rowid_filter_is_active) {
 
icp:
 
		/* Push down an index condition or an end_range check. */
 
		for (ulint i = 0; i < n_fields; i++) {

I think that it could be useful to check what would happen if the change that was mentioned in MDEV-16402 were reverted.

Comment by Oleg Smirnov [ 2022-06-07 ]

Agree, fixed the first two comments and force-pushed.

Regarding the 3rd point: commenting out the restriction mentioned:

sql/opt_index_pushdown.cc

 
  if ((tab->table->file->index_flags(keyno, 0, 1) &
 
      HA_DO_INDEX_COND_PUSHDOWN) &&
 
     optimizer_flag(tab->join->thd, OPTIMIZER_SWITCH_INDEX_COND_PUSHDOWN) &&
 
     tab->join->thd->lex->sql_command != SQLCOM_UPDATE_MULTI &&
 
     tab->join->thd->lex->sql_command != SQLCOM_DELETE_MULTI &&
 
     tab->type != JT_CONST && tab->type != JT_SYSTEM 
/*&&
 
     !(keyno == tab->table->s->primary_key &&             // (6)
 
       tab->table->file->primary_key_is_clustered())*/
 
)     // (6)

causes 9 tests from the main suite fail due to updated execution plans, but no crashes or unexpected results returned are observed.
I tried to analyze performance impact using sysbench (https://github.com/akopytov/sysbench) but couldn't make it work easily. For some reason it doesn't see MariaDB server running. Should I try to get sysbench working or there are other ways we can measure performance impact?

Comment by Marko Mäkelä [ 2022-06-07 ]

oleg.smirnov, thank you for revising the fix. It should be OK to push if all tests pass.

Thank you for testing also MDEV-16402. Let us continue the discussion in that ticket.

Comment by Oleg Smirnov [ 2022-06-08 ]

Pushed commit 5efadf8d8c6076bbee73e45afe050ab62c517f3c to 10.4

Generated at Thu Feb 08 10:03:10 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.