Below is the list of changes that have just been committed into a local
5.1 repository of antony. When antony does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
ChangeSet@stripped, 2007-01-25 18:02:42-08:00, acurtis@stripped +4 -0
Bug#25721
"Concurrent ALTER/CREATE SERVER can deadlock"
Deadlock caused by inconsistant order of locking mutexes.
Test provided.
mysql-test/r/federated_server.result@stripped, 2007-01-25 18:02:36-08:00, acurtis@stripped +25 -0
test for bug25721
mysql-test/t/federated_server.test@stripped, 2007-01-25 18:02:36-08:00, acurtis@stripped +35 -0
test for bug25721
sql/sql_parse.cc@stripped, 2007-01-25 18:02:37-08:00, acurtis@stripped +1 -1
We do want to report an error when the server we drop doesn't exist
sql/sql_servers.cc@stripped, 2007-01-25 18:02:37-08:00, acurtis@stripped +72 -69
bug25721
Too many mutexes and order of lock operations not consistant leading to deadlock.
Removed servers_cache_mutex, only need the THR_LOCK_servers rwlock mutex.
Alter order of operations to hit memory before disk.
# This is a BitKeeper patch. What follows are the unified diffs for the
# set of deltas contained in the patch. The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User: acurtis
# Host: ltamd64.xiphis.org
# Root: /home/antony/work2/p1-bug25721.1
--- 1.620/sql/sql_parse.cc 2007-01-25 18:02:59 -08:00
+++ 1.621/sql/sql_parse.cc 2007-01-25 18:02:59 -08:00
@@ -5232,7 +5232,7 @@
DBUG_PRINT("info", ("case SQLCOM_DROP_SERVER"));
if ((err_code= drop_server(thd, &lex->server_options)))
{
- if (! lex->drop_if_exists && err_code == ER_FOREIGN_SERVER_EXISTS)
+ if (! lex->drop_if_exists && err_code == ER_FOREIGN_SERVER_DOESNT_EXIST)
{
DBUG_PRINT("info", ("problem dropping server %s",
lex->server_options.server_name));
--- 1.4/mysql-test/r/federated_server.result 2007-01-25 18:02:59 -08:00
+++ 1.5/mysql-test/r/federated_server.result 2007-01-25 18:02:59 -08:00
@@ -106,6 +106,31 @@
drop table second_db.t1;
drop database first_db;
drop database second_db;
+use test;
+create procedure p1 ()
+begin
+DECLARE v INT DEFAULT 0;
+DECLARE e INT DEFAULT 0;
+DECLARE CONTINUE HANDLER FOR SQLEXCEPTION SET e = e + 1;
+WHILE v < 10000 do
+CREATE SERVER s
+FOREIGN DATA WRAPPER mysql
+OPTIONS (USER 'Remote', HOST '192.168.1.106', DATABASE 'test');
+ALTER SERVER s OPTIONS (USER 'Remote');
+DROP SERVER s;
+SET v = v + 1;
+END WHILE;
+SELECT e > 0;
+END//
+use test;
+call p1();
+call p1();
+e > 0
+1
+e > 0
+1
+drop procedure p1;
+drop server if exists s;
DROP TABLE IF EXISTS federated.t1;
DROP DATABASE IF EXISTS federated;
DROP TABLE IF EXISTS federated.t1;
--- 1.4/mysql-test/t/federated_server.test 2007-01-25 18:02:59 -08:00
+++ 1.5/mysql-test/t/federated_server.test 2007-01-25 18:02:59 -08:00
@@ -107,4 +107,39 @@
drop database first_db;
drop database second_db;
+
+#
+# Bug#25721 - deadlock with ALTER/CREATE SERVER
+#
+connect (other,localhost,root,,);
+connection master;
+use test;
+delimiter //;
+create procedure p1 ()
+begin
+ DECLARE v INT DEFAULT 0;
+ DECLARE e INT DEFAULT 0;
+ DECLARE CONTINUE HANDLER FOR SQLEXCEPTION SET e = e + 1;
+ WHILE v < 10000 do
+ CREATE SERVER s
+ FOREIGN DATA WRAPPER mysql
+ OPTIONS (USER 'Remote', HOST '192.168.1.106', DATABASE 'test');
+ ALTER SERVER s OPTIONS (USER 'Remote');
+ DROP SERVER s;
+ SET v = v + 1;
+ END WHILE;
+ SELECT e > 0;
+END//
+delimiter ;//
+connection other;
+use test;
+send call p1();
+connection master;
+call p1();
+connection other;
+reap;
+drop procedure p1;
+drop server if exists s;
+
+
source include/federated_cleanup.inc;
--- 1.4/sql/sql_servers.cc 2007-01-25 18:02:59 -08:00
+++ 1.5/sql/sql_servers.cc 2007-01-25 18:02:59 -08:00
@@ -26,9 +26,9 @@
#include "sp.h"
HASH servers_cache;
-pthread_mutex_t servers_cache_mutex; // To init the hash
uint servers_cache_initialised=FALSE;
/* Version of server table. incremented by servers_load */
+static uint delete_count=0, reload_threshold=1000;
static uint servers_version=0;
static MEM_ROOT mem;
static rw_lock_t THR_LOCK_servers;
@@ -72,9 +72,6 @@
DBUG_ENTER("servers_init");
/* init the mutex */
- if (pthread_mutex_init(&servers_cache_mutex, MY_MUTEX_INIT_FAST))
- DBUG_RETURN(1);
-
if (my_rwlock_init(&THR_LOCK_servers, NULL))
DBUG_RETURN(1);
@@ -143,6 +140,7 @@
/* need to figure out how to utilise this variable */
servers_version++; /* servers updated */
+ delete_count= 0;
/* first, send all cached rows to sleep with the fishes, oblivion!
I expect this crappy comment replaced */
@@ -197,10 +195,9 @@
close_thread_tables(thd);
}
- /*
- To avoid deadlocks we should obtain table locks before
- obtaining servers_cache->lock mutex.
- */
+ DBUG_PRINT("info", ("locking servers_cache"));
+ rw_wrlock(&THR_LOCK_servers);
+
bzero((char*) tables, sizeof(tables));
tables[0].alias= tables[0].table_name= (char*) "servers";
tables[0].db= (char*) "mysql";
@@ -213,12 +210,6 @@
goto end;
}
- DBUG_PRINT("info", ("locking servers_cache"));
- VOID(pthread_mutex_lock(&servers_cache_mutex));
-
- //old_servers_cache= servers_cache;
- //old_mem=mem;
-
if ((return_val= servers_load(thd, tables)))
{ // Error. Revert to old list
/* blast, for now, we have no servers, discuss later way to preserve */
@@ -227,11 +218,10 @@
servers_free();
}
- DBUG_PRINT("info", ("unlocking servers_cache"));
- VOID(pthread_mutex_unlock(&servers_cache_mutex));
-
end:
close_thread_tables(thd);
+ DBUG_PRINT("info", ("unlocking servers_cache"));
+ rw_unlock(&THR_LOCK_servers);
DBUG_RETURN(return_val);
}
@@ -309,6 +299,7 @@
DBUG_RETURN(FALSE);
}
+#ifdef NOT_USED
/*
SYNOPSIS
server_exists_in_table()
@@ -331,7 +322,7 @@
{
byte server_key[MAX_KEY_LENGTH];
int result= 1;
- int error= 0;
+ int error= TRUE;
TABLE_LIST tables;
TABLE *table;
@@ -343,12 +334,11 @@
table->use_all_columns();
- /* need to open before acquiring THR_LOCK_plugin or it will deadlock */
- if (! (table= open_ltable(thd, &tables, TL_WRITE)))
- DBUG_RETURN(TRUE);
+ DBUG_PRINT("info", ("locking servers_cache"));
+ rw_rdlock(&THR_LOCK_servers);
- rw_wrlock(&THR_LOCK_servers);
- VOID(pthread_mutex_lock(&servers_cache_mutex));
+ if (! (table= open_ltable(thd, &tables, TL_READ)))
+ goto err;
/* set the field that's the PK to the value we're looking for */
table->field[0]->store(server_options->server_name,
@@ -370,10 +360,11 @@
server_options->server_name));
}
- VOID(pthread_mutex_unlock(&servers_cache_mutex));
+err:
rw_unlock(&THR_LOCK_servers);
DBUG_RETURN(result);
}
+#endif
/*
SYNOPSIS
@@ -385,6 +376,8 @@
This function takes a server object that is has all members properly
prepared, ready to be inserted both into the mysql.servers table and
the servers cache.
+
+ THR_LOCK_servers must be write locked.
RETURN VALUES
0 - no error
@@ -394,7 +387,7 @@
int insert_server(THD *thd, FOREIGN_SERVER *server)
{
byte server_key[MAX_KEY_LENGTH];
- int error= 0;
+ int error= TRUE;
TABLE_LIST tables;
TABLE *table;
@@ -406,13 +399,7 @@
/* need to open before acquiring THR_LOCK_plugin or it will deadlock */
if (! (table= open_ltable(thd, &tables, TL_WRITE)))
- DBUG_RETURN(TRUE);
-
- /* lock mutex to make sure no changes happen */
- VOID(pthread_mutex_lock(&servers_cache_mutex));
-
- /* lock table */
- rw_wrlock(&THR_LOCK_servers);
+ goto end;
/* insert the server into the table */
if ((error= insert_server_record(table, server)))
@@ -423,9 +410,6 @@
goto end;
end:
- /* unlock the table */
- rw_unlock(&THR_LOCK_servers);
- VOID(pthread_mutex_unlock(&servers_cache_mutex));
DBUG_RETURN(error);
}
@@ -438,6 +422,8 @@
This function takes a FOREIGN_SERVER pointer to an allocated (root mem)
and inserts it into the global servers cache
+ THR_LOCK_servers must be write locked.
+
RETURN VALUE
0 - no error
>0 - error code
@@ -609,7 +595,7 @@
int drop_server(THD *thd, LEX_SERVER_OPTIONS *server_options)
{
byte server_key[MAX_KEY_LENGTH];
- int error= 0;
+ int error;
TABLE_LIST tables;
TABLE *table;
@@ -621,25 +607,31 @@
tables.db= (char*) "mysql";
tables.alias= tables.table_name= (char*) "servers";
- /* need to open before acquiring THR_LOCK_plugin or it will deadlock */
- if (! (table= open_ltable(thd, &tables, TL_WRITE)))
- DBUG_RETURN(TRUE);
-
rw_wrlock(&THR_LOCK_servers);
- VOID(pthread_mutex_lock(&servers_cache_mutex));
-
- if ((error= delete_server_record(table,
- server_options->server_name,
- server_options->server_name_length)))
+ /* hit the memory hit first */
+ if ((error= delete_server_record_in_cache(server_options)))
goto end;
-
- if ((error= delete_server_record_in_cache(server_options)))
+ if (! (table= open_ltable(thd, &tables, TL_WRITE)))
+ {
+ error= my_errno;
goto end;
+ }
+
+ if ((error= delete_server_record(table,
+ server_options->server_name,
+ server_options->server_name_length)) ||
+ delete_count > reload_threshold)
+ {
+ /*
+ If delete_server_record emitted an error, it means that the memory
+ cache has somehow become out of sync.
+ */
+ servers_load(thd, &tables);
+ }
end:
- VOID(pthread_mutex_unlock(&servers_cache_mutex));
rw_unlock(&THR_LOCK_servers);
DBUG_RETURN(error);
}
@@ -663,7 +655,6 @@
int delete_server_record_in_cache(LEX_SERVER_OPTIONS *server_options)
{
-
int error= 0;
FOREIGN_SERVER *server;
DBUG_ENTER("delete_server_record_in_cache");
@@ -680,7 +671,7 @@
DBUG_PRINT("info", ("server_name %s length %d not found!",
server_options->server_name,
server_options->server_name_length));
- // what should be done if not found in the cache?
+ DBUG_RETURN(ER_FOREIGN_SERVER_DOESNT_EXIST);
}
/*
We succeded in deletion of the server to the table, now delete
@@ -690,10 +681,10 @@
server->server_name,
server->server_name_length));
- if (server)
- VOID(hash_delete(&servers_cache, (byte*) server));
+ VOID(hash_delete(&servers_cache, (byte*) server));
servers_version++; /* servers updated */
+ delete_count++; /* we have a hole in the memory space */
DBUG_RETURN(error);
}
@@ -717,6 +708,8 @@
table for the particular server via the call to update_server_record,
and in the servers_cache via update_server_record_in_cache.
+ THR_LOCK_servers must be write locked.
+
RETURN VALUE
0 - no error
>0 - error code
@@ -725,7 +718,7 @@
int update_server(THD *thd, FOREIGN_SERVER *existing, FOREIGN_SERVER *altered)
{
- int error= 0;
+ int error;
TABLE *table;
TABLE_LIST tables;
DBUG_ENTER("update_server");
@@ -735,16 +728,17 @@
tables.alias= tables.table_name= (char*)"servers";
if (!(table= open_ltable(thd, &tables, TL_WRITE)))
- DBUG_RETURN(1);
+ {
+ error= my_errno;
+ goto end;
+ }
- rw_wrlock(&THR_LOCK_servers);
if ((error= update_server_record(table, altered)))
goto end;
update_server_record_in_cache(existing, altered);
end:
- rw_unlock(&THR_LOCK_servers);
DBUG_RETURN(error);
}
@@ -764,6 +758,8 @@
HASH, then the updated record inserted, in essence replacing the old
record.
+ THR_LOCK_servers must be write locked.
+
RETURN VALUE
0 - no error
1 - error
@@ -786,6 +782,7 @@
delete the existing server struct from the server cache
*/
VOID(hash_delete(&servers_cache, (byte*)existing));
+ delete_count++;
/*
Insert the altered server struct into the server cache
@@ -968,16 +965,24 @@
int create_server(THD *thd, LEX_SERVER_OPTIONS *server_options)
{
- int error;
+ int error= ER_FOREIGN_SERVER_EXISTS;
FOREIGN_SERVER *server;
DBUG_ENTER("create_server");
DBUG_PRINT("info", ("server_options->server_name %s",
server_options->server_name));
+ rw_wrlock(&THR_LOCK_servers);
+
+ /* hit the memory first */
+ if (hash_search(&servers_cache, (byte*) server_options->server_name,
+ server_options->server_name_length))
+ goto end;
+
server= (FOREIGN_SERVER *)alloc_root(&mem,
sizeof(FOREIGN_SERVER));
+
if ((error= prepare_server_struct_for_insert(server_options, server)))
goto end;
@@ -987,6 +992,7 @@
DBUG_PRINT("info", ("error returned %d", error));
end:
+ rw_unlock(&THR_LOCK_servers);
DBUG_RETURN(error);
}
@@ -1006,24 +1012,21 @@
int alter_server(THD *thd, LEX_SERVER_OPTIONS *server_options)
{
- int error= 0;
+ int error= ER_FOREIGN_SERVER_DOESNT_EXIST;
FOREIGN_SERVER *altered, *existing;
DBUG_ENTER("alter_server");
DBUG_PRINT("info", ("server_options->server_name %s",
server_options->server_name));
- altered= (FOREIGN_SERVER *)alloc_root(&mem,
- sizeof(FOREIGN_SERVER));
-
- VOID(pthread_mutex_lock(&servers_cache_mutex));
+ rw_wrlock(&THR_LOCK_servers);
if (!(existing= (FOREIGN_SERVER *) hash_search(&servers_cache,
(byte*) server_options->server_name,
server_options->server_name_length)))
- {
- error= ER_FOREIGN_SERVER_DOESNT_EXIST;
goto end;
- }
+
+ altered= (FOREIGN_SERVER *)alloc_root(&mem,
+ sizeof(FOREIGN_SERVER));
if ((error= prepare_server_struct_for_update(server_options, existing, altered)))
goto end;
@@ -1033,7 +1036,7 @@
end:
DBUG_PRINT("info", ("error returned %d", error));
- VOID(pthread_mutex_unlock(&servers_cache_mutex));
+ rw_unlock(&THR_LOCK_servers);
DBUG_RETURN(error);
}
@@ -1183,7 +1186,7 @@
DBUG_ENTER("servers_free");
if (!servers_cache_initialised)
DBUG_VOID_RETURN;
- VOID(pthread_mutex_destroy(&servers_cache_mutex));
+ rwlock_destroy(&THR_LOCK_servers);
servers_cache_initialised=0;
free_root(&mem,MYF(0));
hash_free(&servers_cache);
@@ -1223,7 +1226,7 @@
}
DBUG_PRINT("info", ("locking servers_cache"));
- VOID(pthread_mutex_lock(&servers_cache_mutex));
+ rw_rdlock(&THR_LOCK_servers);
if (!(server= (FOREIGN_SERVER *) hash_search(&servers_cache,
(byte*) server_name,
server_name_length)))
@@ -1233,7 +1236,7 @@
server= (FOREIGN_SERVER *) NULL;
}
DBUG_PRINT("info", ("unlocking servers_cache"));
- VOID(pthread_mutex_unlock(&servers_cache_mutex));
+ rw_unlock(&THR_LOCK_servers);
DBUG_RETURN(server);
}
| Thread |
|---|
| • bk commit into 5.1 tree (acurtis:1.2410) BUG#25721 | antony | 26 Jan |
