Re: [Maria-developers] [Commits] Rev 4445: MDEV-6878: Use of uninitialized saved_primary_key in Mrr_ordered_index_reader::resume_read() in file:///home/psergey/dev2/10.0/
Sergey, Nice test case. Would you mind cleaning up the end-space in your patch? Also, is there a reason to put the entire body of Mrr_ordered_index_reader::resume_read in the if() instead of an early return? Regards, Jeremy On Thu, Oct 16, 2014 at 11:58 AM, Sergey Petrunya <psergey@askmonty.org> wrote:
At file:///home/psergey/dev2/10.0/
------------------------------------------------------------ revno: 4445 revision-id: psergey@askmonty.org-20141016135713-md92qr4g9fzhitxh parent: bar@mnogosearch.org-20141013083155-j3j31hqhfc8i95y1 committer: Sergey Petrunya <psergey@askmonty.org> branch nick: 10.0 timestamp: Thu 2014-10-16 17:57:13 +0400 message: MDEV-6878: Use of uninitialized saved_primary_key in Mrr_ordered_index_reader::resume_read()
- Make Mrr_ordered_index_reader::resume_read() restore index position only if it was saved before with Mrr_ordered_index_reader::interrupt_read(). === modified file 'mysql-test/r/innodb_mrr_cpk.result' --- a/mysql-test/r/innodb_mrr_cpk.result 2013-09-26 18:20:15 +0000 +++ b/mysql-test/r/innodb_mrr_cpk.result 2014-10-16 13:57:13 +0000 @@ -194,3 +194,41 @@ id select_type table type possible_keys 2 DERIVED t2 ALL NULL NULL NULL NULL # set join_cache_level= @tmp_mdev5037; drop table t0,t1,t2; +# +# MDEV-6878: Use of uninitialized saved_primary_key in Mrr_ordered_index_reader::resume_read() +# +create table t0(a int); +insert into t0 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); +create table t1 ( +pk varchar(32) character set utf8 primary key, +kp1 char(32) not null, +col1 varchar(32), +key (kp1) +) engine=innodb; +insert into t1 +select +concat('pk-', 1000 +A.a), +concat('kp1-', 1000 +A.a), +concat('val-', 1000 +A.a) +from test.t0 A ; +create table t2 as select kp1 as a from t1; +set join_cache_level=8; +set optimizer_switch='mrr=on,mrr_sort_keys=on'; +explain +select * from t2 straight_join t1 force index(kp1) where t1.kp1=t2.a; +id select_type table type possible_keys key key_len ref rows Extra +1 SIMPLE t2 ALL NULL NULL NULL NULL 10 +1 SIMPLE t1 ref kp1 kp1 32 test.t2.a 1 Using join buffer (flat, BKAH join); Key-ordered Rowid-ordered scan +select * from t2 straight_join t1 force index(kp1) where t1.kp1=t2.a; +a pk kp1 col1 +kp1-1000 pk-1000 kp1-1000 val-1000 +kp1-1001 pk-1001 kp1-1001 val-1001 +kp1-1002 pk-1002 kp1-1002 val-1002 +kp1-1003 pk-1003 kp1-1003 val-1003 +kp1-1004 pk-1004 kp1-1004 val-1004 +kp1-1005 pk-1005 kp1-1005 val-1005 +kp1-1006 pk-1006 kp1-1006 val-1006 +kp1-1007 pk-1007 kp1-1007 val-1007 +kp1-1008 pk-1008 kp1-1008 val-1008 +kp1-1009 pk-1009 kp1-1009 val-1009 +drop table t0,t1,t2;
=== modified file 'mysql-test/t/innodb_mrr_cpk.test' --- a/mysql-test/t/innodb_mrr_cpk.test 2013-09-26 18:20:15 +0000 +++ b/mysql-test/t/innodb_mrr_cpk.test 2014-10-16 13:57:13 +0000 @@ -192,3 +192,35 @@ explain SELECT 1 FROM (SELECT url, id FR set join_cache_level= @tmp_mdev5037;
drop table t0,t1,t2; + +--echo # +--echo # MDEV-6878: Use of uninitialized saved_primary_key in Mrr_ordered_index_reader::resume_read() +--echo # +create table t0(a int); +insert into t0 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); + +create table t1 ( + pk varchar(32) character set utf8 primary key, + kp1 char(32) not null, + col1 varchar(32), + key (kp1) +) engine=innodb; + +insert into t1 +select + concat('pk-', 1000 +A.a), + concat('kp1-', 1000 +A.a), + concat('val-', 1000 +A.a) +from test.t0 A ; + +create table t2 as select kp1 as a from t1; + +set join_cache_level=8; +set optimizer_switch='mrr=on,mrr_sort_keys=on'; +explain +select * from t2 straight_join t1 force index(kp1) where t1.kp1=t2.a; +select * from t2 straight_join t1 force index(kp1) where t1.kp1=t2.a; + +drop table t0,t1,t2; + +
=== modified file 'sql/multi_range_read.cc' --- a/sql/multi_range_read.cc 2014-06-06 17:28:42 +0000 +++ b/sql/multi_range_read.cc 2014-10-16 13:57:13 +0000 @@ -426,6 +426,7 @@ bool Mrr_ordered_index_reader::set_inter *space_start += key_len;
have_saved_rowid= FALSE; + read_was_interrupted= FALSE; return FALSE; }
@@ -434,6 +435,7 @@ void Mrr_ordered_index_reader::set_no_in support_scan_interruptions= FALSE; saved_key_tuple= saved_rowid= saved_primary_key= NULL; /* safety */ have_saved_rowid= FALSE; + read_was_interrupted= FALSE; }
void Mrr_ordered_index_reader::interrupt_read() @@ -451,6 +453,7 @@ void Mrr_ordered_index_reader::interrupt &table->key_info[table->s->primary_key], table->key_info[table->s->primary_key].key_length); } + read_was_interrupted= TRUE;
/* Save the last rowid */ memcpy(saved_rowid, file->ref, file->ref_length); @@ -468,14 +471,17 @@ void Mrr_ordered_index_reader::position( void Mrr_ordered_index_reader::resume_read() { TABLE *table= file->get_table(); - KEY *used_index= &table->key_info[file->active_index]; - key_restore(table->record[0], saved_key_tuple, - used_index, used_index->key_length); - if (saved_primary_key) + if (read_was_interrupted) { - key_restore(table->record[0], saved_primary_key, - &table->key_info[table->s->primary_key], - table->key_info[table->s->primary_key].key_length); + KEY *used_index= &table->key_info[file->active_index]; + key_restore(table->record[0], saved_key_tuple, + used_index, used_index->key_length); + if (saved_primary_key) + { + key_restore(table->record[0], saved_primary_key, + &table->key_info[table->s->primary_key], + table->key_info[table->s->primary_key].key_length); + } } }
@@ -557,8 +563,7 @@ int Mrr_ordered_index_reader::init(handl is_mrr_assoc= !MY_TEST(mode & HA_MRR_NO_ASSOCIATION); mrr_funcs= *seq_funcs; source_exhausted= FALSE; - if (support_scan_interruptions) - bzero(saved_key_tuple, key_info->key_length); + read_was_interrupted= false; have_saved_rowid= FALSE; return 0; }
=== modified file 'sql/multi_range_read.h' --- a/sql/multi_range_read.h 2013-09-26 18:20:15 +0000 +++ b/sql/multi_range_read.h 2014-10-16 13:57:13 +0000 @@ -339,6 +339,12 @@ class Mrr_ordered_index_reader : public
uchar *saved_key_tuple; /* Saved current key tuple */ uchar *saved_primary_key; /* Saved current primary key tuple */ + + /* + TRUE<=> saved_key_tuple (and saved_primary_key when applicable) have + valid values. + */ + bool read_was_interrupted;
static int compare_keys(void* arg, uchar* key1, uchar* key2); static int compare_keys_reverse(void* arg, uchar* key1, uchar* key2);
_______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
Hi Jeremy, On Thu, Oct 16, 2014 at 01:09:31PM -0700, Jeremy Cole wrote:
Nice test case.
Would you mind cleaning up the end-space in your patch?
Yes, I can
Also, is there a reason to put the entire body of Mrr_ordered_index_reader::resume_read in the if() instead of an early return?
Since the function has no DBUG_ENTER/DBUG_RETURN, no specific reason. I've committed a new variant: http://lists.askmonty.org/pipermail/commits/2014-October/006815.html is this one better? (If yes, I'll merge it, overwriting the previous variant)
Regards,
Jeremy
On Thu, Oct 16, 2014 at 11:58 AM, Sergey Petrunya <psergey@askmonty.org> wrote:
At file:///home/psergey/dev2/10.0/
------------------------------------------------------------ revno: 4445 revision-id: psergey@askmonty.org-20141016135713-md92qr4g9fzhitxh parent: bar@mnogosearch.org-20141013083155-j3j31hqhfc8i95y1 committer: Sergey Petrunya <psergey@askmonty.org> branch nick: 10.0 timestamp: Thu 2014-10-16 17:57:13 +0400 message: MDEV-6878: Use of uninitialized saved_primary_key in Mrr_ordered_index_reader::resume_read()
- Make Mrr_ordered_index_reader::resume_read() restore index position only if it was saved before with Mrr_ordered_index_reader::interrupt_read(). === modified file 'mysql-test/r/innodb_mrr_cpk.result' --- a/mysql-test/r/innodb_mrr_cpk.result 2013-09-26 18:20:15 +0000 +++ b/mysql-test/r/innodb_mrr_cpk.result 2014-10-16 13:57:13 +0000 @@ -194,3 +194,41 @@ id select_type table type possible_keys 2 DERIVED t2 ALL NULL NULL NULL NULL # set join_cache_level= @tmp_mdev5037; drop table t0,t1,t2; +# +# MDEV-6878: Use of uninitialized saved_primary_key in Mrr_ordered_index_reader::resume_read() +# +create table t0(a int); +insert into t0 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); +create table t1 ( +pk varchar(32) character set utf8 primary key, +kp1 char(32) not null, +col1 varchar(32), +key (kp1) +) engine=innodb; +insert into t1 +select +concat('pk-', 1000 +A.a), +concat('kp1-', 1000 +A.a), +concat('val-', 1000 +A.a) +from test.t0 A ; +create table t2 as select kp1 as a from t1; +set join_cache_level=8; +set optimizer_switch='mrr=on,mrr_sort_keys=on'; +explain +select * from t2 straight_join t1 force index(kp1) where t1.kp1=t2.a; +id select_type table type possible_keys key key_len ref rows Extra +1 SIMPLE t2 ALL NULL NULL NULL NULL 10 +1 SIMPLE t1 ref kp1 kp1 32 test.t2.a 1 Using join buffer (flat, BKAH join); Key-ordered Rowid-ordered scan +select * from t2 straight_join t1 force index(kp1) where t1.kp1=t2.a; +a pk kp1 col1 +kp1-1000 pk-1000 kp1-1000 val-1000 +kp1-1001 pk-1001 kp1-1001 val-1001 +kp1-1002 pk-1002 kp1-1002 val-1002 +kp1-1003 pk-1003 kp1-1003 val-1003 +kp1-1004 pk-1004 kp1-1004 val-1004 +kp1-1005 pk-1005 kp1-1005 val-1005 +kp1-1006 pk-1006 kp1-1006 val-1006 +kp1-1007 pk-1007 kp1-1007 val-1007 +kp1-1008 pk-1008 kp1-1008 val-1008 +kp1-1009 pk-1009 kp1-1009 val-1009 +drop table t0,t1,t2;
=== modified file 'mysql-test/t/innodb_mrr_cpk.test' --- a/mysql-test/t/innodb_mrr_cpk.test 2013-09-26 18:20:15 +0000 +++ b/mysql-test/t/innodb_mrr_cpk.test 2014-10-16 13:57:13 +0000 @@ -192,3 +192,35 @@ explain SELECT 1 FROM (SELECT url, id FR set join_cache_level= @tmp_mdev5037;
drop table t0,t1,t2; + +--echo # +--echo # MDEV-6878: Use of uninitialized saved_primary_key in Mrr_ordered_index_reader::resume_read() +--echo # +create table t0(a int); +insert into t0 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); + +create table t1 ( + pk varchar(32) character set utf8 primary key, + kp1 char(32) not null, + col1 varchar(32), + key (kp1) +) engine=innodb; + +insert into t1 +select + concat('pk-', 1000 +A.a), + concat('kp1-', 1000 +A.a), + concat('val-', 1000 +A.a) +from test.t0 A ; + +create table t2 as select kp1 as a from t1; + +set join_cache_level=8; +set optimizer_switch='mrr=on,mrr_sort_keys=on'; +explain +select * from t2 straight_join t1 force index(kp1) where t1.kp1=t2.a; +select * from t2 straight_join t1 force index(kp1) where t1.kp1=t2.a; + +drop table t0,t1,t2; + +
=== modified file 'sql/multi_range_read.cc' --- a/sql/multi_range_read.cc 2014-06-06 17:28:42 +0000 +++ b/sql/multi_range_read.cc 2014-10-16 13:57:13 +0000 @@ -426,6 +426,7 @@ bool Mrr_ordered_index_reader::set_inter *space_start += key_len;
have_saved_rowid= FALSE; + read_was_interrupted= FALSE; return FALSE; }
@@ -434,6 +435,7 @@ void Mrr_ordered_index_reader::set_no_in support_scan_interruptions= FALSE; saved_key_tuple= saved_rowid= saved_primary_key= NULL; /* safety */ have_saved_rowid= FALSE; + read_was_interrupted= FALSE; }
void Mrr_ordered_index_reader::interrupt_read() @@ -451,6 +453,7 @@ void Mrr_ordered_index_reader::interrupt &table->key_info[table->s->primary_key], table->key_info[table->s->primary_key].key_length); } + read_was_interrupted= TRUE;
/* Save the last rowid */ memcpy(saved_rowid, file->ref, file->ref_length); @@ -468,14 +471,17 @@ void Mrr_ordered_index_reader::position( void Mrr_ordered_index_reader::resume_read() { TABLE *table= file->get_table(); - KEY *used_index= &table->key_info[file->active_index]; - key_restore(table->record[0], saved_key_tuple, - used_index, used_index->key_length); - if (saved_primary_key) + if (read_was_interrupted) { - key_restore(table->record[0], saved_primary_key, - &table->key_info[table->s->primary_key], - table->key_info[table->s->primary_key].key_length); + KEY *used_index= &table->key_info[file->active_index]; + key_restore(table->record[0], saved_key_tuple, + used_index, used_index->key_length); + if (saved_primary_key) + { + key_restore(table->record[0], saved_primary_key, + &table->key_info[table->s->primary_key], + table->key_info[table->s->primary_key].key_length); + } } }
@@ -557,8 +563,7 @@ int Mrr_ordered_index_reader::init(handl is_mrr_assoc= !MY_TEST(mode & HA_MRR_NO_ASSOCIATION); mrr_funcs= *seq_funcs; source_exhausted= FALSE; - if (support_scan_interruptions) - bzero(saved_key_tuple, key_info->key_length); + read_was_interrupted= false; have_saved_rowid= FALSE; return 0; }
=== modified file 'sql/multi_range_read.h' --- a/sql/multi_range_read.h 2013-09-26 18:20:15 +0000 +++ b/sql/multi_range_read.h 2014-10-16 13:57:13 +0000 @@ -339,6 +339,12 @@ class Mrr_ordered_index_reader : public
uchar *saved_key_tuple; /* Saved current key tuple */ uchar *saved_primary_key; /* Saved current primary key tuple */ + + /* + TRUE<=> saved_key_tuple (and saved_primary_key when applicable) have + valid values. + */ + bool read_was_interrupted;
static int compare_keys(void* arg, uchar* key1, uchar* key2); static int compare_keys_reverse(void* arg, uchar* key1, uchar* key2);
_______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
-- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog
participants (2)
-
Jeremy Cole -
Sergey Petrunia