Merge lp:~sergei.glushchenko/percona-server/ps51-128-max-index into lp:percona-server/5.1

Proposed by Sergei Glushchenko
Status: Merged
Approved by: Alexey Kopytov
Approved revision: 477
Merged at revision: 488
Proposed branch: lp:~sergei.glushchenko/percona-server/ps51-128-max-index
Merge into: lp:percona-server/5.1
Diff against target: 172 lines (+64/-4)
10 files modified
Percona-Server/mysql-test/include/have_64_keys.inc (+12/-0)
Percona-Server/mysql-test/r/have_64_keys.require (+14/-0)
Percona-Server/mysql-test/t/create.test (+5/-0)
Percona-Server/mysql-test/t/ps_1general.test (+5/-0)
Percona-Server/mysql-test/t/ps_2myisam.test (+5/-0)
Percona-Server/mysql-test/t/ps_3innodb.test (+5/-0)
Percona-Server/mysql-test/t/ps_4heap.test (+5/-0)
Percona-Server/mysql-test/t/ps_5merge.test (+5/-0)
Percona-Server/mysys/my_bitmap.c (+1/-1)
Percona-Server/sql/sql_bitmap.h (+7/-3)
To merge this branch: bzr merge lp:~sergei.glushchenko/percona-server/ps51-128-max-index
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) Approve
Laurynas Biveinis (community) Approve
Review via email: mp+122520@code.launchpad.net

Description of the change

This is a port from
http://bazaar.launchpad.net/~percona-core/percona-server/rnt-5.1/revision/166

Been tested locally and the only failed tests were
main.create
main.ps_1general
main.ps_2myisam
main.ps_4heap
main.ps_5merge
main.ps_3innodb

which are designed with the assumption that MAX_INDEXES=64.

To post a comment you must log in.
Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

#25824

Revision history for this message
Alexey Kopytov (akopytov) wrote :

As discussed on IRC we should look into making the failing test cases pass (or be skipped) with all --with-max-indexes values.

review: Needs Fixing
Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

include/have_64_keys.inc been added to check whether we have exactly 64 max indexes or not. If we have not
main.create
main.ps_1general
main.ps_2myisam
main.ps_4heap
main.ps_5merge
main.ps_3innodb
are skipped.

http://jenkins.percona.com/view/PS%205.1/job/percona-server-5.1-param/403/

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

Minor comments:

    Please add a comment to have_64_keys explaining what part of the
    metadata confirms that max_indexes = 64.
    Typos: line 39: s/check/checks, line 41: s/skipping/skip. These
    are repeated in other test headers.

    Bitmap::to_ulonglong() returns the first ulonglong-sized bytes of
    the bitmap. In case of default bitmap representation that's the
    whole bitmap, in case of the general bitmap that's a prefix only.
    It does suggest that the code in
    opt_range.cc:get_best_group_min_max() might produce suboptimal
    query plans whenever max_indexes > 64 and actual indexes (key
    parts > 64). I am not sure that we should do anything about it.

review: Needs Fixing
Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

Laurynas,
> Bitmap::to_ulonglong() returns the first ulonglong-sized bytes of
> the bitmap. In case of default bitmap representation that's the
> whole bitmap, in case of the general bitmap that's a prefix only.
> It does suggest that the code in
> opt_range.cc:get_best_group_min_max() might produce suboptimal
> query plans whenever max_indexes > 64 and actual indexes (key
> parts > 64). I am not sure that we should do anything about it.

Code in opt_range.cc can be easily modified to compare prefix by using other operations of Bitmap<> class. More interesting is to construct a good testcase for this issue.

Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

Comments been added and typos been fixed

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

    Diff line 12: "a synonym"
    Diff line 46 and friends: s/if it been done/in this case.

No need for another MP, just edit and re-push the branch. Thanks!

review: Approve
Revision history for this message
Alexey Kopytov (akopytov) wrote :

But we also need a 5.5 MP to be updated.

