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
1=== added file 'Percona-Server/mysql-test/include/have_64_keys.inc'
2--- Percona-Server/mysql-test/include/have_64_keys.inc 1970-01-01 00:00:00 +0000
3+++ Percona-Server/mysql-test/include/have_64_keys.inc 2012-09-26 10:50:28 +0000
4@@ -0,0 +1,12 @@
5+# Check that we have MAX_INDEXES=64
6+
7+--require r/have_64_keys.require
8+
9+# Check that maximum length of possible_keys and key_len fields is 4096
10+# They are defined in THD::send_explain_fields as NAME_CHAR_LEN*MAX_KEY
11+# where NAME_CHAR_LEN is always 64 (mysql_com.h) and MAX_KEY is
12+# a synonym of MAX_INDEXES
13+
14+--enable_metadata
15+EXPLAIN SELECT 1;
16+--disable_metadata
17
18=== added file 'Percona-Server/mysql-test/r/have_64_keys.require'
19--- Percona-Server/mysql-test/r/have_64_keys.require 1970-01-01 00:00:00 +0000
20+++ Percona-Server/mysql-test/r/have_64_keys.require 2012-09-26 10:50:28 +0000
21@@ -0,0 +1,14 @@
22+EXPLAIN SELECT 1;
23+Catalog Database Table Table_alias Column Column_alias Type Length Max length Is_null Flags Decimals Charsetnr
24+def id 8 3 1 N 32929 0 63
25+def select_type 253 19 6 N 1 31 8
26+def table 253 64 0 Y 0 31 8
27+def type 253 10 0 Y 0 31 8
28+def possible_keys 253 4096 0 Y 0 31 8
29+def key 253 64 0 Y 0 31 8
30+def key_len 253 4096 0 Y 0 31 8
31+def ref 253 1024 0 Y 0 31 8
32+def rows 8 10 0 Y 32928 0 63
33+def Extra 253 255 14 N 1 31 8
34+id select_type table type possible_keys key key_len ref rows Extra
35+1 SIMPLE NULL NULL NULL NULL NULL NULL NULL No tables used
36
37=== modified file 'Percona-Server/mysql-test/t/create.test'
38--- Percona-Server/mysql-test/t/create.test 2010-01-16 07:44:24 +0000
39+++ Percona-Server/mysql-test/t/create.test 2012-09-26 10:50:28 +0000
40@@ -2,6 +2,11 @@
41 # Check some special create statements.
42 #
43
44+# This test checks some limits which are related to MAX_INDEXES constant
45+# which could be set to non-default value at build time.
46+# We just skip the test in this case.
47+--source include/have_64_keys.inc
48+
49 --disable_warnings
50 drop table if exists t1,t2,t3,t4,t5;
51 drop database if exists mysqltest;
52
53=== modified file 'Percona-Server/mysql-test/t/ps_1general.test'
54--- Percona-Server/mysql-test/t/ps_1general.test 2009-04-01 08:58:55 +0000
55+++ Percona-Server/mysql-test/t/ps_1general.test 2012-09-26 10:50:28 +0000
56@@ -4,6 +4,11 @@
57 # #
58 ##############################################################
59
60+# This test checks some limits which are related to MAX_INDEXES constant
61+# which could be set to non-default value at build time.
62+# We just skip the test in this case.
63+--source include/have_64_keys.inc
64+
65 #
66 # NOTE: PLEASE SEE THE DETAILED DESCRIPTION AT THE BOTTOM OF THIS FILE
67 # BEFORE ADDING NEW TEST CASES HERE !!!
68
69=== modified file 'Percona-Server/mysql-test/t/ps_2myisam.test'
70--- Percona-Server/mysql-test/t/ps_2myisam.test 2005-07-28 00:22:47 +0000
71+++ Percona-Server/mysql-test/t/ps_2myisam.test 2012-09-26 10:50:28 +0000
72@@ -4,6 +4,11 @@
73 # #
74 ###############################################
75
76+# This test checks some limits which are related to MAX_INDEXES constant
77+# which could be set to non-default value at build time.
78+# We just skip the test in this case.
79+--source include/have_64_keys.inc
80+
81 #
82 # NOTE: PLEASE SEE ps_1general.test (bottom)
83 # BEFORE ADDING NEW TEST CASES HERE !!!
84
85=== modified file 'Percona-Server/mysql-test/t/ps_3innodb.test'
86--- Percona-Server/mysql-test/t/ps_3innodb.test 2010-06-09 12:07:34 +0000
87+++ Percona-Server/mysql-test/t/ps_3innodb.test 2012-09-26 10:50:28 +0000
88@@ -4,6 +4,11 @@
89 # #
90 ###############################################
91
92+# This test checks some limits which are related to MAX_INDEXES constant
93+# which could be set to non-default value at build time.
94+# We just skip the test in this case.
95+--source include/have_64_keys.inc
96+
97 #
98 # NOTE: PLEASE SEE ps_1general.test (bottom)
99 # BEFORE ADDING NEW TEST CASES HERE !!!
100
101=== modified file 'Percona-Server/mysql-test/t/ps_4heap.test'
102--- Percona-Server/mysql-test/t/ps_4heap.test 2005-07-28 14:09:54 +0000
103+++ Percona-Server/mysql-test/t/ps_4heap.test 2012-09-26 10:50:28 +0000
104@@ -4,6 +4,11 @@
105 # #
106 ###############################################
107
108+# This test checks some limits which are related to MAX_INDEXES constant
109+# which could be set to non-default value at build time.
110+# We just skip the test in this case.
111+--source include/have_64_keys.inc
112+
113 #
114 # NOTE: PLEASE SEE ps_1general.test (bottom)
115 # BEFORE ADDING NEW TEST CASES HERE !!!
116
117=== modified file 'Percona-Server/mysql-test/t/ps_5merge.test'
118--- Percona-Server/mysql-test/t/ps_5merge.test 2005-07-28 14:09:54 +0000
119+++ Percona-Server/mysql-test/t/ps_5merge.test 2012-09-26 10:50:28 +0000
120@@ -4,6 +4,11 @@
121 # #
122 ###############################################
123
124+# This test checks some limits which are related to MAX_INDEXES constant
125+# which could be set to non-default value at build time.
126+# We just skip the test in this case.
127+--source include/have_64_keys.inc
128+
129 #
130 # NOTE: PLEASE SEE ps_1general.test (bottom)
131 # BEFORE ADDING NEW TEST CASES HERE !!!
132
133=== modified file 'Percona-Server/mysys/my_bitmap.c'
134--- Percona-Server/mysys/my_bitmap.c 2011-07-03 15:47:37 +0000
135+++ Percona-Server/mysys/my_bitmap.c 2012-09-26 10:50:28 +0000
136@@ -303,7 +303,7 @@
137 m+= prefix_bytes;
138 if ((prefix_bits= prefix_size & 7))
139 *(m++)= (1 << prefix_bits)-1;
140- if ((d= no_bytes_in_map(map)-prefix_bytes))
141+ if ((d= no_bytes_in_map(map) - (m - (uchar *) map->bitmap)))
142 bzero(m, d);
143 }
144
145
146=== modified file 'Percona-Server/sql/sql_bitmap.h'
147--- Percona-Server/sql/sql_bitmap.h 2008-01-29 11:14:34 +0000
148+++ Percona-Server/sql/sql_bitmap.h 2012-09-26 10:50:28 +0000
149@@ -47,7 +47,11 @@
150 void intersect(ulonglong map2buff)
151 {
152 MY_BITMAP map2;
153- bitmap_init(&map2, (uint32 *)&map2buff, sizeof(ulonglong)*8, 0);
154+ ulonglong buf;
155+
156+ /* bitmap_init() zeroes the supplied buffer */
157+ bitmap_init(&map2, (uint32 *)&buf, sizeof(ulonglong)*8, 0);
158+ buf= map2buff;
159 bitmap_intersect(&map, &map2);
160 }
161 /* Use highest bit for all bits above sizeof(ulonglong)*8. */
162@@ -87,9 +91,9 @@
163 ulonglong to_ulonglong() const
164 {
165 if (sizeof(buffer) >= 8)
166- return uint8korr(buffer);
167+ return uint8korr((uchar *) buffer);
168 DBUG_ASSERT(sizeof(buffer) >= 4);
169- return (ulonglong) uint4korr(buffer);
170+ return (ulonglong) uint4korr((uchar *) buffer);
171 }
172 };
173

Subscribers

People subscribed via source and target branches