[MDEV-28747] Possible problem with ha_innobase::build_template - Jira

Details

Type: Bug
Status: Closed (View Workflow)
Priority: Major
Resolution: Fixed
Affects Version/s: 10.4(EOL), 10.5, 10.6, 10.7(EOL), 10.8(EOL), 10.9(EOL)
Fix Version/s: 10.4.26, 10.5.17, 10.6.9, 10.7.5, 10.8.4, 10.9.2
Component/s: Storage Engine - InnoDB
Labels:
None

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.

Attachments

Issue Links

relates to

MDEV-27366 SIGSEGV in handler_index_cond_check on SELECT in connection with rowid_filter setting

In Review

Activity

Ascending order - Click to sort in descending order

Oleg Smirnov added a comment - 2022-06-04 06:33

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

Oleg Smirnov added a comment - 2022-06-04 06:33 marko , could you please review the branch bb-10.4- MDEV-28747 ?

Marko Mäkelä added a comment - 2022-06-06 05:58

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.

Marko Mäkelä added a comment - 2022-06-06 05:58 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.

Oleg Smirnov added a comment - 2022-06-07 06:52

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?

Oleg Smirnov added a comment - 2022-06-07 06:52 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?

Marko Mäkelä added a comment - 2022-06-07 07:06

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.

Marko Mäkelä added a comment - 2022-06-07 07:06 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.

Oleg Smirnov added a comment - 2022-06-08 16:49

Pushed commit 5efadf8d8c6076bbee73e45afe050ab62c517f3c to 10.4

Oleg Smirnov added a comment - 2022-06-08 16:49 Pushed commit 5efadf8d8c6076bbee73e45afe050ab62c517f3c to 10.4

People

Assignee:: Oleg Smirnov

Reporter:: Oleg Smirnov

Votes:: 0 Vote for this issue

Watchers:: 2 Start watching this issue

Dates

Created:: 2022-06-04 05:05

Updated:: 2022-06-08 16:49

Resolved:: 2022-06-08 16:49

Git Integration

Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.

MariaDB Server