[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 created issue - 2022-06-04 05:05

Oleg Smirnov made changes - 2022-06-04 05:07

Field	Original Value	New Value
Fix Version/s		10.4 [ 22408 ]
Fix Version/s		10.5 [ 23123 ]
Fix Version/s		10.6 [ 24028 ]
Fix Version/s		10.7 [ 24805 ]
Fix Version/s		10.8 [ 26121 ]
Fix Version/s		10.9 [ 26905 ]

Oleg Smirnov made changes - 2022-06-04 05:07

Status

Open [ 1 ]

In Progress [ 3 ]

Oleg Smirnov made changes - 2022-06-04 05:36

Description

While working on MDEV-27366 a potentially problematic piece of code was found in /storage/innobase/handler/ha_innodb.cc:
{code}
if (active_index == pushed_idx_cond_keyno) {
m_prebuilt->idx_cond = this;
}
{code}

This condition satisfies when {code}active_index == pushed_idx_cond_keyno == MAX_KEY{code} which doesn't look correct. After fixing MDEV-27366 there is no test case to reproduce this scenario.

While working on MDEV-27366 a potentially problematic piece of code was found in /storage/innobase/handler/ha_innodb.cc:
{code}
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;

}

}

{code}

This condition satisfies when {code}active_index == pushed_idx_cond_keyno == MAX_KEY{code} which doesn't look correct. After fixing MDEV-27366 there is no test case to reproduce this scenario.

Oleg Smirnov made changes - 2022-06-04 05:38

Description

While working on MDEV-27366 a potentially problematic piece of code was found in /storage/innobase/handler/ha_innodb.cc:
{code}
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;
}
}
{code}

The last condition satisfies when {code}active_index == pushed_idx_cond_keyno == MAX_KEY{code} which doesn't look correct. After fixing MDEV-27366 there is no test case to reproduce this scenario.

Oleg Smirnov made changes - 2022-06-04 06:32

Assignee	Oleg Smirnov [ JIRAUSER50405 ]	Marko Mäkelä [ marko ]
Status	In Progress [ 3 ]	In Review [ 10002 ]

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 ?

Roel Van de Paar made changes - 2022-06-05 22:00

Link

This issue relates to MDEV-27366 [ MDEV-27366 ]

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.

Marko Mäkelä made changes - 2022-06-06 05:58

Assignee	Marko Mäkelä [ marko ]	Oleg Smirnov [ JIRAUSER50405 ]
Status	In Review [ 10002 ]	Stalled [ 10000 ]

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

Oleg Smirnov made changes - 2022-06-08 16:49

Component/s		Storage Engine - InnoDB [ 10129 ]
Fix Version/s		10.4.26 [ 27511 ]
Fix Version/s		10.5.17 [ 27509 ]
Fix Version/s		10.6.9 [ 27507 ]
Fix Version/s		10.7.5 [ 27505 ]
Fix Version/s		10.8.4 [ 27503 ]
Fix Version/s		10.9.2 [ 27115 ]
Fix Version/s	10.4 [ 22408 ]
Fix Version/s	10.5 [ 23123 ]
Fix Version/s	10.6 [ 24028 ]
Fix Version/s	10.7 [ 24805 ]
Fix Version/s	10.8 [ 26121 ]
Fix Version/s	10.9 [ 26905 ]
Resolution		Fixed [ 1 ]
Status	Stalled [ 10000 ]	Closed [ 6 ]

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

Details

Description

Attachments

Issue Links

Activity

People

Dates

Git Integration