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

[George Ormond Lorch III]

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.1/job/percona-server-5.1-param/596/

[Vlad Lesin (vlad-lesin)]

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)

[Laurynas Biveinis (laurynas-biveinis)]

    This looks fine, just relatively minor comments:

    - Vlad's review comment seconded. The testcase can also drop
      "test." prefixes.

    - 99: s/for(/for (

    - 218-220: use ut_d(), just like the code above

review: Needs Fixing

[George Ormond Lorch III (gl-az)]

Done...

[Laurynas Biveinis (laurynas-biveinis)]

review: Approve

=== added file 'Percona-Server/mysql-test/suite/innodb_plugin/r/percona_bug758788.result'
--- Percona-Server/mysql-test/suite/innodb_plugin/r/percona_bug758788.result	1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/suite/innodb_plugin/r/percona_bug758788.result	2014-02-12 18:02:37 +0000
@@ -0,0 +1,9 @@
+SET SESSION AUTOCOMMIT=0;
+CREATE TABLE t1 (c1 INT) ENGINE=INNODB;
+CREATE TABLE t2 (c1 INT) ENGINE=INNODB;
+INSERT INTO t1(c1) VALUES(1);
+FLUSH TABLES;
+INSERT INTO t2(c1) VALUES(1);
+COMMIT;
+DROP TABLE t2;
+DROP TABLE t1;

=== added file 'Percona-Server/mysql-test/suite/innodb_plugin/t/percona_bug758788-master.opt'
--- Percona-Server/mysql-test/suite/innodb_plugin/t/percona_bug758788-master.opt	1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/suite/innodb_plugin/t/percona_bug758788-master.opt	2014-02-12 18:02:37 +0000
@@ -0,0 +1,1 @@
+--innodb_dict_size_limit=1

=== added file 'Percona-Server/mysql-test/suite/innodb_plugin/t/percona_bug758788.test'
--- Percona-Server/mysql-test/suite/innodb_plugin/t/percona_bug758788.test	1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/suite/innodb_plugin/t/percona_bug758788.test	2014-02-12 18:02:37 +0000
@@ -0,0 +1,24 @@
+--source include/have_innodb_plugin.inc
+
+#
+# Bug #758788: mysql process crashes after setting innodb_dict_size
+#
+# With innodb_dict_size=1 in the .opt file, the 'FLUSH TABLES' command below
+# above allows the eviction of the dict_table for t1 in the dict cache even
+# though there is a lock held against it due to the INSERT and referenced
+# from the transaction lock list. When the COMMIT comes along and tries to 
+# clean up the transaction lock list, the reference to t1 that was locked 
+# during the first INSERT is now completely invalid and can cause various
+# assertions since it is now pointing to potentially reclaimed memory.
+#
+SET SESSION AUTOCOMMIT=0;
+
+CREATE TABLE t1 (c1 INT) ENGINE=INNODB;
+CREATE TABLE t2 (c1 INT) ENGINE=INNODB;
+INSERT INTO t1(c1) VALUES(1);
+FLUSH TABLES;
+INSERT INTO t2(c1) VALUES(1);
+COMMIT;
+
+DROP TABLE t2;
+DROP TABLE t1;

=== modified file 'Percona-Server/storage/innodb_plugin/dict/dict0dict.c'
--- Percona-Server/storage/innodb_plugin/dict/dict0dict.c	2013-08-05 13:11:52 +0000
+++ Percona-Server/storage/innodb_plugin/dict/dict0dict.c	2014-02-12 18:02:37 +0000
@@ -57,6 +57,7 @@
 #include "m_ctype.h" /* my_isspace() */
 #include "ha_prototypes.h" /* innobase_strcasecmp() */
 #include "srv0start.h" /* SRV_LOG_SPACE_FIRST_ID */
+#include "lock0lock.h"
 
 #include <ctype.h>
 
@@ -1329,6 +1330,7 @@
 	ut_ad(table);
 	ut_ad(mutex_own(&(dict_sys->mutex)));
 	ut_ad(table->magic_n == DICT_TABLE_MAGIC_N);
+	ut_ad(lock_table_has_locks(table) == FALSE);
 
 #if 0
 	fputs("Removing table ", stderr);
@@ -1381,6 +1383,53 @@
 	dict_mem_table_free(table);
 }
 
+/**********************************************************************//**
+Test whether a table can be evicted from the LRU cache.
+@return TRUE if table can be evicted. */
+static
+ibool
+dict_table_can_be_evicted(
+/*======================*/
+	const dict_table_t*	table)	/*!< in: table to test */
+{
+	dict_index_t*	index;
+	dict_foreign_t*	foreign;
+	ibool		has_locks;
+
+	ut_ad(mutex_own(&dict_sys->mutex));
+
+	/* bug 758788: A table may not have an active handle opened on it
+	   but may still have locks held on it across multiple statements
+	   within an individual transaction. So a table is not evictable if
+	   there are locks held on it */
+	has_locks = lock_table_has_locks(table);
+
+	if (table->n_mysql_handles_opened || table->is_corrupt || has_locks)
+		return(FALSE);
+
+	/* bug 758788: We are not allowed to free the in-memory index struct
+	   dict_index_t if there are any locks held on any of its indexes. */
+	for (index = dict_table_get_first_index(table); index != NULL;
+	     index = dict_table_get_next_index(index)) {
+
+		rw_lock_t* lock = dict_index_get_lock(index);
+
+		if (rw_lock_is_locked(lock, RW_LOCK_SHARED)
+		    || rw_lock_is_locked(lock, RW_LOCK_EX)) {
+			return(FALSE);
+		} 
+	}
+
+	for (foreign = UT_LIST_GET_FIRST(table->foreign_list); foreign != NULL;
+	     foreign = UT_LIST_GET_NEXT(foreign_list, foreign)) {
+
+		if (foreign->referenced_table) {
+			return(FALSE);
+		}
+	}
+	return(TRUE);
+}
+
 /**************************************************************************
 Frees tables from the end of table_LRU if the dictionary cache occupies
 too much space. */
@@ -1392,51 +1441,65 @@
 {
 	dict_table_t*	table;
 	dict_table_t*	prev_table;
-	dict_foreign_t*	foreign;
-	ulint		n_removed;
-	ulint		n_have_parent;
-	ulint		cached_foreign_tables;
+	ulint		dict_size;
+	ulint		max_depth;
+	ulint		max_evict;
+	ulint		visited;
+	ulint		evicted;
 
 #ifdef UNIV_SYNC_DEBUG
 	ut_ad(mutex_own(&(dict_sys->mutex)));
 #endif /* UNIV_SYNC_DEBUG */
 
-retry:
-	n_removed = n_have_parent = 0;
+	if (srv_dict_size_limit == 0)
+		return;
+
+	/* Calculate this once here and then once on every eviction */
+	dict_size = (dict_sys->table_hash->n_cells
+		    + dict_sys->table_id_hash->n_cells)
+		      * sizeof(hash_cell_t) + dict_sys->size;
+
+	/* Just some magic numbers to help keep us from holding the dict mutex
+	   for too long as well as preventing constant full scans of the list in
+	   a memory pressure situation. */
+
+	/* Don't go scanning into the front 50% of the list, chances are very
+	   good that there is nothing to be evicted in there and if there is,
+	   it should quickly get pushed into the back 50% */
+	max_depth = UT_LIST_GET_LEN(dict_sys->table_LRU) / 2;
+
+	/* Don't try to evict any more than 10% of evictable tables at once. */
+	max_evict = UT_LIST_GET_LEN(dict_sys->table_LRU) / 10;
+
+	visited = evicted = 0;
+
 	table = UT_LIST_GET_LAST(dict_sys->table_LRU);
 
-	while ( srv_dict_size_limit && table
-		&& ((dict_sys->table_hash->n_cells
-		     + dict_sys->table_id_hash->n_cells) * sizeof(hash_cell_t)
-		    + dict_sys->size) > srv_dict_size_limit ) {
+	while (table && dict_size > srv_dict_size_limit
+	       && visited <= max_depth
+	       && srv_shutdown_state == SRV_SHUTDOWN_NONE) {
+
 		prev_table = UT_LIST_GET_PREV(table_LRU, table);
 
-		if (table == self || table->n_mysql_handles_opened || table->is_corrupt)
-			goto next_loop;
-
-		cached_foreign_tables = 0;
-		foreign = UT_LIST_GET_FIRST(table->foreign_list);
-		while (foreign != NULL) {
-			if (foreign->referenced_table)
-				cached_foreign_tables++;
-			foreign = UT_LIST_GET_NEXT(foreign_list, foreign);
-		}
-
-		if (cached_foreign_tables == 0) {
+		if (table != self && dict_table_can_be_evicted(table)) {
 			dict_table_remove_from_cache(table);
-			n_removed++;
-		} else {
-			n_have_parent++;
+
+			evicted++;
+
+			if (evicted >= max_evict)
+				break;
+
+			/* Only need to recalculate this when something
+			   has been evicted */
+			dict_size = (dict_sys->table_hash->n_cells
+				    + dict_sys->table_id_hash->n_cells)
+				      * sizeof(hash_cell_t) + dict_sys->size;
 		}
-next_loop:
+
+		visited++;
+
 		table = prev_table;
 	}
-
-	if ( srv_dict_size_limit && n_have_parent && n_removed
-		&& ((dict_sys->table_hash->n_cells
-		     + dict_sys->table_id_hash->n_cells) * sizeof(hash_cell_t)
-		    + dict_sys->size) > srv_dict_size_limit )
-		goto retry;
 }
 
 /****************************************************************//**

=== modified file 'Percona-Server/storage/innodb_plugin/dict/dict0mem.c'
--- Percona-Server/storage/innodb_plugin/dict/dict0mem.c	2013-08-05 13:11:52 +0000
+++ Percona-Server/storage/innodb_plugin/dict/dict0mem.c	2014-02-12 18:02:37 +0000
@@ -108,6 +108,7 @@
 	ut_ad(table);
 	ut_ad(table->magic_n == DICT_TABLE_MAGIC_N);
 	ut_d(table->cached = FALSE);
+	ut_d(table->magic_n = 0);
 
 #ifndef UNIV_HOTBACKUP
 	mutex_free(&(table->autoinc_mutex));

=== modified file 'Percona-Server/storage/innodb_plugin/include/lock0lock.h'
--- Percona-Server/storage/innodb_plugin/include/lock0lock.h	2013-08-05 13:11:52 +0000
+++ Percona-Server/storage/innodb_plugin/include/lock0lock.h	2014-02-12 18:02:37 +0000
@@ -458,6 +458,16 @@
 					by a read cursor */
 	const read_view_t*	view);	/*!< in: consistent read view */
 /*********************************************************************//**
+Check if there are any locks (table or rec) against table.
+@return	TRUE if locks exist */
+UNIV_INLINE
+ibool
+lock_table_has_locks(
+/*=================*/
+	const dict_table_t*	table);	/*!< in: check if there are any locks
+					held on records in this table or on the
+					table itself */
+/*********************************************************************//**
 Locks the specified database table in the mode given. If the lock cannot
 be granted immediately, the query thread is put to wait.
 @return	DB_SUCCESS, DB_LOCK_WAIT, DB_DEADLOCK, or DB_QUE_THR_SUSPENDED */

=== modified file 'Percona-Server/storage/innodb_plugin/include/lock0lock.ic'
--- Percona-Server/storage/innodb_plugin/include/lock0lock.ic	2013-06-10 19:43:07 +0000
+++ Percona-Server/storage/innodb_plugin/include/lock0lock.ic	2014-02-12 18:02:37 +0000
@@ -119,3 +119,17 @@
 						   FALSE)));
 	}
 }
+
+/*******************************************************************//**
+Check if there are any locks (table or rec) against table.
+@return	TRUE if table has either table or record locks. */
+UNIV_INLINE
+ibool
+lock_table_has_locks(
+/*=================*/
+	const dict_table_t*	table)	/*!< in: check if there are any locks
+					held on records in this table or on the
+					table itself */
+{
+	return(UT_LIST_GET_LEN(table->locks) > 0);
+}

=== modified file 'Percona-Server/storage/innodb_plugin/lock/lock0lock.c'
--- Percona-Server/storage/innodb_plugin/lock/lock0lock.c	2013-08-05 13:11:52 +0000
+++ Percona-Server/storage/innodb_plugin/lock/lock0lock.c	2014-02-12 18:02:37 +0000
@@ -3766,6 +3766,8 @@
 	trx = lock->trx;
 	table = lock->un_member.tab_lock.table;
 
+	ut_ad(table->magic_n == DICT_TABLE_MAGIC_N);
+
 	/* Remove the table from the transaction's AUTOINC vector, if
 	the lock that is being release is an AUTOINC lock. */
 	if (lock_get_mode(lock) == LOCK_AUTO_INC) {