MDEV-24931: Fix Bitmap<64>::is_prefix assertion with >64-column NATURAL JOIN on derived table#4756
MDEV-24931: Fix Bitmap<64>::is_prefix assertion with >64-column NATURAL JOIN on derived table#4756bodyhedia44 wants to merge 1 commit intoMariaDB:10.11from
Conversation
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution. This is a preliminary review.
sql/sql_select.cc
Outdated
| KEYUSE *first_keyuse= keyuse; | ||
| uint prev_part= keyuse->keypart; | ||
| uint parts= 0; | ||
| uint max_parts= sizeof(key_part_map) * 8; |
There was a problem hiding this comment.
This should not be sizeof(key_part_map) IMHO. I'd use table->file->max_key_parts() to get the info from the SE.
sql/sql_select.cc
Outdated
| while (i < count && keyuse->used_tables == first_keyuse->used_tables && | ||
| keyuse->keypart == prev_part); | ||
| parts++; | ||
| if (parts < max_parts) |
There was a problem hiding this comment.
why the extra check here? You are already checking above.
|
Done |
gkodinov
left a comment
There was a problem hiding this comment.
Thanks. LGTM. Please stand by for the final review.
mariadb-RexJohnston
left a comment
There was a problem hiding this comment.
Please rebase your commit onto 10.11.
mysql-test/main/mdev24931.result
Outdated
| SET SESSION optimizer_switch= @save_optimizer_switch; | ||
| DROP VIEW v1; | ||
| DROP TABLE t1; | ||
| # |
There was a problem hiding this comment.
please collapse these 2 tests into one and add a test for joining a CTE.
mysql-test/main/mdev24931.result
Outdated
| SELECT * FROM t64 AS a NATURAL JOIN (SELECT * FROM t64) AS b; | ||
| c1 c2 c3 c4 c5 c6 c7 c8 c9 c10 c11 c12 c13 c14 c15 c16 c17 c18 c19 c20 c21 c22 c23 c24 c25 c26 c27 c28 c29 c30 c31 c32 c33 c34 c35 c36 c37 c38 c39 c40 c41 c42 c43 c44 c45 c46 c47 c48 c49 c50 c51 c52 c53 c54 c55 c56 c57 c58 c59 c60 c61 c62 c63 c64 | ||
| SET SESSION optimizer_switch= @save_optimizer_switch; | ||
| DROP TABLE t64; |
There was a problem hiding this comment.
Does this test fail without your patch?
mysql-test/main/mdev24931.test
Outdated
| SELECT * FROM t64 AS a NATURAL JOIN (SELECT * FROM t64) AS b; | ||
|
|
||
| SET SESSION optimizer_switch= @save_optimizer_switch; | ||
| DROP TABLE t64; |
There was a problem hiding this comment.
Please add your tests to derived_opt.test
| uint max_parts= table->file->max_key_parts(); | ||
| uint i= 0; | ||
|
|
||
| for ( ; i < count && key_count < keys; ) |
There was a problem hiding this comment.
could we not just clip count at table->file->max_key_parts() ?
sql/sql_select.cc
Outdated
| { | ||
| keyuse->key= table->s->keys; | ||
| keyuse->keypart_map= (key_part_map) (1 << parts); | ||
| keyuse->keypart_map= (key_part_map) 1 << parts; |
There was a problem hiding this comment.
please don't change the formatting on existing code.
| get_next_field_for_derived_key, | ||
| if (table->add_tmp_key(table->s->keys, parts, | ||
| get_next_field_for_derived_key, | ||
| (uchar *) &first_keyuse, |
…AL JOIN on derived table When a NATURAL JOIN operates on a derived table (or view with derived_merge=off) having more than 64 columns, the optimizer's generate_derived_keys_for_table() would: 1) Overflow a 32-bit shift: (key_part_map)(1 << parts) when parts >= 32 2) Create a derived key with more parts than Bitmap<64>/key_part_map can hold 3) Crash on DBUG_ASSERT(prefix_size <= width) in Bitmap<64>::is_prefix(65) Fix: cap derived keys at sizeof(key_part_map)*8 (64) parts. Excess columns are excluded from the key (keyuse->key = MAX_KEY). Also fix the shift to cast before shifting: (key_part_map) 1 << parts.
Problem
When a
NATURAL JOINoperates on a derived table (or view withderived_merge=off) having more than 64 columns, the server crashes with:Assertion `prefix_size <= width' failed in Bitmap<64u>::is_prefix
UBSAN also reports: shift exponent 32 is too large for 32-bit type 'int'
Root Cause
generate_derived_keys_for_table() in sql_select.cc creates derived keys with no cap on the number of key parts. When a 65-column table is NATURAL JOINed:
1by 32+ bits).How to Reproduce
Fix
Test
Added
main.mdev24931with three cases:derived_merge=off(original bug report).