Merge lp:~gl-az/percona-server/bug758788-5.5 into lp:percona-server/5.5

Proposed by George Ormond Lorch III on 2014-02-07
Status: Merged
Approved by: Laurynas Biveinis on 2014-02-12
Approved revision: 608
Merged at revision: 624
Proposed branch: lp:~gl-az/percona-server/bug758788-5.5
Merge into: lp:percona-server/5.5
Diff against target: 288 lines (+156/-32)
8 files modified
mysql-test/suite/innodb/r/percona_bug758788.result (+9/-0)
mysql-test/suite/innodb/t/percona_bug758788-master.opt (+1/-0)
mysql-test/suite/innodb/t/percona_bug758788.test (+24/-0)
storage/innobase/dict/dict0dict.c (+95/-32)
storage/innobase/dict/dict0mem.c (+1/-0)
storage/innobase/include/lock0lock.h (+10/-0)
storage/innobase/include/lock0lock.ic (+14/-0)
storage/innobase/lock/lock0lock.c (+2/-0)
To merge this branch: bzr merge lp:~gl-az/percona-server/bug758788-5.5
Reviewer Review Type Date Requested Status
Laurynas Biveinis Approve on 2014-02-12
Vlad Lesin (community) g2 2014-02-07 Needs Fixing on 2014-02-10
Review via email: mp+205279@code.launchpad.net

Description of the change

Fix for bug 758788 - mysql process crashes after setting innodb_dict_size
Fix for bug 1250018 - innodb_dict_size_limit tries to do LRU eviction of an index that is in use

dict_table_LRU_trim was missing some critical checks for existing locks on tables and indexes. Cleaned this up a bit and changed the algo to be not so aggressive when trying to find and evict tables from the dict cache.

With some of the new functionality, added some more debug assertions that should trip if the table is being evicted when it shouldn't as well as watching for an unexpectedly deleted table in some places.

Created very simple debug test case for 758788.

This very likely doesn't address every possible edge crash as the InnoDB dict system was simply never designed to be a dynamic cache but a static set of instances.

jenkins http://jenkins.percona.com/view/PS%205.5/job/percona-server-5.5-param/943/

To post a comment you must log in.
Vlad Lesin (vlad-lesin) wrote :

Looks good for me, but the test case is not obvious. You did good explanation in bug #758788 comments about this test case, it would be more clear to have some resume in test comments.

review: Needs Fixing (g2)
review: Needs Fixing
George Ormond Lorch III (gl-az) wrote :