477. By Sergei Glushchenko

This is a merge of fix for --with-max-indexes=128 by Alexey Kopytov.
This fix originally was committed to lp:percona-server/rnt-5.1.
Original commit message is:
Fix for MySQL bug #54127: mysqld segfaults when built using
--with-max-indexes=128

When using a non-default MAX_KEY value, a different code path is used
when processing index bitmaps. With the default value of 64, the
optimized "template <> class Bitmap<64>" is used which represents and
processes bitmaps as 64-bit integeres. Otherwise, "template <uint
default_width> class Bitmap" is used in which case bitmaps are
represented as arrays.

Multiple problems with the "non-optimized" Bitmap class were discovered
when testing a server binary built with --with-max-indexes=128:

1. bitmap_set_prefix() could overrun the internal buffer when resetting
the remainder of the buffer after setting the prefix due to an
incorrectly calculated remainder's length. This was the reason for the
crash on startup in MySQL bug #54127.

2. Bitmap::intersect() did not take into account that bitmap_init()
resets the supplied buffer, so an intersection with a zero bitmap was
always calculated (reported as MySQL #61178). This led to numerous test
failures due to different execution plans produced by the optimizer.

3. Bitmap::to_ulonglong() incorrectly calculated the result value due to
serious bugs in [u]int*korr/[u]int*store set of macros in
my_global.h (reported as MySQL bugs #61179 and #61180). This led to test
failures in distinct.test and group_min_max.test.

There are still a number of failing tests when running the test suite
with --with-max-indexes=128:

- create.test contains a test case explicitly testing the 64-bit index
limit;

- the ps_N* series of tests verifies the metadata sent by EXPLAIN, where
the field length of "possible_keys" and "key_len" columns depends on
the MAX_KEY value and hence, is different for a binary built with a
non-default value of --with-max-indexes.

The workaround was implemented for failing tests.
include/have_64_keys.inc verify MAX_INDEXES by requesting EXPLAIN
metadata and if MAX_INDEXES is different from default value the
test is skipped.

Revision history for this message
Alexey Kopytov (akopytov) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== added file 'Percona-Server/mysql-test/include/have_64_keys.inc'
--- Percona-Server/mysql-test/include/have_64_keys.inc 1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/include/have_64_keys.inc 2012-09-26 10:50:28 +0000
@@ -0,0 +1,12 @@
1# Check that we have MAX_INDEXES=64
2
3--require r/have_64_keys.require
4
5# Check that maximum length of possible_keys and key_len fields is 4096
6# They are defined in THD::send_explain_fields as NAME_CHAR_LEN*MAX_KEY
7# where NAME_CHAR_LEN is always 64 (mysql_com.h) and MAX_KEY is
8# a synonym of MAX_INDEXES
9
10--enable_metadata
11EXPLAIN SELECT 1;
12--disable_metadata
013
=== added file 'Percona-Server/mysql-test/r/have_64_keys.require'
--- Percona-Server/mysql-test/r/have_64_keys.require 1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/r/have_64_keys.require 2012-09-26 10:50:28 +0000
@@ -0,0 +1,14 @@
1EXPLAIN SELECT 1;
2Catalog Database Table Table_alias Column Column_alias Type Length Max length Is_null Flags Decimals Charsetnr
3def id 8 3 1 N 32929 0 63
4def select_type 253 19 6 N 1 31 8
5def table 253 64 0 Y 0 31 8
6def type 253 10 0 Y 0 31 8
7def possible_keys 253 4096 0 Y 0 31 8
8def key 253 64 0 Y 0 31 8
9def key_len 253 4096 0 Y 0 31 8
10def ref 253 1024 0 Y 0 31 8
11def rows 8 10 0 Y 32928 0 63
12def Extra 253 255 14 N 1 31 8
13id select_type table type possible_keys key key_len ref rows Extra
141 SIMPLE NULL NULL NULL NULL NULL NULL NULL No tables used
015
=== modified file 'Percona-Server/mysql-test/t/create.test'
--- Percona-Server/mysql-test/t/create.test 2010-01-16 07:44:24 +0000
+++ Percona-Server/mysql-test/t/create.test 2012-09-26 10:50:28 +0000
@@ -2,6 +2,11 @@
2# Check some special create statements.2# Check some special create statements.
3#3#
44
5# This test checks some limits which are related to MAX_INDEXES constant
6# which could be set to non-default value at build time.
7# We just skip the test in this case.
8--source include/have_64_keys.inc
9
5--disable_warnings10--disable_warnings
6drop table if exists t1,t2,t3,t4,t5;11drop table if exists t1,t2,t3,t4,t5;
7drop database if exists mysqltest;12drop database if exists mysqltest;
813
=== modified file 'Percona-Server/mysql-test/t/ps_1general.test'
--- Percona-Server/mysql-test/t/ps_1general.test 2009-04-01 08:58:55 +0000
+++ Percona-Server/mysql-test/t/ps_1general.test 2012-09-26 10:50:28 +0000
@@ -4,6 +4,11 @@
4# #4# #
5##############################################################5##############################################################
66
7# This test checks some limits which are related to MAX_INDEXES constant
8# which could be set to non-default value at build time.
9# We just skip the test in this case.
10--source include/have_64_keys.inc
11
7# 12#
8# NOTE: PLEASE SEE THE DETAILED DESCRIPTION AT THE BOTTOM OF THIS FILE13# NOTE: PLEASE SEE THE DETAILED DESCRIPTION AT THE BOTTOM OF THIS FILE
9# BEFORE ADDING NEW TEST CASES HERE !!!14# BEFORE ADDING NEW TEST CASES HERE !!!
1015
=== modified file 'Percona-Server/mysql-test/t/ps_2myisam.test'
--- Percona-Server/mysql-test/t/ps_2myisam.test 2005-07-28 00:22:47 +0000
+++ Percona-Server/mysql-test/t/ps_2myisam.test 2012-09-26 10:50:28 +0000
@@ -4,6 +4,11 @@
4# #4# #
5###############################################5###############################################
66
7# This test checks some limits which are related to MAX_INDEXES constant
8# which could be set to non-default value at build time.
9# We just skip the test in this case.
10--source include/have_64_keys.inc
11
7# 12#
8# NOTE: PLEASE SEE ps_1general.test (bottom) 13# NOTE: PLEASE SEE ps_1general.test (bottom)
9# BEFORE ADDING NEW TEST CASES HERE !!!14# BEFORE ADDING NEW TEST CASES HERE !!!
1015
=== modified file 'Percona-Server/mysql-test/t/ps_3innodb.test'
--- Percona-Server/mysql-test/t/ps_3innodb.test 2010-06-09 12:07:34 +0000
+++ Percona-Server/mysql-test/t/ps_3innodb.test 2012-09-26 10:50:28 +0000
@@ -4,6 +4,11 @@
4# #4# #
5###############################################5###############################################
66
7# This test checks some limits which are related to MAX_INDEXES constant
8# which could be set to non-default value at build time.
9# We just skip the test in this case.
10--source include/have_64_keys.inc
11
7# 12#
8# NOTE: PLEASE SEE ps_1general.test (bottom) 13# NOTE: PLEASE SEE ps_1general.test (bottom)
9# BEFORE ADDING NEW TEST CASES HERE !!!14# BEFORE ADDING NEW TEST CASES HERE !!!
1015
=== modified file 'Percona-Server/mysql-test/t/ps_4heap.test'
--- Percona-Server/mysql-test/t/ps_4heap.test 2005-07-28 14:09:54 +0000
+++ Percona-Server/mysql-test/t/ps_4heap.test 2012-09-26 10:50:28 +0000
@@ -4,6 +4,11 @@
4# #4# #
5###############################################5###############################################
66
7# This test checks some limits which are related to MAX_INDEXES constant
8# which could be set to non-default value at build time.
9# We just skip the test in this case.
10--source include/have_64_keys.inc
11
7# 12#
8# NOTE: PLEASE SEE ps_1general.test (bottom) 13# NOTE: PLEASE SEE ps_1general.test (bottom)
9# BEFORE ADDING NEW TEST CASES HERE !!!14# BEFORE ADDING NEW TEST CASES HERE !!!
1015
=== modified file 'Percona-Server/mysql-test/t/ps_5merge.test'
--- Percona-Server/mysql-test/t/ps_5merge.test 2005-07-28 14:09:54 +0000
+++ Percona-Server/mysql-test/t/ps_5merge.test 2012-09-26 10:50:28 +0000
@@ -4,6 +4,11 @@
4# #4# #
5###############################################5###############################################
66
7# This test checks some limits which are related to MAX_INDEXES constant
8# which could be set to non-default value at build time.
9# We just skip the test in this case.
10--source include/have_64_keys.inc
11
7# 12#
8# NOTE: PLEASE SEE ps_1general.test (bottom) 13# NOTE: PLEASE SEE ps_1general.test (bottom)
9# BEFORE ADDING NEW TEST CASES HERE !!!14# BEFORE ADDING NEW TEST CASES HERE !!!
1015
=== modified file 'Percona-Server/mysys/my_bitmap.c'
--- Percona-Server/mysys/my_bitmap.c 2011-07-03 15:47:37 +0000
+++ Percona-Server/mysys/my_bitmap.c 2012-09-26 10:50:28 +0000
@@ -303,7 +303,7 @@
303 m+= prefix_bytes;303 m+= prefix_bytes;
304 if ((prefix_bits= prefix_size & 7))304 if ((prefix_bits= prefix_size & 7))
305 *(m++)= (1 << prefix_bits)-1;305 *(m++)= (1 << prefix_bits)-1;
306 if ((d= no_bytes_in_map(map)-prefix_bytes))306 if ((d= no_bytes_in_map(map) - (m - (uchar *) map->bitmap)))
307 bzero(m, d);307 bzero(m, d);
308}308}
309309
310310
=== modified file 'Percona-Server/sql/sql_bitmap.h'
--- Percona-Server/sql/sql_bitmap.h 2008-01-29 11:14:34 +0000
+++ Percona-Server/sql/sql_bitmap.h 2012-09-26 10:50:28 +0000
@@ -47,7 +47,11 @@
47 void intersect(ulonglong map2buff)47 void intersect(ulonglong map2buff)
48 {48 {
49 MY_BITMAP map2;49 MY_BITMAP map2;
50 bitmap_init(&map2, (uint32 *)&map2buff, sizeof(ulonglong)*8, 0);50 ulonglong buf;
51
52 /* bitmap_init() zeroes the supplied buffer */
53 bitmap_init(&map2, (uint32 *)&buf, sizeof(ulonglong)*8, 0);
54 buf= map2buff;
51 bitmap_intersect(&map, &map2);55 bitmap_intersect(&map, &map2);
52 }56 }
53 /* Use highest bit for all bits above sizeof(ulonglong)*8. */57 /* Use highest bit for all bits above sizeof(ulonglong)*8. */
@@ -87,9 +91,9 @@
87 ulonglong to_ulonglong() const91 ulonglong to_ulonglong() const
88 {92 {
89 if (sizeof(buffer) >= 8)93 if (sizeof(buffer) >= 8)
90 return uint8korr(buffer);94 return uint8korr((uchar *) buffer);
91 DBUG_ASSERT(sizeof(buffer) >= 4);95 DBUG_ASSERT(sizeof(buffer) >= 4);
92 return (ulonglong) uint4korr(buffer);96 return (ulonglong) uint4korr((uchar *) buffer);
93 }97 }
94};98};
9599

Subscribers

People subscribed via source and target branches