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

Proposed by George Ormond Lorch III
Status: Superseded
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) Approve
Review via email: mp+170165@code.launchpad.net

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

This proposal has been superseded by a proposal from 2013-06-25.

Description of the change

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.
Revision history for this message
George Ormond Lorch III (gl-az) wrote : Posted in a previous version of this proposal
Revision history for this message
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
Revision history for this message
Alexey Kopytov (akopytov) wrote :

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

review: Approve
Revision history for this message
Laurynas Biveinis (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.

Revision history for this message
George Ormond Lorch III (gl-az) wrote :

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.

Revision history for this message
George Ormond Lorch III (gl-az) wrote :

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

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

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== added file 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.result'
--- Percona-Server/mysql-test/suite/federated/federated_bug_68354.result 1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/suite/federated/federated_bug_68354.result 2013-06-25 17:20:39 +0000
@@ -0,0 +1,80 @@
1CREATE DATABASE federated;
2CREATE DATABASE federated;
3SET @OLD_MASTER_CONCURRENT_INSERT= @@GLOBAL.CONCURRENT_INSERT;
4SET @@GLOBAL.CONCURRENT_INSERT= 0;
5SET @OLD_SLAVE_CONCURRENT_INSERT= @@GLOBAL.CONCURRENT_INSERT;
6SET @@GLOBAL.CONCURRENT_INSERT= 0;
7DROP TABLE IF EXISTS federated.t1;
8Warnings:
9Note 1051 Unknown table 't1'
10CREATE TABLE federated.t1 (
11`id` int(20) NOT NULL,
12`dummy` int(20) DEFAULT NULL,
13PRIMARY KEY (`id`))
14ENGINE="MyISAM"
15 DEFAULT CHARSET=latin1;
16INSERT INTO federated.t1 (`id`) VALUES
17(1001), (1002), (1003), (1004), (1005),
18(1006), (1007), (1008), (1009), (1010);
19CREATE TABLE federated.t1 (
20`id` int(20) NOT NULL,
21`dummy` int(20) DEFAULT NULL,
22PRIMARY KEY (`id`))
23ENGINE="FEDERATED"
24 DEFAULT CHARSET=latin1
25CONNECTION='mysql://root@127.0.0.1:SLAVE_PORT/federated/t1';
26CREATE TABLE federated.t2 (
27`id` int(20) NOT NULL,
28`dummy` int(20) NOT NULL,
29PRIMARY KEY (`id`))
30ENGINE="MyISAM"
31 DEFAULT CHARSET=latin1;
32INSERT INTO federated.t2 (`id`, `dummy`) VALUES (1005, 5);
33SELECT * FROM federated.t1;
34id dummy
351001 NULL
361002 NULL
371003 NULL
381004 NULL
391005 NULL
401006 NULL
411007 NULL
421008 NULL
431009 NULL
441010 NULL
45UPDATE federated.t1 ft1 INNER JOIN federated.t2 ft2 ON ft1.id = ft2.id SET ft1.dummy = ft2.dummy WHERE ft1.dummy IS NULL;
46SELECT * FROM federated.t1;
47id dummy
481001 NULL
491002 NULL
501003 NULL
511004 NULL
521005 5
531006 NULL
541007 NULL
551008 NULL
561009 NULL
571010 NULL
58DELETE FROM federated.t1 WHERE id = 1005;
59SELECT * FROM federated.t1 WHERE id = 1005;
60id dummy
61SELECT * FROM federated.t1;
62id dummy
631001 NULL
641002 NULL
651003 NULL
661004 NULL
671006 NULL
681007 NULL
691008 NULL
701009 NULL
711010 NULL
72DROP TABLE federated.t2;
73DROP TABLE federated.t1;
74DROP TABLE federated.t1;
75SET @@GLOBAL.CONCURRENT_INSERT= @OLD_MASTER_CONCURRENT_INSERT;
76SET @@GLOBAL.CONCURRENT_INSERT= @OLD_SLAVE_CONCURRENT_INSERT;
77DROP TABLE IF EXISTS federated.t1;
78DROP DATABASE federated;
79DROP TABLE IF EXISTS federated.t1;
80DROP DATABASE federated;
081
=== renamed file 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.result' => 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.result.moved'
=== added file 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.test'
--- Percona-Server/mysql-test/suite/federated/federated_bug_68354.test 1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/suite/federated/federated_bug_68354.test 2013-06-25 17:20:39 +0000
@@ -0,0 +1,79 @@
1# Bug 1182572 68354: Server crashes on update/join FEDERATED + local table when
2# only 1 local row.
3#
4# When updating a federated table with UPDATE ... JOIN, the server consistently
5# crashes with Signal 11 when only 1 row exists in the local table involved in
6# the join and that 1 row can be joined with a row in the federated table.
7#
8
9--source suite/federated/include/federated.inc
10
11connection default;
12
13# Disable concurrent inserts to avoid test failures when reading
14# data from concurrent connections (insert might return before
15# the data is actually in the table).
16SET @OLD_MASTER_CONCURRENT_INSERT= @@GLOBAL.CONCURRENT_INSERT;
17SET @@GLOBAL.CONCURRENT_INSERT= 0;
18
19connection slave;
20SET @OLD_SLAVE_CONCURRENT_INSERT= @@GLOBAL.CONCURRENT_INSERT;
21SET @@GLOBAL.CONCURRENT_INSERT= 0;
22DROP TABLE IF EXISTS federated.t1;
23CREATE TABLE federated.t1 (
24 `id` int(20) NOT NULL,
25 `dummy` int(20) DEFAULT NULL,
26 PRIMARY KEY (`id`))
27 ENGINE="MyISAM"
28 DEFAULT CHARSET=latin1;
29
30INSERT INTO federated.t1 (`id`) VALUES
31 (1001), (1002), (1003), (1004), (1005),
32 (1006), (1007), (1008), (1009), (1010);
33
34connection master;
35--replace_result $SLAVE_MYPORT SLAVE_PORT
36eval CREATE TABLE federated.t1 (
37 `id` int(20) NOT NULL,
38 `dummy` int(20) DEFAULT NULL,
39 PRIMARY KEY (`id`))
40 ENGINE="FEDERATED"
41 DEFAULT CHARSET=latin1
42 CONNECTION='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1';
43
44CREATE TABLE federated.t2 (
45 `id` int(20) NOT NULL,
46 `dummy` int(20) NOT NULL,
47 PRIMARY KEY (`id`))
48 ENGINE="MyISAM"
49 DEFAULT CHARSET=latin1;
50
51INSERT INTO federated.t2 (`id`, `dummy`) VALUES (1005, 5);
52
53SELECT * FROM federated.t1;
54
55# master would crash here before bug fix
56UPDATE federated.t1 ft1 INNER JOIN federated.t2 ft2 ON ft1.id = ft2.id SET ft1.dummy = ft2.dummy WHERE ft1.dummy IS NULL;
57
58# sanity test to make sure correct rows are being returned and server doesn't
59# also crash during DELETE
60SELECT * FROM federated.t1;
61DELETE FROM federated.t1 WHERE id = 1005;
62SELECT * FROM federated.t1 WHERE id = 1005;
63
64SELECT * FROM federated.t1;
65
66DROP TABLE federated.t2;
67DROP TABLE federated.t1;
68
69connection slave;
70DROP TABLE federated.t1;
71
72connection default;
73
74SET @@GLOBAL.CONCURRENT_INSERT= @OLD_MASTER_CONCURRENT_INSERT;
75connection slave;
76SET @@GLOBAL.CONCURRENT_INSERT= @OLD_SLAVE_CONCURRENT_INSERT;
77
78connection default;
79--source suite/federated/include/federated_cleanup.inc
080
=== renamed file 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.test' => 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.test.moved'
=== modified file 'Percona-Server/storage/federated/ha_federated.cc'
--- Percona-Server/storage/federated/ha_federated.cc 2013-06-18 18:39:22 +0000
+++ Percona-Server/storage/federated/ha_federated.cc 2013-06-25 17:20:39 +0000
@@ -2329,6 +2329,7 @@
23292329
2330 DBUG_ENTER("ha_federated::index_read");2330 DBUG_ENTER("ha_federated::index_read");
23312331
2332<<<<<<< TREE
2332 if ((retval= index_read_idx_with_result_set(buf, active_index, key,2333 if ((retval= index_read_idx_with_result_set(buf, active_index, key,
2333 key_len, find_flag,2334 key_len, find_flag,
2334 &mysql_result)))2335 &mysql_result)))
@@ -2336,6 +2337,15 @@
23362337
2337 set_last_result(mysql_result);2338 set_last_result(mysql_result);
2338 DBUG_RETURN(0);2339 DBUG_RETURN(0);
2340=======
2341 if ((retval= index_read_idx_with_result_set(buf, active_index, key,
2342 key_len, find_flag,
2343 &mysql_result)))
2344 DBUG_RETURN(retval);
2345
2346 set_last_result(mysql_result);
2347 DBUG_RETURN(0);
2348>>>>>>> MERGE-SOURCE
2339}2349}
23402350
23412351
@@ -2505,10 +2515,17 @@
2505 retval= HA_ERR_END_OF_FILE;2515 retval= HA_ERR_END_OF_FILE;
2506 goto error;2516 goto error;
2507 }2517 }
2518<<<<<<< TREE
25082519
2509 set_last_result(mysql_result);2520 set_last_result(mysql_result);
2510 2521
2511 DBUG_RETURN(read_next(table->record[0], mysql_result));2522 DBUG_RETURN(read_next(table->record[0], mysql_result));
2523=======
2524
2525 set_last_result(mysql_result);
2526
2527 DBUG_RETURN(read_next(table->record[0], mysql_result));
2528>>>>>>> MERGE-SOURCE
25122529
2513error:2530error:
2514 table->status= STATUS_NOT_FOUND;2531 table->status= STATUS_NOT_FOUND;
@@ -2714,8 +2731,12 @@
2714 /* Store data cursor position. */2731 /* Store data cursor position. */
2715 memcpy_fixed(ref + sizeof(MYSQL_RES *), &current_position,2732 memcpy_fixed(ref + sizeof(MYSQL_RES *), &current_position,
2716 sizeof(MYSQL_ROW_OFFSET));2733 sizeof(MYSQL_ROW_OFFSET));
2734<<<<<<< TREE
27172735
2718 position_called= true;2736 position_called= true;
2737=======
2738 position_called= true;
2739>>>>>>> MERGE-SOURCE
2719 DBUG_VOID_RETURN;2740 DBUG_VOID_RETURN;
2720}2741}
27212742

Subscribers

People subscribed via source and target branches