Done...

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'mysql-test/suite/innodb/r/percona_bug758788.result'
2--- mysql-test/suite/innodb/r/percona_bug758788.result 1970-01-01 00:00:00 +0000
3+++ mysql-test/suite/innodb/r/percona_bug758788.result 2014-02-12 18:07:20 +0000
4@@ -0,0 +1,9 @@
5+SET SESSION AUTOCOMMIT=0;
6+CREATE TABLE t1 (c1 INT) ENGINE=INNODB;
7+CREATE TABLE t2 (c1 INT) ENGINE=INNODB;
8+INSERT INTO t1(c1) VALUES(1);
9+FLUSH TABLES;
10+INSERT INTO t2(c1) VALUES(1);
11+COMMIT;
12+DROP TABLE t2;
13+DROP TABLE t1;
14
15=== added file 'mysql-test/suite/innodb/t/percona_bug758788-master.opt'
16--- mysql-test/suite/innodb/t/percona_bug758788-master.opt 1970-01-01 00:00:00 +0000
17+++ mysql-test/suite/innodb/t/percona_bug758788-master.opt 2014-02-12 18:07:20 +0000
18@@ -0,0 +1,1 @@
19+--innodb_dict_size_limit=1
20
21=== added file 'mysql-test/suite/innodb/t/percona_bug758788.test'
22--- mysql-test/suite/innodb/t/percona_bug758788.test 1970-01-01 00:00:00 +0000
23+++ mysql-test/suite/innodb/t/percona_bug758788.test 2014-02-12 18:07:20 +0000
24@@ -0,0 +1,24 @@
25+--source include/have_innodb.inc
26+
27+#
28+# Bug #758788: mysql process crashes after setting innodb_dict_size
29+#
30+# With innodb_dict_size=1 in the .opt file, the 'FLUSH TABLES' command below
31+# above allows the eviction of the dict_table for t1 in the dict cache even
32+# though there is a lock held against it due to the INSERT and referenced
33+# from the transaction lock list. When the COMMIT comes along and tries to
34+# clean up the transaction lock list, the reference to t1 that was locked
35+# during the first INSERT is now completely invalid and can cause various
36+# assertions since it is now pointing to potentially reclaimed memory.
37+#
38+SET SESSION AUTOCOMMIT=0;
39+
40+CREATE TABLE t1 (c1 INT) ENGINE=INNODB;
41+CREATE TABLE t2 (c1 INT) ENGINE=INNODB;
42+INSERT INTO t1(c1) VALUES(1);
43+FLUSH TABLES;
44+INSERT INTO t2(c1) VALUES(1);
45+COMMIT;
46+
47+DROP TABLE t2;
48+DROP TABLE t1;
49
50=== modified file 'storage/innobase/dict/dict0dict.c'
51--- storage/innobase/dict/dict0dict.c 2013-12-05 08:07:26 +0000
52+++ storage/innobase/dict/dict0dict.c 2014-02-12 18:07:20 +0000
53@@ -65,6 +65,7 @@
54 #include "srv0start.h" /* SRV_LOG_SPACE_FIRST_ID */
55 #include "m_string.h"
56 #include "my_sys.h"
57+#include "lock0lock.h"
58
59 #include <ctype.h>
60
61@@ -1331,6 +1332,7 @@
62 ut_ad(table);
63 ut_ad(mutex_own(&(dict_sys->mutex)));
64 ut_ad(table->magic_n == DICT_TABLE_MAGIC_N);
65+ ut_ad(lock_table_has_locks(table) == FALSE);
66
67 #if 0
68 fputs("Removing table ", stderr);
69@@ -1383,6 +1385,53 @@
70 dict_mem_table_free(table);
71 }
72
73+/**********************************************************************//**
74+Test whether a table can be evicted from the LRU cache.
75+@return TRUE if table can be evicted. */
76+static
77+ibool
78+dict_table_can_be_evicted(
79+/*======================*/
80+ const dict_table_t* table) /*!< in: table to test */
81+{
82+ dict_index_t* index;
83+ dict_foreign_t* foreign;
84+ ibool has_locks;
85+
86+ ut_ad(mutex_own(&dict_sys->mutex));
87+
88+ /* bug 758788: A table may not have an active handle opened on it
89+ but may still have locks held on it across multiple statements
90+ within an individual transaction. So a table is not evictable if
91+ there are locks held on it */
92+ has_locks = lock_table_has_locks(table);
93+
94+ if (table->n_mysql_handles_opened || table->is_corrupt || has_locks)
95+ return(FALSE);
96+
97+ /* bug 758788: We are not allowed to free the in-memory index struct
98+ dict_index_t if there are any locks held on any of its indexes. */
99+ for (index = dict_table_get_first_index(table); index != NULL;
100+ index = dict_table_get_next_index(index)) {
101+
102+ rw_lock_t* lock = dict_index_get_lock(index);
103+
104+ if (rw_lock_is_locked(lock, RW_LOCK_SHARED)
105+ || rw_lock_is_locked(lock, RW_LOCK_EX)) {
106+ return(FALSE);
107+ }
108+ }
109+
110+ for (foreign = UT_LIST_GET_FIRST(table->foreign_list); foreign != NULL;
111+ foreign = UT_LIST_GET_NEXT(foreign_list, foreign)) {
112+
113+ if (foreign->referenced_table) {
114+ return(FALSE);
115+ }
116+ }
117+ return(TRUE);
118+}
119+
120 /**************************************************************************
121 Frees tables from the end of table_LRU if the dictionary cache occupies
122 too much space. */
123@@ -1394,51 +1443,65 @@
124 {
125 dict_table_t* table;
126 dict_table_t* prev_table;
127- dict_foreign_t* foreign;
128- ulint n_removed;
129- ulint n_have_parent;
130- ulint cached_foreign_tables;
131+ ulint dict_size;
132+ ulint max_depth;
133+ ulint max_evict;
134+ ulint visited;
135+ ulint evicted;
136
137 #ifdef UNIV_SYNC_DEBUG
138 ut_ad(mutex_own(&(dict_sys->mutex)));
139 #endif /* UNIV_SYNC_DEBUG */
140
141-retry:
142- n_removed = n_have_parent = 0;
143+ if (srv_dict_size_limit == 0)
144+ return;
145+
146+ /* Calculate this once here and then once on every eviction */
147+ dict_size = (dict_sys->table_hash->n_cells
148+ + dict_sys->table_id_hash->n_cells)
149+ * sizeof(hash_cell_t) + dict_sys->size;
150+
151+ /* Just some magic numbers to help keep us from holding the dict mutex
152+ for too long as well as preventing constant full scans of the list in
153+ a memory pressure situation. */
154+
155+ /* Don't go scanning into the front 50% of the list, chances are very
156+ good that there is nothing to be evicted in there and if there is,
157+ it should quickly get pushed into the back 50% */
158+ max_depth = UT_LIST_GET_LEN(dict_sys->table_LRU) / 2;
159+
160+ /* Don't try to evict any more than 10% of evictable tables at once. */
161+ max_evict = UT_LIST_GET_LEN(dict_sys->table_LRU) / 10;
162+
163+ visited = evicted = 0;
164+
165 table = UT_LIST_GET_LAST(dict_sys->table_LRU);
166
167- while ( srv_dict_size_limit && table
168- && ((dict_sys->table_hash->n_cells
169- + dict_sys->table_id_hash->n_cells) * sizeof(hash_cell_t)
170- + dict_sys->size) > srv_dict_size_limit ) {
171+ while (table && dict_size > srv_dict_size_limit
172+ && visited <= max_depth
173+ && srv_shutdown_state == SRV_SHUTDOWN_NONE) {
174+
175 prev_table = UT_LIST_GET_PREV(table_LRU, table);
176
177- if (table == self || table->n_mysql_handles_opened || table->is_corrupt)
178- goto next_loop;
179-
180- cached_foreign_tables = 0;
181- foreign = UT_LIST_GET_FIRST(table->foreign_list);
182- while (foreign != NULL) {
183- if (foreign->referenced_table)
184- cached_foreign_tables++;
185- foreign = UT_LIST_GET_NEXT(foreign_list, foreign);
186- }
187-
188- if (cached_foreign_tables == 0) {
189+ if (table != self && dict_table_can_be_evicted(table)) {
190 dict_table_remove_from_cache(table);
191- n_removed++;
192- } else {
193- n_have_parent++;
194+
195+ evicted++;
196+
197+ if (evicted >= max_evict)
198+ break;
199+
200+ /* Only need to recalculate this when something
201+ has been evicted */
202+ dict_size = (dict_sys->table_hash->n_cells
203+ + dict_sys->table_id_hash->n_cells)
204+ * sizeof(hash_cell_t) + dict_sys->size;
205 }
206-next_loop:
207+
208+ visited++;
209+
210 table = prev_table;
211 }
212-
213- if ( srv_dict_size_limit && n_have_parent && n_removed
214- && ((dict_sys->table_hash->n_cells
215- + dict_sys->table_id_hash->n_cells) * sizeof(hash_cell_t)
216- + dict_sys->size) > srv_dict_size_limit )
217- goto retry;
218 }
219
220 /****************************************************************//**
221
222=== modified file 'storage/innobase/dict/dict0mem.c'
223--- storage/innobase/dict/dict0mem.c 2013-08-14 03:52:04 +0000
224+++ storage/innobase/dict/dict0mem.c 2014-02-12 18:07:20 +0000
225@@ -115,6 +115,7 @@
226 ut_ad(table);
227 ut_ad(table->magic_n == DICT_TABLE_MAGIC_N);
228 ut_d(table->cached = FALSE);
229+ ut_d(table->magic_n = 0);
230
231 #ifndef UNIV_HOTBACKUP
232 mutex_free(&(table->autoinc_mutex));
233
234=== modified file 'storage/innobase/include/lock0lock.h'
235--- storage/innobase/include/lock0lock.h 2013-08-14 03:52:04 +0000
236+++ storage/innobase/include/lock0lock.h 2014-02-12 18:07:20 +0000
237@@ -459,6 +459,16 @@
238 by a read cursor */
239 const read_view_t* view); /*!< in: consistent read view */
240 /*********************************************************************//**
241+Check if there are any locks (table or rec) against table.
242+@return TRUE if locks exist */
243+UNIV_INLINE
244+ibool
245+lock_table_has_locks(
246+/*=================*/
247+ const dict_table_t* table); /*!< in: check if there are any locks
248+ held on records in this table or on the
249+ table itself */
250+/*********************************************************************//**
251 Locks the specified database table in the mode given. If the lock cannot
252 be granted immediately, the query thread is put to wait.
253 @return DB_SUCCESS, DB_LOCK_WAIT, DB_DEADLOCK, or DB_QUE_THR_SUSPENDED */
254
255=== modified file 'storage/innobase/include/lock0lock.ic'
256--- storage/innobase/include/lock0lock.ic 2013-06-10 20:29:41 +0000
257+++ storage/innobase/include/lock0lock.ic 2014-02-12 18:07:20 +0000
258@@ -119,3 +119,17 @@
259 FALSE)));
260 }
261 }
262+
263+/*******************************************************************//**
264+Check if there are any locks (table or rec) against table.
265+@return TRUE if table has either table or record locks. */
266+UNIV_INLINE
267+ibool
268+lock_table_has_locks(
269+/*=================*/
270+ const dict_table_t* table) /*!< in: check if there are any locks
271+ held on records in this table or on the
272+ table itself */
273+{
274+ return(UT_LIST_GET_LEN(table->locks) > 0);
275+}
276
277=== modified file 'storage/innobase/lock/lock0lock.c'
278--- storage/innobase/lock/lock0lock.c 2013-10-17 07:44:31 +0000
279+++ storage/innobase/lock/lock0lock.c 2014-02-12 18:07:20 +0000
280@@ -3832,6 +3832,8 @@
281 trx = lock->trx;
282 table = lock->un_member.tab_lock.table;
283
284+ ut_ad(table->magic_n == DICT_TABLE_MAGIC_N);
285+
286 /* Remove the table from the transaction's AUTOINC vector, if
287 the lock that is being release is an AUTOINC lock. */
288 if (lock_get_mode(lock) == LOCK_AUTO_INC) {

Subscribers

People subscribed via source and target branches