Merge lp:~gl-az/percona-server/ST-31919-5.1 into lp:percona-server/5.1

Proposed by George Ormond Lorch III on 2013-06-25
Status: Merged
Approved by: Alexey Kopytov on 2013-06-26
Approved revision: 549
Merged at revision: 577
Proposed branch: lp:~gl-az/percona-server/ST-31919-5.1
Merge into: lp:percona-server/5.1
Diff against target: 228 lines (+180/-0) (has conflicts)
3 files modified
Percona-Server/mysql-test/suite/federated/federated_bug_68354.result (+80/-0)
Percona-Server/mysql-test/suite/federated/federated_bug_68354.test (+79/-0)
Percona-Server/storage/federated/ha_federated.cc (+21/-0)
Conflict adding file Percona-Server/mysql-test/suite/federated/federated_bug_68354.result.  Moved existing file to Percona-Server/mysql-test/suite/federated/federated_bug_68354.result.moved.
Conflict adding file Percona-Server/mysql-test/suite/federated/federated_bug_68354.test.  Moved existing file to Percona-Server/mysql-test/suite/federated/federated_bug_68354.test.moved.
Text conflict in Percona-Server/storage/federated/ha_federated.cc
To merge this branch: bzr merge lp:~gl-az/percona-server/ST-31919-5.1
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) 2013-06-25 Approve on 2013-06-26
Review via email: mp+171371@code.launchpad.net

This proposal supersedes a proposal from 2013-06-18.

Description of the change

Resubmitting with correct GCA procedure followed whcih will cause issues now when merging to trunk but _should_ leave the trunk in a clean state.

Jenkins --suite=federated: http://jenkins.percona.com/view/PS%205.1/job/percona-server-5.1-param/559/

Fix for upstream 68354 and lp 1182572:
  Server crashes on update/join FEDERATED + local table when only 1 local row

This issue seems to be the result of a poor or changed assumption in the order
in which handler calls are made. In this specific scenario (a join with a local
table that has exactly one matching record) there is an assumption in the code
that a call to position will always come after a 'rnd_init/index_init' and
'rnd_next/index_read' but before 'rnd_end/index_end' is called.

The failing call order is:
  ha_federated::index_init
  ha_federated::index_read
  ha_federated::index_end
  ha_federated::position
  ha_federated::rnd_init
  ha_federated::extra
  ha_federated::rnd_pos

So here you can see position is getting called _outside_ of the scope of an
_init.._end table and/or index scan. This does indicate a change/problem in
the server but that is outside the scope of this fix.

As results are fetched, they are added to an interal list of result sets called
'results'. The last result set/current position is also held in the
'stored_result' value. If 'index_end' or 'rnd_end' is called before 'position'
has been called, the 'stored_result' is zeroed out and that result is removed
from the 'stored_results' list. When 'position' is called, a NULL result is
returned, which is then passed back through 'rnd_pos', which then dereferences
the NULL value and tries to manipulate some of the result internals, crashing.

So seems that the intention of the whole 'stored_result' was to have a place
to track all results allocated for the purposes of keeping them valid for the
'position' and 'rnd_pos' calls.

My approch to fixing this is to restructure this somewhan and instead of
tracking all results allocated in the 'results' list and popping them out when
they are discovered to not be needed, I went the other way. The results only
get addes to a storage list when it has been proven that they are in fact
needed later. So now for each new result set, it is temporarily stored in a
new value called 'last_result'. Whenever a new last_result needs created, it is
set by calling a new function 'set_last_result'. This function will check to
see if 'position' has been called on that result and if so, add it to the
(renamed) list 'stored_results' and clears the 'position_called' flag, else it
will free the 'last_result'. It then sets the new 'last_result' value.

Because of the out of scan scope call of position, we can no longer clear the
'stored_results' lists on '_end'. It will be cleared now when 'reset' is
called.

I have created a test case (suite/federated/federated_bug_68354.test) and
verified that it fails without the fix. Everything tests out under the federated
suite and the issue appears fixed.

The only downside is that a lengthy query could use a lot of memory while
hanging onto old 'position'ed results but I can't see how that can be safely
avoided in this particular handler.

To post a comment you must log in.
Alexey Kopytov (akopytov) wrote : Posted in a previous version of this proposal

George,

