Skip to content

MDEV-24931: Fix Bitmap<64>::is_prefix assertion with >64-column NATURAL JOIN on derived table#4756

Open
bodyhedia44 wants to merge 1 commit intoMariaDB:10.11from
bodyhedia44:10.6
Open

MDEV-24931: Fix Bitmap<64>::is_prefix assertion with >64-column NATURAL JOIN on derived table#4756
bodyhedia44 wants to merge 1 commit intoMariaDB:10.11from
bodyhedia44:10.6

Conversation

@bodyhedia44
Copy link
Copy Markdown
Contributor

Problem

When a NATURAL JOIN operates on a derived table (or view with derived_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:

  1. Shift overflow: (key_part_map)(1 << parts) — when parts >= 32, this is undefined behavior (shifting a 32-bit 1 by 32+ bits).
  2. Bitmap overflow: A derived key with 65 parts is created, but key_map (Bitmap<64>) and key_part_map (uint64) can only track 64 bits.
  3. Crash: make_join_statistics() calls eq_part.is_prefix(65) on a Bitmap<64>, hitting the assertion.

How to Reproduce

CREATE DATABASE IF NOT EXISTS test;
USE test;

DROP TABLE IF EXISTS t;

CREATE TABLE t (c1 INT,c2 INT,c3 INT,c4 INT,c5 INT,c6 INT,c7 INT,c8 INT,c9 INT,c10 INT,c11 INT,c12 INT,c13 INT,c14 INT,c15 INT,c16 INT,c17 INT,c18 INT,c19 INT,c20 INT,c21 INT,c22 INT,c23 INT,c24 INT,c25 INT,c26 INT,c27 INT,c28 INT,c29 INT,c30 INT,c31 INT,c32 INT,c33 INT,c34 INT,c35 INT,c36 INT,c37 INT,c38 INT,c39 INT,c40 INT,c41 INT,c42 INT,c43 INT,c44 INT,c45 INT,c46 INT,c47 INT,c48 INT,c49 INT,c50 INT,c51 INT,c52 INT,c53 INT,c54 INT,c55 INT,c56 INT,c57 INT,c58 INT,c59 INT,c60 INT,c61 INT,c62 INT,c63 INT,c64 INT,c65 INT) ENGINE=InnoDB;

SET SESSION optimizer_switch='derived_merge=off';

SELECT * FROM t AS a NATURAL JOIN (SELECT * FROM t) AS b;

DROP TABLE t;

Fix

  • Cap derived keys at sizeof(key_part_map) * 8 (64) parts in generate_derived_keys_for_table().
  • Excess key parts beyond 64 are excluded (keyuse->key = MAX_KEY).
  • Fix the shift to cast before shifting: (key_part_map) 1 << parts instead of (key_part_map)(1 << parts).

Test

Added main.mdev24931 with three cases:

  • 65-column derived table via subquery (the crash case).
  • 65-column view with derived_merge=off (original bug report).
  • 64-column boundary test (should always work).

@gkodinov gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Mar 9, 2026
Copy link
Copy Markdown
Member

@gkodinov gkodinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. This is a preliminary review.

Comment thread 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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be sizeof(key_part_map) IMHO. I'd use table->file->max_key_parts() to get the info from the SE.

Comment thread sql/sql_select.cc Outdated
while (i < count && keyuse->used_tables == first_keyuse->used_tables &&
keyuse->keypart == prev_part);
parts++;
if (parts < max_parts)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the extra check here? You are already checking above.

Comment thread sql/sql_select.cc Outdated
@bodyhedia44
Copy link
Copy Markdown
Contributor Author

Done

Copy link
Copy Markdown
Member

@gkodinov gkodinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM. Please stand by for the final review.

Copy link
Copy Markdown
Member

@mariadb-RexJohnston mariadb-RexJohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase your commit onto 10.11.

Comment thread mysql-test/main/mdev24931.result Outdated
SET SESSION optimizer_switch= @save_optimizer_switch;
DROP VIEW v1;
DROP TABLE t1;
#
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please collapse these 2 tests into one and add a test for joining a CTE.

Comment thread 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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test fail without your patch?

Comment thread 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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add your tests to derived_opt.test

Comment thread sql/sql_select.cc Outdated
Comment thread sql/sql_select.cc
uint max_parts= table->file->max_key_parts();
uint i= 0;

for ( ; i < count && key_count < keys; )
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we not just clip count at table->file->max_key_parts() ?

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 11, 2026

CLA assistant check
All committers have signed the CLA.

@bodyhedia44 bodyhedia44 changed the base branch from 10.6 to 10.11 March 11, 2026 13:47
Comment thread 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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't change the formatting on existing code.

Comment thread sql/sql_select.cc
get_next_field_for_derived_key,
if (table->add_tmp_key(table->s->keys, parts,
get_next_field_for_derived_key,
(uchar *) &first_keyuse,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@mariadb-RexJohnston
Copy link
Copy Markdown
Member

If i checkout your PR, undo your changes to sql_select.cc with
git restore -s c43bf13631f sql_select.cc
then run
mtr main.derived_opt
i would expect the test to fail. It doesn't here. These sorts of differences are usually due to differing storage engines.

As a general approach, it is better to correct data structures upstream than make the rest of the code deal with invalid data.
Here i would expect that the keyuse_array size, incremented beyond what is valid by add_key_part(), shouldn't be.

…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.
@bodyhedia44
Copy link
Copy Markdown
Contributor Author

bodyhedia44 commented Apr 2, 2026

@mariadb-RexJohnston
Hi Rex, thanks for the review.

You're right. I reverted sql_select.cc and ran mtr main.derived_opt it passed. The reason is the test didn't specify ENGINE=InnoDB, so it used MyISAM (the default in MTR). The bug only triggers with InnoDB. I confirmed this by running a standalone test: same 65-column NATURAL JOIN query crashes with ENGINE=InnoDB but passes with MyISAM.

I fixed the test to use --source include/have_innodb.inc and ENGINE=InnoDB. Now mtr main.derived_opt fails without the fix and passes with it.

secondly, I moved the fix upstream to add_key_part(), where the keyuse_array grows. For materialized derived tables, it now stops adding hash-join KEYUSE entries once max_key_parts() is reached, so generate_derived_keys_for_table() never sees more entries than it can handle. I removed the downstream cap (the max_parts check and the while-loop marking excess entries) from generate_derived_keys_for_table().

@gkodinov
Copy link
Copy Markdown
Member

@bodyhedia44 have you forgotten to push the changes you've described? I see no updated commit

@bodyhedia44
Copy link
Copy Markdown
Contributor Author

bodyhedia44 commented Apr 23, 2026

@gkodinov
No, I push it before my message, not after it
I am now waiting @mariadb-RexJohnston review on them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.

Development

Successfully merging this pull request may close these issues.

4 participants