Merge lp:~sergei.glushchenko/percona-server/ps51-bug903617 into lp:percona-server/5.1

[Sergei Glushchenko]

Bug903617
Deadlock or crash on concurrent
  TRUNCATE TABLE testdb.t
and
  SELECT * FROM information_schema.tables
           WHERE table_schema="testdb" AND table_name="t"
dict_stats_latches array of RW-locks is used for locking dict
statisctics. Latch to lock is picked by table->id. TRUNCATE TABLE changes
table->id without locking any latches.
Solution is to lock the latch in exclusive mode before changing
table->id. The fact that table->id could change also taken to account
in the latch locking algorithm.

http://jenkins.percona.com/view/Percona%20Server%205.1/job/percona-server-5.1-param/275/

[Alexey Kopytov (akopytov)]

Sergei,

The patch looks good to me, but please add short comments before the for loops and the call to dict_table_stats_lock() explaining why it's being done, as it may not be obvious without knowing the bug context. Please also add the upstream bug number to commit message when you have it.

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

> Sergei,
>
> The patch looks good to me, but please add short comments before the for loops
> and the call to dict_table_stats_lock() explaining why it's being done, as it
> may not be obvious without knowing the bug context. Please also add the
> upstream bug number to commit message when you have it.

DONE.

[Alexey Kopytov (akopytov)]

review: Approve

=== modified file 'Percona-Server/storage/innodb_plugin/dict/dict0dict.c'
--- Percona-Server/storage/innodb_plugin/dict/dict0dict.c	2011-11-24 16:33:30 +0000
+++ Percona-Server/storage/innodb_plugin/dict/dict0dict.c	2012-02-28 03:27:27 +0000
@@ -270,15 +270,39 @@
 	ulint			latch_mode)	/*!< in: RW_S_LATCH or
 						RW_X_LATCH */
 {
+	rw_lock_t *want, *got;
+
 	ut_ad(table != NULL);
 	ut_ad(table->magic_n == DICT_TABLE_MAGIC_N);
 
 	switch (latch_mode) {
 	case RW_S_LATCH:
-		rw_lock_s_lock(GET_TABLE_STATS_LATCH(table));
+		/* Lock one of dict_table_stats_latches in S-mode.
+		Latch is picked using table->id. table->id might be
+		changed while we are waiting for lock to be grabbed */
+		for (;;) {
+			want= GET_TABLE_STATS_LATCH(table);
+			rw_lock_s_lock(want);
+			got= GET_TABLE_STATS_LATCH(table);
+			if (want == got) {
+				break;
+			}
+			rw_lock_s_unlock(want);
+		}
 		break;
 	case RW_X_LATCH:
-		rw_lock_x_lock(GET_TABLE_STATS_LATCH(table));
+		/* Lock one of dict_table_stats_latches in X-mode.
+		Latch is picked using table->id. table->id might be
+		changed while we are waiting for lock to be grabbed */
+		for (;;) {
+			want= GET_TABLE_STATS_LATCH(table);
+			rw_lock_x_lock(want);
+			got= GET_TABLE_STATS_LATCH(table);
+			if (want == got) {
+				break;
+			}
+			rw_lock_x_unlock(want);
+		}
 		break;
 	case RW_NO_LATCH:
 		/* fall through */
@@ -1164,12 +1188,21 @@
 	dict_table_t*	table,	/*!< in/out: table object already in cache */
 	dulint		new_id)	/*!< in: new id to set */
 {
+	dict_table_t table_tmp;
+
 	ut_ad(table);
 	ut_ad(mutex_own(&(dict_sys->mutex)));
 	ut_ad(table->magic_n == DICT_TABLE_MAGIC_N);
 
 	/* Remove the table from the hash table of id's */
 
+	/* Lock is needed to prevent dict_table_stats_latches from
+	being leaked. dict_table_stats_lock picks one latch using
+	table->id. We are changing table->id below. That is why
+	we also should remember the old value to unlock table */
+	dict_table_stats_lock(table, RW_X_LATCH);
+	table_tmp= *table;
+
 	HASH_DELETE(dict_table_t, id_hash, dict_sys->table_id_hash,
 		    ut_fold_dulint(table->id), table);
 	table->id = new_id;
@@ -1177,6 +1210,8 @@
 	/* Add the table back to the hash table */
 	HASH_INSERT(dict_table_t, id_hash, dict_sys->table_id_hash,
 		    ut_fold_dulint(table->id), table);
+
+	dict_table_stats_unlock(&table_tmp, RW_X_LATCH);
 }
 
 /**********************************************************************//**