I agree with the general idea of the fix. I would try to keep the
patch minimal and avoid refactoring to simplify maintenance, but
that's more a personal preference. The only significant comment that
I have is that we seem to introduce a memory leak:

- before the patch, 'stored_result' is guaranteed to also be in the
  'results' array. Which ensures that ha_federated::reset() calls
  mysql_free_result() for all stored results. Now last_result is
  guaranteed to NOT be in the 'stored_results' array. Don't we need
  an explicit mysql_free_result() for it on reset?

Minor stuff:

- the test case needs a header referencing bug number(s)
- no need to include include/not_embedded.inc in the test case, as
  it is already included from suite/federated/include/federated.inc
- unnecessary whitespace changes on lines 411 (the server code
  formatting rules require 2 blank lines between functions), 439 and 447
- wrong comment formatting in ha_federated.h (again, server code
  formatting is different from InnoDB)

review: Needs Fixing
Alexey Kopytov (akopytov) wrote : Posted in a previous version of this proposal

Approved, though the changes to ha_federated::rnd_end() and the blank line change on line 420 are not needed.

review: Approve
Laurynas Biveinis (laurynas-biveinis) wrote : Posted in a previous version of this proposal

George -

Feel free to send this diff (and its 5.5/5.6 counterparts) to the upstream bug (use Contributions tab) now that we have OCA signed.

George Ormond Lorch III (gl-az) wrote : Posted in a previous version of this proposal

Fixed rnd_end and odd blank line (wasn't a blank line, but a line with a single space in the original and I had removed the space) and repushed to same brranch.

George Ormond Lorch III (gl-az) wrote : Posted in a previous version of this proposal

On 6/19/2013 4:21 AM, Laurynas Biveinis wrote:
> George -
>
> Feel free to send this diff (and its 5.5/5.6 counterparts) to the upstream bug (use Contributions tab) now that we have OCA signed.
Good idea...done.

--
George O. Lorch III
Software Engineer, Percona
+1-888-401-3401 x542 US/Arizona (GMT -7)
skype: george.ormond.lorch.iii

Laurynas Biveinis (laurynas-biveinis) wrote : Posted in a previous version of this proposal

Since I merged a previous version of this MP (yay!), will create a new MP to revert

@@ -2591,14 +2598,14 @@
 int ha_federated::rnd_end()
 {
   DBUG_ENTER("ha_federated::rnd_end");
- DBUG_RETURN(index_end());
+ active_index= MAX_KEY;
+ DBUG_RETURN(0);
 }

and

-
+
 int ha_federated::external_lock(THD *thd, int lock_type)

Alexey Kopytov (akopytov) wrote :

I think it would be much easier to just resubmit those 2 minor cleanups as separate MPs against current trunk and be done with it. But OK, hope this will be merged correctly.

review: Approve

We are also fixing not only my premature trunk merge, but also that the feature branches were not properly upmerged (i.e. 5.6 file-ids differ from 5.5 ones).

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.result'
2--- Percona-Server/mysql-test/suite/federated/federated_bug_68354.result 1970-01-01 00:00:00 +0000
3+++ Percona-Server/mysql-test/suite/federated/federated_bug_68354.result 2013-06-25 17:38:27 +0000
4@@ -0,0 +1,80 @@
5+CREATE DATABASE federated;
6+CREATE DATABASE federated;
7+SET @OLD_MASTER_CONCURRENT_INSERT= @@GLOBAL.CONCURRENT_INSERT;
8+SET @@GLOBAL.CONCURRENT_INSERT= 0;
9+SET @OLD_SLAVE_CONCURRENT_INSERT= @@GLOBAL.CONCURRENT_INSERT;
10+SET @@GLOBAL.CONCURRENT_INSERT= 0;
11+DROP TABLE IF EXISTS federated.t1;
12+Warnings:
13+Note 1051 Unknown table 't1'
14+CREATE TABLE federated.t1 (
15+`id` int(20) NOT NULL,
16+`dummy` int(20) DEFAULT NULL,
17+PRIMARY KEY (`id`))
18+ENGINE="MyISAM"
19+ DEFAULT CHARSET=latin1;
20+INSERT INTO federated.t1 (`id`) VALUES
21+(1001), (1002), (1003), (1004), (1005),
22+(1006), (1007), (1008), (1009), (1010);
23+CREATE TABLE federated.t1 (
24+`id` int(20) NOT NULL,
25+`dummy` int(20) DEFAULT NULL,
26+PRIMARY KEY (`id`))
27+ENGINE="FEDERATED"
28+ DEFAULT CHARSET=latin1
29+CONNECTION='mysql://root@127.0.0.1:SLAVE_PORT/federated/t1';
30+CREATE TABLE federated.t2 (
31+`id` int(20) NOT NULL,
32+`dummy` int(20) NOT NULL,
33+PRIMARY KEY (`id`))
34+ENGINE="MyISAM"
35+ DEFAULT CHARSET=latin1;
36+INSERT INTO federated.t2 (`id`, `dummy`) VALUES (1005, 5);
37+SELECT * FROM federated.t1;
38+id dummy
39+1001 NULL
40+1002 NULL
41+1003 NULL
42+1004 NULL
43+1005 NULL
44+1006 NULL
45+1007 NULL
46+1008 NULL
47+1009 NULL
48+1010 NULL
49+UPDATE federated.t1 ft1 INNER JOIN federated.t2 ft2 ON ft1.id = ft2.id SET ft1.dummy = ft2.dummy WHERE ft1.dummy IS NULL;
50+SELECT * FROM federated.t1;
51+id dummy
52+1001 NULL
53+1002 NULL
54+1003 NULL
55+1004 NULL
56+1005 5
57+1006 NULL
58+1007 NULL
59+1008 NULL
60+1009 NULL
61+1010 NULL
62+DELETE FROM federated.t1 WHERE id = 1005;
63+SELECT * FROM federated.t1 WHERE id = 1005;
64+id dummy
65+SELECT * FROM federated.t1;
66+id dummy
67+1001 NULL
68+1002 NULL
69+1003 NULL
70+1004 NULL
71+1006 NULL
72+1007 NULL
73+1008 NULL
74+1009 NULL
75+1010 NULL
76+DROP TABLE federated.t2;
77+DROP TABLE federated.t1;
78+DROP TABLE federated.t1;
79+SET @@GLOBAL.CONCURRENT_INSERT= @OLD_MASTER_CONCURRENT_INSERT;
80+SET @@GLOBAL.CONCURRENT_INSERT= @OLD_SLAVE_CONCURRENT_INSERT;
81+DROP TABLE IF EXISTS federated.t1;
82+DROP DATABASE federated;
83+DROP TABLE IF EXISTS federated.t1;
84+DROP DATABASE federated;
85
86=== renamed file 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.result' => 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.result.moved'
87=== added file 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.test'
88--- Percona-Server/mysql-test/suite/federated/federated_bug_68354.test 1970-01-01 00:00:00 +0000
89+++ Percona-Server/mysql-test/suite/federated/federated_bug_68354.test 2013-06-25 17:38:27 +0000
90@@ -0,0 +1,79 @@
91+# Bug 1182572 68354: Server crashes on update/join FEDERATED + local table when
92+# only 1 local row.
93+#
94+# When updating a federated table with UPDATE ... JOIN, the server consistently
95+# crashes with Signal 11 when only 1 row exists in the local table involved in
96+# the join and that 1 row can be joined with a row in the federated table.
97+#
98+
99+--source suite/federated/include/federated.inc
100+
101+connection default;
102+
103+# Disable concurrent inserts to avoid test failures when reading
104+# data from concurrent connections (insert might return before
105+# the data is actually in the table).
106+SET @OLD_MASTER_CONCURRENT_INSERT= @@GLOBAL.CONCURRENT_INSERT;
107+SET @@GLOBAL.CONCURRENT_INSERT= 0;
108+
109+connection slave;
110+SET @OLD_SLAVE_CONCURRENT_INSERT= @@GLOBAL.CONCURRENT_INSERT;
111+SET @@GLOBAL.CONCURRENT_INSERT= 0;
112+DROP TABLE IF EXISTS federated.t1;
113+CREATE TABLE federated.t1 (
114+ `id` int(20) NOT NULL,
115+ `dummy` int(20) DEFAULT NULL,
116+ PRIMARY KEY (`id`))
117+ ENGINE="MyISAM"
118+ DEFAULT CHARSET=latin1;
119+
120+INSERT INTO federated.t1 (`id`) VALUES
121+ (1001), (1002), (1003), (1004), (1005),
122+ (1006), (1007), (1008), (1009), (1010);
123+
124+connection master;
125+--replace_result $SLAVE_MYPORT SLAVE_PORT
126+eval CREATE TABLE federated.t1 (
127+ `id` int(20) NOT NULL,
128+ `dummy` int(20) DEFAULT NULL,
129+ PRIMARY KEY (`id`))
130+ ENGINE="FEDERATED"
131+ DEFAULT CHARSET=latin1
132+ CONNECTION='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1';
133+
134+CREATE TABLE federated.t2 (
135+ `id` int(20) NOT NULL,
136+ `dummy` int(20) NOT NULL,
137+ PRIMARY KEY (`id`))
138+ ENGINE="MyISAM"
139+ DEFAULT CHARSET=latin1;
140+
141+INSERT INTO federated.t2 (`id`, `dummy`) VALUES (1005, 5);
142+
143+SELECT * FROM federated.t1;
144+
145+# master would crash here before bug fix
146+UPDATE federated.t1 ft1 INNER JOIN federated.t2 ft2 ON ft1.id = ft2.id SET ft1.dummy = ft2.dummy WHERE ft1.dummy IS NULL;
147+
148+# sanity test to make sure correct rows are being returned and server doesn't
149+# also crash during DELETE
150+SELECT * FROM federated.t1;
151+DELETE FROM federated.t1 WHERE id = 1005;
152+SELECT * FROM federated.t1 WHERE id = 1005;
153+
154+SELECT * FROM federated.t1;
155+
156+DROP TABLE federated.t2;
157+DROP TABLE federated.t1;
158+
159+connection slave;
160+DROP TABLE federated.t1;
161+
162+connection default;
163+
164+SET @@GLOBAL.CONCURRENT_INSERT= @OLD_MASTER_CONCURRENT_INSERT;
165+connection slave;
166+SET @@GLOBAL.CONCURRENT_INSERT= @OLD_SLAVE_CONCURRENT_INSERT;
167+
168+connection default;
169+--source suite/federated/include/federated_cleanup.inc
170
171=== renamed file 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.test' => 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.test.moved'
172=== modified file 'Percona-Server/storage/federated/ha_federated.cc'
173--- Percona-Server/storage/federated/ha_federated.cc 2013-06-18 18:39:22 +0000
174+++ Percona-Server/storage/federated/ha_federated.cc 2013-06-25 17:38:27 +0000
175@@ -2329,6 +2329,7 @@
176
177 DBUG_ENTER("ha_federated::index_read");
178
179+<<<<<<< TREE
180 if ((retval= index_read_idx_with_result_set(buf, active_index, key,
181 key_len, find_flag,
182 &mysql_result)))
183@@ -2336,6 +2337,15 @@
184
185 set_last_result(mysql_result);
186 DBUG_RETURN(0);
187+=======
188+ if ((retval= index_read_idx_with_result_set(buf, active_index, key,
189+ key_len, find_flag,
190+ &mysql_result)))
191+ DBUG_RETURN(retval);
192+
193+ set_last_result(mysql_result);
194+ DBUG_RETURN(0);
195+>>>>>>> MERGE-SOURCE
196 }
197
198
199@@ -2505,10 +2515,17 @@
200 retval= HA_ERR_END_OF_FILE;
201 goto error;
202 }
203+<<<<<<< TREE
204
205 set_last_result(mysql_result);
206
207 DBUG_RETURN(read_next(table->record[0], mysql_result));
208+=======
209+
210+ set_last_result(mysql_result);
211+
212+ DBUG_RETURN(read_next(table->record[0], mysql_result));
213+>>>>>>> MERGE-SOURCE
214
215 error:
216 table->status= STATUS_NOT_FOUND;
217@@ -2714,8 +2731,12 @@
218 /* Store data cursor position. */
219 memcpy_fixed(ref + sizeof(MYSQL_RES *), &current_position,
220 sizeof(MYSQL_ROW_OFFSET));
221+<<<<<<< TREE
222
223 position_called= true;
224+=======
225+ position_called= true;
226+>>>>>>> MERGE-SOURCE
227 DBUG_VOID_RETURN;
228 }
229

Subscribers

People subscribed via source and target branches