From 54356b6fcf487e857d229e8dfd99d9f8da50494c Mon Sep 17 00:00:00 2001 From: unknown Date: Wed, 7 Mar 2007 16:30:13 +0100 Subject: [PATCH 01/12] Bug#26782 - Patch: myisamchk -rq creates .MYI.MYI file on packed MyISAM tables When fixing the indexes with "myisamchk -rq" after compressing the table with "myisampack", an optionally supplied extension ".MYI" of the index file was not detected. The extension was appended unconditionally. The result was ".MYI.MYI". Now an extension is no longer appended if present already. Thanks to David Shrewsbury for providing this patch. Another problem was a misplaced parenthesis. We did never unpack the file name ("~/..") and always returned a real path. No test case. This is manually tested with the utilities "myisampack" and "myisamchk". storage/myisam/mi_create.c: Bug#26782 - Patch: myisamchk -rq creates .MYI.MYI file on packed MyISAM tables Added code to detect existing extension on index file name. Thanks to David Shrewsbury for providing this patch. Additionally fixed an parenthesis. We did never unpack the file name and always returned real path. --- storage/myisam/mi_create.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/storage/myisam/mi_create.c b/storage/myisam/mi_create.c index 2b8cbcc7da5..e2245aff065 100644 --- a/storage/myisam/mi_create.c +++ b/storage/myisam/mi_create.c @@ -599,10 +599,12 @@ int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs, } else { + char *iext= strrchr(name, '.'); + int have_iext= iext && !strcmp(iext, MI_NAME_IEXT); fn_format(filename, name, "", MI_NAME_IEXT, - (MY_UNPACK_FILENAME | - (flags & HA_DONT_TOUCH_DATA) ? MY_RETURN_REAL_PATH : 0) | - MY_APPEND_EXT); + MY_UNPACK_FILENAME | + ((flags & HA_DONT_TOUCH_DATA) ? MY_RETURN_REAL_PATH : 0) | + (have_iext ? MY_REPLACE_EXT : MY_APPEND_EXT)); linkname_ptr=0; /* Replace the current file */ create_flag=MY_DELETE_OLD; From 6d98913be66c402b19e5827e2a69711cef68c66e Mon Sep 17 00:00:00 2001 From: unknown Date: Tue, 13 Mar 2007 11:58:24 -0700 Subject: [PATCH 02/12] Bug#25671 "CREATE/DROP/ALTER SERVER should require privileges" Add check for SUPER privilege when executing CREATE/DROP/ALTER SERVER. Previously, any user even with only USAGE priv can execute those commands. mysql-test/r/federated_server.result: Bug25671 - CREATE/DROP/ALTER SERVER should require privileges mysql-test/t/federated_server.test: Bug25671 - CREATE/DROP/ALTER SERVER should require privileges sql/sql_parse.cc: Bug25671 - CREATE/DROP/ALTER SERVER should require privileges Add check for SUPER privilege when executing CREATE/DROP/ALTER SERVER --- mysql-test/r/federated_server.result | 85 ++++++++++++++++++ mysql-test/t/federated_server.test | 127 +++++++++++++++++++++++++++ sql/sql_parse.cc | 12 +++ 3 files changed, 224 insertions(+) diff --git a/mysql-test/r/federated_server.result b/mysql-test/r/federated_server.result index 7a1a6e0970d..3682c0c793f 100644 --- a/mysql-test/r/federated_server.result +++ b/mysql-test/r/federated_server.result @@ -104,6 +104,91 @@ drop table first_db.t1; drop table second_db.t1; drop database first_db; drop database second_db; +create database db_legitimate; +create database db_bogus; +use db_legitimate; +CREATE TABLE db_legitimate.t1 ( +`id` int(20) NOT NULL, +`name` varchar(64) NOT NULL default '' + ); +INSERT INTO db_legitimate.t1 VALUES ('1','this is legitimate'); +use db_bogus; +CREATE TABLE db_bogus.t1 ( +`id` int(20) NOT NULL, +`name` varchar(64) NOT NULL default '' + ) +; +INSERT INTO db_bogus.t1 VALUES ('2','this is bogus'); +create server 's1' foreign data wrapper 'mysql' options +(HOST '127.0.0.1', +DATABASE 'db_legitimate', +USER 'root', +PASSWORD '', +PORT SLAVE_PORT, +SOCKET '', +OWNER 'root'); +create user guest_select@localhost; +grant select on federated.* to guest_select@localhost; +create user guest_super@localhost; +grant select,SUPER,RELOAD on *.* to guest_super@localhost; +create user guest_usage@localhost; +grant usage on *.* to guest_usage@localhost; +CREATE TABLE federated.t1 ( +`id` int(20) NOT NULL, +`name` varchar(64) NOT NULL default '' + ) ENGINE = FEDERATED CONNECTION = 's1'; +select * from federated.t1; +id name +1 this is legitimate +alter server s1 options (database 'db_bogus'); +ERROR 42000: Access denied; you need the SUPER privilege for this operation +flush tables; +select * from federated.t1; +id name +1 this is legitimate +alter server s1 options (database 'db_bogus'); +ERROR 42000: Access denied; you need the SUPER privilege for this operation +flush tables; +select * from federated.t1; +id name +1 this is legitimate +alter server s1 options (database 'db_bogus'); +flush tables; +select * from federated.t1; +id name +2 this is bogus +drop server if exists 's1'; +ERROR 42000: Access denied; you need the SUPER privilege for this operation +create server 's1' foreign data wrapper 'mysql' options +(HOST '127.0.0.1', +DATABASE 'db_legitimate', +USER 'root', +PASSWORD '', +PORT SLAVE_PORT, +SOCKET '', +OWNER 'root'); +ERROR 42000: Access denied; you need the SUPER privilege for this operation +drop server 's1'; +create server 's1' foreign data wrapper 'mysql' options +(HOST '127.0.0.1', +DATABASE 'db_legitimate', +USER 'root', +PASSWORD '', +PORT SLAVE_PORT, +SOCKET '', +OWNER 'root'); +flush tables; +select * from federated.t1; +id name +1 this is legitimate +drop database db_legitimate; +drop database db_bogus; +drop user guest_super@localhost; +drop user guest_usage@localhost; +drop user guest_select@localhost; +drop table federated.t1; +drop server 's1'; +# End of 5.1 tests DROP TABLE IF EXISTS federated.t1; DROP DATABASE IF EXISTS federated; DROP TABLE IF EXISTS federated.t1; diff --git a/mysql-test/t/federated_server.test b/mysql-test/t/federated_server.test index 3e47d9bc95d..e65f319f98d 100644 --- a/mysql-test/t/federated_server.test +++ b/mysql-test/t/federated_server.test @@ -107,4 +107,131 @@ drop table second_db.t1; drop database first_db; drop database second_db; +# +# Bug#25671 - CREATE/DROP/ALTER SERVER should require privileges +# +# Changes to SERVER declarations should require SUPER privilege. +# Based upon test case by Giuseppe Maxia + +create database db_legitimate; +create database db_bogus; + +use db_legitimate; +CREATE TABLE db_legitimate.t1 ( + `id` int(20) NOT NULL, + `name` varchar(64) NOT NULL default '' + ); +INSERT INTO db_legitimate.t1 VALUES ('1','this is legitimate'); + +use db_bogus; +CREATE TABLE db_bogus.t1 ( + `id` int(20) NOT NULL, + `name` varchar(64) NOT NULL default '' + ) + ; +INSERT INTO db_bogus.t1 VALUES ('2','this is bogus'); + +connection master; +--replace_result $SLAVE_MYPORT SLAVE_PORT +eval create server 's1' foreign data wrapper 'mysql' options + (HOST '127.0.0.1', + DATABASE 'db_legitimate', + USER 'root', + PASSWORD '', + PORT $SLAVE_MYPORT, + SOCKET '', + OWNER 'root'); + +create user guest_select@localhost; +grant select on federated.* to guest_select@localhost; + +create user guest_super@localhost; +grant select,SUPER,RELOAD on *.* to guest_super@localhost; + +create user guest_usage@localhost; +grant usage on *.* to guest_usage@localhost; + +CREATE TABLE federated.t1 ( + `id` int(20) NOT NULL, + `name` varchar(64) NOT NULL default '' + ) ENGINE = FEDERATED CONNECTION = 's1'; + +select * from federated.t1; + +connect (conn_select,127.0.0.1,guest_select,,federated,$MASTER_MYPORT); +connect (conn_usage,127.0.0.1,guest_usage,,,$MASTER_MYPORT); +connect (conn_super,127.0.0.1,guest_super,,,$MASTER_MYPORT); + +connection conn_select; +--error ER_SPECIFIC_ACCESS_DENIED_ERROR +alter server s1 options (database 'db_bogus'); + +connection master; +flush tables; +select * from federated.t1; + +connection conn_usage; +--error ER_SPECIFIC_ACCESS_DENIED_ERROR +alter server s1 options (database 'db_bogus'); + +connection master; +flush tables; +select * from federated.t1; + +connection conn_super; +alter server s1 options (database 'db_bogus'); + +connection master; +flush tables; +select * from federated.t1; + +connection conn_select; +--error ER_SPECIFIC_ACCESS_DENIED_ERROR +drop server if exists 's1'; +--replace_result $SLAVE_MYPORT SLAVE_PORT +--error ER_SPECIFIC_ACCESS_DENIED_ERROR +eval create server 's1' foreign data wrapper 'mysql' options + (HOST '127.0.0.1', + DATABASE 'db_legitimate', + USER 'root', + PASSWORD '', + PORT $SLAVE_MYPORT, + SOCKET '', + OWNER 'root'); + +connection conn_super; +drop server 's1'; +--replace_result $SLAVE_MYPORT SLAVE_PORT +eval create server 's1' foreign data wrapper 'mysql' options + (HOST '127.0.0.1', + DATABASE 'db_legitimate', + USER 'root', + PASSWORD '', + PORT $SLAVE_MYPORT, + SOCKET '', + OWNER 'root'); + +connection master; +flush tables; +select * from federated.t1; + +# clean up test +connection slave; +drop database db_legitimate; +drop database db_bogus; + +disconnect conn_select; +disconnect conn_usage; +disconnect conn_super; + +connection master; +drop user guest_super@localhost; +drop user guest_usage@localhost; +drop user guest_select@localhost; +drop table federated.t1; +drop server 's1'; + + +--echo # End of 5.1 tests + source include/federated_cleanup.inc; diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index bfcbd4663b4..6684b741464 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -4273,6 +4273,10 @@ create_sp_error: int error; LEX *lex= thd->lex; DBUG_PRINT("info", ("case SQLCOM_CREATE_SERVER")); + + if (check_global_access(thd, SUPER_ACL)) + break; + if ((error= create_server(thd, &lex->server_options))) { DBUG_PRINT("info", ("problem creating server <%s>", @@ -4288,6 +4292,10 @@ create_sp_error: int error; LEX *lex= thd->lex; DBUG_PRINT("info", ("case SQLCOM_ALTER_SERVER")); + + if (check_global_access(thd, SUPER_ACL)) + break; + if ((error= alter_server(thd, &lex->server_options))) { DBUG_PRINT("info", ("problem altering server <%s>", @@ -4303,6 +4311,10 @@ create_sp_error: int err_code; LEX *lex= thd->lex; DBUG_PRINT("info", ("case SQLCOM_DROP_SERVER")); + + if (check_global_access(thd, SUPER_ACL)) + break; + if ((err_code= drop_server(thd, &lex->server_options))) { if (! lex->drop_if_exists && err_code == ER_FOREIGN_SERVER_EXISTS) From 3c89dd7966e727d7ded902b195d8f133de94b565 Mon Sep 17 00:00:00 2001 From: unknown Date: Wed, 14 Mar 2007 16:27:37 +0100 Subject: [PATCH 03/12] Bug#25289 - repair table causes "my_seek.c:56: my_seek: Assertion `fd != -1' failed" In difficult optimize/repair situations the server could crash. Under some circumstances the server retries an optimize/repair with more elaborate options. But it did not check if the first attempt failed so badly that a second one must not be tried. This could happen when a new data file has been created but it was not possible to open it. In this case the repair leaves behind a table with closed data file. This must not be used for another repair attempt. We do now detect the closed data file and do not try another repair attempt in this situation. No test case. The required table corruption can not be repeated easily. There is a test program attached to bug 25433. sql/ha_myisam.cc: Bug#25289 - repair table causes "my_seek.c:56: my_seek: Assertion `fd != -1' failed" Added code to detect a closed data file. It could be closed by a preceeding repair attempt. We must not try another repair then. --- sql/ha_myisam.cc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/sql/ha_myisam.cc b/sql/ha_myisam.cc index 2cec03a51db..42be008de9e 100644 --- a/sql/ha_myisam.cc +++ b/sql/ha_myisam.cc @@ -867,6 +867,22 @@ int ha_myisam::repair(THD *thd, MI_CHECK ¶m, bool optimize) ha_rows rows= file->state->records; DBUG_ENTER("ha_myisam::repair"); + /* + Normally this method is entered with a properly opened table. If the + repair fails, it can be repeated with more elaborate options. Under + special circumstances it can happen that a repair fails so that it + closed the data file and cannot re-open it. In this case file->dfile + is set to -1. We must not try another repair without an open data + file. (Bug #25289) + */ + if (file->dfile == -1) + { + sql_print_information("Retrying repair of: '%s' failed. " + "Please try REPAIR EXTENDED or myisamchk", + table->path); + DBUG_RETURN(HA_ADMIN_FAILED); + } + param.db_name = table->table_cache_key; param.table_name = table->table_name; param.tmpfile_createflag = O_RDWR | O_TRUNC; From db573e637c2ba66f214f7e1e2172ce9efeca0db0 Mon Sep 17 00:00:00 2001 From: unknown Date: Mon, 19 Mar 2007 15:56:53 +0100 Subject: [PATCH 04/12] Bug#26996 - Update of a Field in a Memory Table ends with wrong result Using a MEMORY table BTREE index for scanning for updatable rows could lead to an infinite loop. Everytime a key was inserted into a btree index, the position in the index scan was cleared. The search started from the beginning and found the same key again. Now we do not clear the position on key insert an more. heap/hp_write.c: Bug#26996 - Update of a Field in a Memory Table ends with wrong result Removed the index-scan-breaking nulling of last_pos. The comment behind this line ("For heap_rnext/heap_rprev") was misleading. It should have been "Breaks heap_rnext/heap_rprev". mysql-test/r/heap_btree.result: Bug#26996 - Update of a Field in a Memory Table ends with wrong result Added test result. mysql-test/t/heap_btree.test: Bug#26996 - Update of a Field in a Memory Table ends with wrong result Added test. --- heap/hp_write.c | 1 - mysql-test/r/heap_btree.result | 15 +++++++++++++++ mysql-test/t/heap_btree.test | 14 ++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/heap/hp_write.c b/heap/hp_write.c index 841dda6264e..59dfca31fd9 100644 --- a/heap/hp_write.c +++ b/heap/hp_write.c @@ -106,7 +106,6 @@ int hp_rb_write_key(HP_INFO *info, HP_KEYDEF *keyinfo, const byte *record, heap_rb_param custom_arg; uint old_allocated; - info->last_pos= NULL; /* For heap_rnext/heap_rprev */ custom_arg.keyseg= keyinfo->seg; custom_arg.key_length= hp_rb_make_key(keyinfo, info->recbuf, record, recpos); if (keyinfo->flag & HA_NOSAME) diff --git a/mysql-test/r/heap_btree.result b/mysql-test/r/heap_btree.result index e6492e90b80..1a0666514be 100644 --- a/mysql-test/r/heap_btree.result +++ b/mysql-test/r/heap_btree.result @@ -280,4 +280,19 @@ a 1 1 drop table t1; +CREATE TABLE t1 ( +c1 CHAR(3), +c2 INTEGER, +KEY USING BTREE(c1), +KEY USING BTREE(c2) +) ENGINE= MEMORY; +INSERT INTO t1 VALUES ('ABC',0), ('A',0), ('B',0), ('C',0); +UPDATE t1 SET c2= c2 + 1 WHERE c1 = 'A'; +SELECT * FROM t1; +c1 c2 +ABC 0 +A 1 +B 0 +C 0 +DROP TABLE t1; End of 4.1 tests diff --git a/mysql-test/t/heap_btree.test b/mysql-test/t/heap_btree.test index 9aa820becd9..63baa968981 100644 --- a/mysql-test/t/heap_btree.test +++ b/mysql-test/t/heap_btree.test @@ -182,4 +182,18 @@ delete from t1 where a >= 2; select a from t1 order by a; drop table t1; +# +# Bug#26996 - Update of a Field in a Memory Table ends with wrong result +# +CREATE TABLE t1 ( + c1 CHAR(3), + c2 INTEGER, + KEY USING BTREE(c1), + KEY USING BTREE(c2) +) ENGINE= MEMORY; +INSERT INTO t1 VALUES ('ABC',0), ('A',0), ('B',0), ('C',0); +UPDATE t1 SET c2= c2 + 1 WHERE c1 = 'A'; +SELECT * FROM t1; +DROP TABLE t1; + --echo End of 4.1 tests From a5c27d6cb3b234318cd578b772508c7700d47856 Mon Sep 17 00:00:00 2001 From: unknown Date: Wed, 21 Mar 2007 17:12:30 +0400 Subject: [PATCH 05/12] BUG#25908 - corrupted myisam table crashes server even after repair Opening certain tables that have different definitions in .MYI and .frm may result in a server crash. Compare .MYI and .frm definition when myisam table is opened. In case definitions are diffirent refuse to open such table. No test case, since it requires broken table. storage/myisam/ha_myisam.cc: Compare .MYI and .frm definition when myisam table is opened. In case definitions are diffirent refuse to open such table. --- storage/myisam/ha_myisam.cc | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc index a67d62f7447..587e5220e1c 100644 --- a/storage/myisam/ha_myisam.cc +++ b/storage/myisam/ha_myisam.cc @@ -632,6 +632,9 @@ bool ha_myisam::check_if_locking_is_allowed(uint sql_command, int ha_myisam::open(const char *name, int mode, uint test_if_locked) { + MI_KEYDEF *keyinfo; + MI_COLUMNDEF *recinfo= 0; + uint recs; uint i; /* @@ -654,6 +657,26 @@ int ha_myisam::open(const char *name, int mode, uint test_if_locked) if (!(file=mi_open(name, mode, test_if_locked | HA_OPEN_FROM_SQL_LAYER))) return (my_errno ? my_errno : -1); + if (!table->s->tmp_table) /* No need to perform a check for tmp table */ + { + if ((my_errno= table2myisam(table, &keyinfo, &recinfo, &recs))) + { + /* purecov: begin inspected */ + DBUG_PRINT("error", ("Failed to convert TABLE object to MyISAM " + "key and column definition")); + goto err; + /* purecov: end */ + } + if (check_definition(keyinfo, recinfo, table->s->keys, recs, + file->s->keyinfo, file->s->rec, + file->s->base.keys, file->s->base.fields, true)) + { + /* purecov: begin inspected */ + my_errno= HA_ERR_CRASHED; + goto err; + /* purecov: end */ + } + } if (test_if_locked & (HA_OPEN_IGNORE_IF_LOCKED | HA_OPEN_TMP_TABLE)) VOID(mi_extra(file, HA_EXTRA_NO_WAIT_LOCK, 0)); @@ -675,6 +698,15 @@ int ha_myisam::open(const char *name, int mode, uint test_if_locked) table->key_info[i].block_size= file->s->keyinfo[i].block_length; } return (0); +err: + this->close(); + /* + Both recinfo and keydef are allocated by my_multi_malloc(), thus only + recinfo must be freed. + */ + if (recinfo) + my_free((gptr) recinfo, MYF(0)); + return my_errno; } int ha_myisam::close(void) From eebba6a2c4ce2776fa471b58d175bedb60cf096b Mon Sep 17 00:00:00 2001 From: unknown Date: Thu, 22 Mar 2007 21:28:28 +0100 Subject: [PATCH 06/12] After merge fix --- storage/myisam/ha_myisam.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc index 6f4d12ec5f7..505febf8828 100644 --- a/storage/myisam/ha_myisam.cc +++ b/storage/myisam/ha_myisam.cc @@ -697,9 +697,11 @@ int ha_myisam::open(const char *name, int mode, uint test_if_locked) (struct st_mysql_ftparser *)parser->plugin->info; table->key_info[i].block_size= file->s->keyinfo[i].block_length; } - return (0); -err: + my_errno= 0; + goto end; + err: this->close(); + end: /* Both recinfo and keydef are allocated by my_multi_malloc(), thus only recinfo must be freed. From ab785bfe5b505a0a5c7e91edcb5f5fc2a2516b86 Mon Sep 17 00:00:00 2001 From: unknown Date: Fri, 23 Mar 2007 10:26:14 +0100 Subject: [PATCH 07/12] Bug#26782 - Patch: myisamchk -rq creates .MYI.MYI file on packed MyISAM tables After merge fix In conjunction with the newest 5.1 we always need to create a real path name to be able to detect already open tables. --- storage/myisam/mi_create.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/storage/myisam/mi_create.c b/storage/myisam/mi_create.c index 5ce07c5ac4e..71d377c8b6b 100644 --- a/storage/myisam/mi_create.c +++ b/storage/myisam/mi_create.c @@ -573,6 +573,10 @@ int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs, pthread_mutex_lock(&THR_LOCK_myisam); + /* + NOTE: For test_if_reopen() we need a real path name. Hence we need + MY_RETURN_REAL_PATH for every fn_format(filename, ...). + */ if (ci->index_file_name) { char *iext= strrchr(ci->index_file_name, '.'); @@ -584,13 +588,14 @@ int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs, if ((path= strrchr(ci->index_file_name, FN_LIBCHAR))) *path= '\0'; fn_format(filename, name, ci->index_file_name, MI_NAME_IEXT, - MY_REPLACE_DIR | MY_UNPACK_FILENAME | MY_APPEND_EXT); + MY_REPLACE_DIR | MY_UNPACK_FILENAME | + MY_RETURN_REAL_PATH | MY_APPEND_EXT); } else { fn_format(filename, ci->index_file_name, "", MI_NAME_IEXT, - MY_UNPACK_FILENAME | (have_iext ? MY_REPLACE_EXT : - MY_APPEND_EXT)); + MY_UNPACK_FILENAME | MY_RETURN_REAL_PATH | + (have_iext ? MY_REPLACE_EXT : MY_APPEND_EXT)); } fn_format(linkname, name, "", MI_NAME_IEXT, MY_UNPACK_FILENAME|MY_APPEND_EXT); @@ -606,8 +611,7 @@ int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs, char *iext= strrchr(name, '.'); int have_iext= iext && !strcmp(iext, MI_NAME_IEXT); fn_format(filename, name, "", MI_NAME_IEXT, - MY_UNPACK_FILENAME | - ((flags & HA_DONT_TOUCH_DATA) ? MY_RETURN_REAL_PATH : 0) | + MY_UNPACK_FILENAME | MY_RETURN_REAL_PATH | (have_iext ? MY_REPLACE_EXT : MY_APPEND_EXT)); linkname_ptr=0; /* Replace the current file */ @@ -620,6 +624,9 @@ int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs, A TRUNCATE command checks for the table in the cache only and could be fooled to believe, the table is not open. Pull the emergency brake in this situation. (Bug #8306) + + NOTE: The filename is compared against unique_file_name of every + open table. Hence we need a real path here. */ if (test_if_reopen(filename)) { From cb5b56c8efc1752d6768547a6f0517455cdbe7e2 Mon Sep 17 00:00:00 2001 From: unknown Date: Fri, 23 Mar 2007 17:31:27 -0700 Subject: [PATCH 08/12] Bug#25721 "Concurrent ALTER/CREATE SERVER can lead to deadlock" Deadlock caused by inconsistant use of mutexes in sql_server.cc One mutex has been removed to resolve deadlock. Many functions were made private which should not be exported. Unused variables and function removed. mysql-test/r/federated_server.result: Bug 25721 "Concurrent ALTER/CREATE SERVER can lead to deadlock" New test results mysql-test/t/federated_server.test: Bug 25721 "Concurrent ALTER/CREATE SERVER can lead to deadlock" Test for bug by using stored procedure. Unpatched server would deadlock frequently. sql/sql_parse.cc: Bug 25721 "Concurrent ALTER/CREATE SERVER can lead to deadlock" check for correct error code when dropping server sql/sql_servers.cc: Bug 25721 "Concurrent ALTER/CREATE SERVER can lead to deadlock" Removed unneccessary mutex, only need THR_LOCK_servers rwlock to guard data structures against race conditions. Misuse of other mutex caused deadlock by inconsistant ordering of mutex lock operations. Alter order of some operations to hit memory before disk. Removed unused function. Removed servers_version and servers_cache_initialised variables. Made many internal functions static. sql/sql_servers.h: Bug 25721 "Concurrent ALTER/CREATE SERVER can lead to deadlock" remove internal functions from being exported. --- mysql-test/r/federated_server.result | 25 ++ mysql-test/t/federated_server.test | 35 +++ sql/sql_parse.cc | 2 +- sql/sql_servers.cc | 361 ++++++++++++--------------- sql/sql_servers.h | 30 +-- 5 files changed, 226 insertions(+), 227 deletions(-) diff --git a/mysql-test/r/federated_server.result b/mysql-test/r/federated_server.result index 3682c0c793f..8faf33581d1 100644 --- a/mysql-test/r/federated_server.result +++ b/mysql-test/r/federated_server.result @@ -189,6 +189,31 @@ drop user guest_select@localhost; drop table federated.t1; drop server 's1'; # End of 5.1 tests +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; diff --git a/mysql-test/t/federated_server.test b/mysql-test/t/federated_server.test index e65f319f98d..27dd494e5c5 100644 --- a/mysql-test/t/federated_server.test +++ b/mysql-test/t/federated_server.test @@ -234,4 +234,39 @@ drop server 's1'; --echo # End of 5.1 tests + +# +# 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; diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index a0e412afdf0..23bbeb594d5 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -4320,7 +4320,7 @@ create_sp_error: 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)); diff --git a/sql/sql_servers.cc b/sql/sql_servers.cc index 9a5176d9fe3..9b01a1b6889 100644 --- a/sql/sql_servers.cc +++ b/sql/sql_servers.cc @@ -25,14 +25,43 @@ #include "sp_head.h" #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 servers_version=0; +/* + We only use 1 mutex to guard the data structures - THR_LOCK_servers. + Read locked when only reading data and write-locked for all other access. +*/ + +static HASH servers_cache; static MEM_ROOT mem; static rw_lock_t THR_LOCK_servers; +static bool get_server_from_table_to_cache(TABLE *table); + +/* insert functions */ +static int insert_server(THD *thd, FOREIGN_SERVER *server_options); +static int insert_server_record(TABLE *table, FOREIGN_SERVER *server); +static int insert_server_record_into_cache(FOREIGN_SERVER *server); +static void prepare_server_struct_for_insert(LEX_SERVER_OPTIONS *server_options, + FOREIGN_SERVER *server); +/* drop functions */ +static int delete_server_record(TABLE *table, + char *server_name, + int server_name_length); +static int delete_server_record_in_cache(LEX_SERVER_OPTIONS *server_options); + +/* update functions */ +static void prepare_server_struct_for_update(LEX_SERVER_OPTIONS *server_options, + FOREIGN_SERVER *existing, + FOREIGN_SERVER *altered); +static int update_server(THD *thd, FOREIGN_SERVER *existing, + FOREIGN_SERVER *altered); +static int update_server_record(TABLE *table, FOREIGN_SERVER *server); +static int update_server_record_in_cache(FOREIGN_SERVER *existing, + FOREIGN_SERVER *altered); +/* utility functions */ +static void merge_server_struct(FOREIGN_SERVER *from, FOREIGN_SERVER *to); + + + static byte *servers_cache_get_key(FOREIGN_SERVER *server, uint *length, my_bool not_used __attribute__((unused))) { @@ -45,6 +74,7 @@ static byte *servers_cache_get_key(FOREIGN_SERVER *server, uint *length, DBUG_RETURN((byte*) server->server_name); } + /* Initialize structures responsible for servers used in federated server scheme information for them from the server @@ -64,35 +94,27 @@ static byte *servers_cache_get_key(FOREIGN_SERVER *server, uint *length, 1 Could not initialize servers */ -my_bool servers_init(bool dont_read_servers_table) +bool servers_init(bool dont_read_servers_table) { THD *thd; - my_bool return_val= 0; + bool return_val= FALSE; 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); + DBUG_RETURN(TRUE); /* initialise our servers cache */ if (hash_init(&servers_cache, system_charset_info, 32, 0, 0, (hash_get_key) servers_cache_get_key, 0, 0)) { - return_val= 1; /* we failed, out of memory? */ + return_val= TRUE; /* we failed, out of memory? */ goto end; } /* Initialize the mem root for data */ init_alloc_root(&mem, ACL_ALLOC_BLOCK_SIZE, 0); - /* - at this point, the cache is initialised, let it be known - */ - servers_cache_initialised= TRUE; - if (dont_read_servers_table) goto end; @@ -100,7 +122,7 @@ my_bool servers_init(bool dont_read_servers_table) To be able to run this from boot, we allocate a temporary THD */ if (!(thd=new THD)) - DBUG_RETURN(1); + DBUG_RETURN(TRUE); thd->thread_stack= (char*) &thd; thd->store_globals(); /* @@ -130,19 +152,13 @@ end: TRUE Error */ -static my_bool servers_load(THD *thd, TABLE_LIST *tables) +static bool servers_load(THD *thd, TABLE_LIST *tables) { TABLE *table; READ_RECORD read_record_info; - my_bool return_val= TRUE; + bool return_val= TRUE; DBUG_ENTER("servers_load"); - if (!servers_cache_initialised) - DBUG_RETURN(0); - - /* need to figure out how to utilise this variable */ - servers_version++; /* servers updated */ - /* first, send all cached rows to sleep with the fishes, oblivion! I expect this crappy comment replaced */ free_root(&mem, MYF(MY_MARK_BLOCKS_FREE)); @@ -156,7 +172,7 @@ static my_bool servers_load(THD *thd, TABLE_LIST *tables) goto end; } - return_val=0; + return_val= FALSE; end: end_read_record(&read_record_info); @@ -183,10 +199,10 @@ end: TRUE Failure */ -my_bool servers_reload(THD *thd) +bool servers_reload(THD *thd) { TABLE_LIST tables[1]; - my_bool return_val= 1; + bool return_val= TRUE; DBUG_ENTER("servers_reload"); if (thd->locked_tables) @@ -196,10 +212,9 @@ my_bool servers_reload(THD *thd) 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"; @@ -212,12 +227,6 @@ my_bool servers_reload(THD *thd) 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 */ @@ -226,14 +235,14 @@ my_bool servers_reload(THD *thd) 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); } + /* Initialize structures responsible for servers used in federated server scheme information for them from the server @@ -260,7 +269,8 @@ end: 1 could not insert server struct into global servers cache */ -my_bool get_server_from_table_to_cache(TABLE *table) +static bool +get_server_from_table_to_cache(TABLE *table) { /* alloc a server struct */ char *ptr; @@ -308,69 +318,6 @@ my_bool get_server_from_table_to_cache(TABLE *table) DBUG_RETURN(FALSE); } -/* - SYNOPSIS - server_exists_in_table() - THD *thd - thread pointer - LEX_SERVER_OPTIONS *server_options - pointer to Lex->server_options - - NOTES - This function takes a LEX_SERVER_OPTIONS struct, which is very much the - same type of structure as a FOREIGN_SERVER, it contains the values parsed - in any one of the [CREATE|DELETE|DROP] SERVER statements. Using the - member "server_name", index_read_idx either founds the record and returns - 1, or doesn't find the record, and returns 0 - - RETURN VALUES - 0 record not found - 1 record found -*/ - -my_bool server_exists_in_table(THD *thd, LEX_SERVER_OPTIONS *server_options) -{ - int result= 1; - int error= 0; - TABLE_LIST tables; - TABLE *table; - DBUG_ENTER("server_exists"); - - bzero((char*) &tables, sizeof(tables)); - 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); - - table->use_all_columns(); - - rw_wrlock(&THR_LOCK_servers); - VOID(pthread_mutex_lock(&servers_cache_mutex)); - - /* set the field that's the PK to the value we're looking for */ - table->field[0]->store(server_options->server_name, - server_options->server_name_length, - system_charset_info); - - if ((error= table->file->index_read_idx(table->record[0], 0, - (byte *)table->field[0]->ptr, - table->key_info[0].key_length, - HA_READ_KEY_EXACT))) - { - if (error != HA_ERR_KEY_NOT_FOUND && error != HA_ERR_END_OF_FILE) - { - table->file->print_error(error, MYF(0)); - result= -1; - } - result= 0; - DBUG_PRINT("info",("record for server '%s' not found!", - server_options->server_name)); - } - - VOID(pthread_mutex_unlock(&servers_cache_mutex)); - rw_unlock(&THR_LOCK_servers); - DBUG_RETURN(result); -} /* SYNOPSIS @@ -382,15 +329,18 @@ my_bool server_exists_in_table(THD *thd, LEX_SERVER_OPTIONS *server_options) 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 other - error code */ -int insert_server(THD *thd, FOREIGN_SERVER *server) +static int +insert_server(THD *thd, FOREIGN_SERVER *server) { - int error= 0; + int error= -1; TABLE_LIST tables; TABLE *table; @@ -402,13 +352,7 @@ int insert_server(THD *thd, FOREIGN_SERVER *server) /* 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))) @@ -419,12 +363,10 @@ int insert_server(THD *thd, FOREIGN_SERVER *server) goto end; end: - /* unlock the table */ - rw_unlock(&THR_LOCK_servers); - VOID(pthread_mutex_unlock(&servers_cache_mutex)); DBUG_RETURN(error); } + /* SYNOPSIS int insert_server_record_into_cache() @@ -434,13 +376,16 @@ end: 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 */ -int insert_server_record_into_cache(FOREIGN_SERVER *server) +static int +insert_server_record_into_cache(FOREIGN_SERVER *server) { int error=0; DBUG_ENTER("insert_server_record_into_cache"); @@ -461,6 +406,7 @@ int insert_server_record_into_cache(FOREIGN_SERVER *server) DBUG_RETURN(error); } + /* SYNOPSIS store_server_fields() @@ -478,7 +424,8 @@ int insert_server_record_into_cache(FOREIGN_SERVER *server) */ -void store_server_fields(TABLE *table, FOREIGN_SERVER *server) +static void +store_server_fields(TABLE *table, FOREIGN_SERVER *server) { table->use_all_columns(); @@ -539,6 +486,7 @@ void store_server_fields(TABLE *table, FOREIGN_SERVER *server) */ +static int insert_server_record(TABLE *table, FOREIGN_SERVER *server) { int error; @@ -606,7 +554,7 @@ int insert_server_record(TABLE *table, FOREIGN_SERVER *server) int drop_server(THD *thd, LEX_SERVER_OPTIONS *server_options) { - int error= 0; + int error; TABLE_LIST tables; TABLE *table; @@ -618,28 +566,33 @@ int drop_server(THD *thd, LEX_SERVER_OPTIONS *server_options) 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))) - goto end; - + /* hit the memory hit first */ if ((error= delete_server_record_in_cache(server_options))) goto end; + if (! (table= open_ltable(thd, &tables, TL_WRITE))) + { + error= my_errno; + goto end; + } + + error= delete_server_record(table, + server_options->server_name, + server_options->server_name_length); + + /* + Perform a reload so we don't have a 'hole' in our mem_root + */ + servers_load(thd, &tables); + end: - VOID(pthread_mutex_unlock(&servers_cache_mutex)); rw_unlock(&THR_LOCK_servers); DBUG_RETURN(error); } + + /* SYNOPSIS @@ -658,10 +611,10 @@ end: */ -int delete_server_record_in_cache(LEX_SERVER_OPTIONS *server_options) +static int +delete_server_record_in_cache(LEX_SERVER_OPTIONS *server_options) { - - int error= 0; + int error= ER_FOREIGN_SERVER_DOESNT_EXIST; FOREIGN_SERVER *server; DBUG_ENTER("delete_server_record_in_cache"); @@ -677,7 +630,7 @@ int delete_server_record_in_cache(LEX_SERVER_OPTIONS *server_options) 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? + goto end; } /* We succeded in deletion of the server to the table, now delete @@ -687,14 +640,15 @@ int delete_server_record_in_cache(LEX_SERVER_OPTIONS *server_options) server->server_name, server->server_name_length)); - if (server) - VOID(hash_delete(&servers_cache, (byte*) server)); - - servers_version++; /* servers updated */ + VOID(hash_delete(&servers_cache, (byte*) server)); + + error= 0; +end: DBUG_RETURN(error); } + /* SYNOPSIS @@ -714,6 +668,8 @@ int delete_server_record_in_cache(LEX_SERVER_OPTIONS *server_options) 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 @@ -722,7 +678,7 @@ int delete_server_record_in_cache(LEX_SERVER_OPTIONS *server_options) 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"); @@ -732,19 +688,26 @@ int update_server(THD *thd, FOREIGN_SERVER *existing, FOREIGN_SERVER *altered) 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); + error= update_server_record_in_cache(existing, altered); + + /* + Perform a reload so we don't have a 'hole' in our mem_root + */ + servers_load(thd, &tables); end: - rw_unlock(&THR_LOCK_servers); DBUG_RETURN(error); } + /* SYNOPSIS @@ -761,6 +724,8 @@ end: 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 @@ -791,13 +756,13 @@ int update_server_record_in_cache(FOREIGN_SERVER *existing, { DBUG_PRINT("info", ("had a problem inserting server %s at %lx", altered->server_name, (long unsigned int) altered)); - error= 1; + error= ER_OUT_OF_RESOURCES; } - servers_version++; /* servers updated */ DBUG_RETURN(error); } + /* SYNOPSIS @@ -830,9 +795,9 @@ void merge_server_struct(FOREIGN_SERVER *from, FOREIGN_SERVER *to) to->password= strdup_root(&mem, from->password); if (to->port == -1) to->port= from->port; - if (!to->socket) + if (!to->socket && from->socket) to->socket= strdup_root(&mem, from->socket); - if (!to->scheme) + if (!to->scheme && from->scheme) to->scheme= strdup_root(&mem, from->scheme); if (!to->owner) to->owner= strdup_root(&mem, from->owner); @@ -840,6 +805,7 @@ void merge_server_struct(FOREIGN_SERVER *from, FOREIGN_SERVER *to) DBUG_VOID_RETURN; } + /* SYNOPSIS @@ -862,7 +828,9 @@ void merge_server_struct(FOREIGN_SERVER *from, FOREIGN_SERVER *to) */ -int update_server_record(TABLE *table, FOREIGN_SERVER *server) + +static int +update_server_record(TABLE *table, FOREIGN_SERVER *server) { int error=0; DBUG_ENTER("update_server_record"); @@ -878,10 +846,7 @@ int update_server_record(TABLE *table, FOREIGN_SERVER *server) HA_READ_KEY_EXACT))) { if (error != HA_ERR_KEY_NOT_FOUND && error != HA_ERR_END_OF_FILE) - { table->file->print_error(error, MYF(0)); - error= 1; - } DBUG_PRINT("info",("server not found!")); error= ER_FOREIGN_SERVER_DOESNT_EXIST; } @@ -901,6 +866,7 @@ end: DBUG_RETURN(error); } + /* SYNOPSIS @@ -916,11 +882,11 @@ end: */ -int delete_server_record(TABLE *table, - char *server_name, - int server_name_length) +static int +delete_server_record(TABLE *table, + char *server_name, int server_name_length) { - int error= 0; + int error; DBUG_ENTER("delete_server_record"); table->use_all_columns(); @@ -933,10 +899,7 @@ int delete_server_record(TABLE *table, HA_READ_KEY_EXACT))) { if (error != HA_ERR_KEY_NOT_FOUND && error != HA_ERR_END_OF_FILE) - { table->file->print_error(error, MYF(0)); - error= 1; - } DBUG_PRINT("info",("server not found!")); error= ER_FOREIGN_SERVER_DOESNT_EXIST; } @@ -965,28 +928,35 @@ int delete_server_record(TABLE *table, 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; + prepare_server_struct_for_insert(server_options, server); - if ((error= insert_server(thd, server))) - goto end; + error= insert_server(thd, server); DBUG_PRINT("info", ("error returned %d", error)); end: + rw_unlock(&THR_LOCK_servers); DBUG_RETURN(error); } + /* SYNOPSIS @@ -1003,37 +973,33 @@ end: 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; - } - - if ((error= prepare_server_struct_for_update(server_options, existing, altered))) goto end; - if ((error= update_server(thd, existing, altered))) - goto end; + altered= (FOREIGN_SERVER *)alloc_root(&mem, + sizeof(FOREIGN_SERVER)); + + prepare_server_struct_for_update(server_options, existing, altered); + + error= update_server(thd, existing, altered); end: DBUG_PRINT("info", ("error returned %d", error)); - VOID(pthread_mutex_unlock(&servers_cache_mutex)); + rw_unlock(&THR_LOCK_servers); DBUG_RETURN(error); } + /* SYNOPSIS @@ -1044,19 +1010,17 @@ end: NOTES RETURN VALUE - 0 - no error + none */ -int prepare_server_struct_for_insert(LEX_SERVER_OPTIONS *server_options, - FOREIGN_SERVER *server) +static void +prepare_server_struct_for_insert(LEX_SERVER_OPTIONS *server_options, + FOREIGN_SERVER *server) { - int error; char *unset_ptr= (char*)""; DBUG_ENTER("prepare_server_struct"); - error= 0; - /* these two MUST be set */ server->server_name= strdup_root(&mem, server_options->server_name); server->server_name_length= server_options->server_name_length; @@ -1086,7 +1050,7 @@ int prepare_server_struct_for_insert(LEX_SERVER_OPTIONS *server_options, server->owner= server_options->owner ? strdup_root(&mem, server_options->owner) : unset_ptr; - DBUG_RETURN(error); + DBUG_VOID_RETURN; } /* @@ -1102,13 +1066,12 @@ int prepare_server_struct_for_insert(LEX_SERVER_OPTIONS *server_options, */ -int prepare_server_struct_for_update(LEX_SERVER_OPTIONS *server_options, - FOREIGN_SERVER *existing, - FOREIGN_SERVER *altered) +static void +prepare_server_struct_for_update(LEX_SERVER_OPTIONS *server_options, + FOREIGN_SERVER *existing, + FOREIGN_SERVER *altered) { - int error; DBUG_ENTER("prepare_server_struct_for_update"); - error= 0; altered->server_name= strdup_root(&mem, server_options->server_name); altered->server_name_length= server_options->server_name_length; @@ -1159,7 +1122,7 @@ int prepare_server_struct_for_update(LEX_SERVER_OPTIONS *server_options, (strcmp(server_options->owner, existing->owner))) ? strdup_root(&mem, server_options->owner) : 0; - DBUG_RETURN(error); + DBUG_VOID_RETURN; } /* @@ -1178,17 +1141,15 @@ int prepare_server_struct_for_update(LEX_SERVER_OPTIONS *server_options, void servers_free(bool end) { DBUG_ENTER("servers_free"); - if (!servers_cache_initialised) + if (!hash_inited(&servers_cache)) DBUG_VOID_RETURN; - VOID(pthread_mutex_destroy(&servers_cache_mutex)); - servers_cache_initialised=0; + rwlock_destroy(&THR_LOCK_servers); free_root(&mem,MYF(0)); hash_free(&servers_cache); DBUG_VOID_RETURN; } - /* SYNOPSIS @@ -1220,7 +1181,7 @@ FOREIGN_SERVER *get_server_by_name(const char *server_name) } 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))) @@ -1230,7 +1191,7 @@ FOREIGN_SERVER *get_server_by_name(const char *server_name) 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); } diff --git a/sql/sql_servers.h b/sql/sql_servers.h index 23b8cefd5bb..9618374dcab 100644 --- a/sql/sql_servers.h +++ b/sql/sql_servers.h @@ -25,40 +25,18 @@ typedef struct st_federated_server } FOREIGN_SERVER; /* cache handlers */ -my_bool servers_init(bool dont_read_server_table); -my_bool servers_reload(THD *thd); -my_bool get_server_from_table_to_cache(TABLE *table); +bool servers_init(bool dont_read_server_table); +bool servers_reload(THD *thd); void servers_free(bool end=0); /* insert functions */ int create_server(THD *thd, LEX_SERVER_OPTIONS *server_options); -int insert_server(THD *thd, FOREIGN_SERVER *server_options); -int insert_server_record(TABLE *table, FOREIGN_SERVER *server); -int insert_server_record_into_cache(FOREIGN_SERVER *server); -void store_server_fields_for_insert(TABLE *table, FOREIGN_SERVER *server); -void store_server_fields_for_insert(TABLE *table, - FOREIGN_SERVER *existing, - FOREIGN_SERVER *altered); -int prepare_server_struct_for_insert(LEX_SERVER_OPTIONS *server_options, - FOREIGN_SERVER *server); /* drop functions */ int drop_server(THD *thd, LEX_SERVER_OPTIONS *server_options); -int delete_server_record(TABLE *table, - char *server_name, - int server_name_length); -int delete_server_record_in_cache(LEX_SERVER_OPTIONS *server_options); /* update functions */ int alter_server(THD *thd, LEX_SERVER_OPTIONS *server_options); -int prepare_server_struct_for_update(LEX_SERVER_OPTIONS *server_options, - FOREIGN_SERVER *existing, - FOREIGN_SERVER *altered); -int update_server(THD *thd, FOREIGN_SERVER *existing, FOREIGN_SERVER *altered); -int update_server_record(TABLE *table, FOREIGN_SERVER *server); -int update_server_record_in_cache(FOREIGN_SERVER *existing, - FOREIGN_SERVER *altered); -/* utility functions */ -void merge_server_struct(FOREIGN_SERVER *from, FOREIGN_SERVER *to); + +/* lookup functions */ FOREIGN_SERVER *get_server_by_name(const char *server_name); -my_bool server_exists_in_table(THD *thd, char *server_name); From 99c5c28c3cc9eb417f4ca12f606c5abcf6b02a19 Mon Sep 17 00:00:00 2001 From: unknown Date: Sat, 24 Mar 2007 01:18:19 -0700 Subject: [PATCH 09/12] BUG#26257 New Federated Server Functionality Doesn't support differently named tables * Modified Federated memory allocation to use MEM_ROOT * Modified sql_servers and federated to allocate share connection parameters to use MEM_ROOT * Modified Federated to allow tablename in addition to server name * Implicit flushing of tables using altered/dropped server name * Added tests to prove new functionality works Contributors to this patch: Patrick Galbraith, Antony Curtis mysql-test/r/federated_server.result: BUG #26257 New Federated Server Functionality Doesn't support differently named tables New test results mysql-test/t/federated_server.test: BUG #26257 New Federated Server Functionality Doesn't support differently named tables New test which ensures that one can use the new 'create server' functionality and have tables point to the correct table, using CONNECTION='server', CONNECTION="server/tablename" and CONNECTION="mysql://...url" sql/mysql_priv.h: BUG #26257 New Federated Server Functionality Doesn't support differently named tables new function: close_cached_connection_tables() sql/sql_base.cc: BUG #26257 New Federated Server Functionality Doesn't support differently named tables new function: close_cached_connection_tables() closes all open tables which match connection string provides functionality to allow flushing of altered/dropped server names. sql/sql_servers.cc: BUG #26257 New Federated Server Functionality Doesn't support differently named tables * Added function clone_server() to allocate a new server for use by get_server_by_name() when creating a federated table * Now using MEM_ROOT allocation (mark and sweep) to account for meta data parameters being allocated properly, particularly with regards to to SERVER object. Also cleans up code allocating share. * Tables using the old definition of server name are now flushed on successful execution of ALTER/DROP SERVER. style: fixed some line-wrapping sql/sql_servers.h: BUG #26257 New Federated Server Functionality Doesn't support differently named tables * change in prototype to get_server_by_name() caller can now provide mem_root which strings will be copied in to. storage/federated/ha_federated.cc: BUG #26257 New Federated Server Functionality Doesn't support differently named tables * Simplified share and share member memory allocaton to use MEM_ROOT * Modified parse_url to parse table names along with server names storage/federated/ha_federated.h: BUG #26257 New Federated Server Functionality Doesn't support differently named tables * Added MEM_ROOT share member --- mysql-test/r/federated_server.result | 85 +++++++-- mysql-test/t/federated_server.test | 62 ++++++- sql/mysql_priv.h | 3 + sql/sql_base.cc | 66 +++++++ sql/sql_servers.cc | 110 ++++++++++-- sql/sql_servers.h | 3 +- storage/federated/ha_federated.cc | 258 ++++++++++++++++----------- storage/federated/ha_federated.h | 3 + 8 files changed, 458 insertions(+), 132 deletions(-) diff --git a/mysql-test/r/federated_server.result b/mysql-test/r/federated_server.result index 8faf33581d1..a3e7cd793a6 100644 --- a/mysql-test/r/federated_server.result +++ b/mysql-test/r/federated_server.result @@ -20,6 +20,14 @@ CREATE TABLE first_db.t1 ( `name` varchar(64) NOT NULL default '' ) DEFAULT CHARSET=latin1; +DROP TABLE IF EXISTS first_db.t2; +Warnings: +Note 1051 Unknown table 't2' +CREATE TABLE first_db.t2 ( +`id` int(20) NOT NULL, +`name` varchar(64) NOT NULL default '' + ) +DEFAULT CHARSET=latin1; use second_db; DROP TABLE IF EXISTS second_db.t1; Warnings: @@ -29,6 +37,14 @@ CREATE TABLE second_db.t1 ( `name` varchar(64) NOT NULL default '' ) DEFAULT CHARSET=latin1; +DROP TABLE IF EXISTS second_db.t2; +Warnings: +Note 1051 Unknown table 't2' +CREATE TABLE second_db.t2 ( +`id` int(20) NOT NULL, +`name` varchar(64) NOT NULL default '' + ) +DEFAULT CHARSET=latin1; drop server if exists 'server_one'; create server 'server_one' foreign data wrapper 'mysql' options (HOST '127.0.0.1', @@ -60,10 +76,10 @@ CREATE TABLE federated.old ( ) ENGINE="FEDERATED" DEFAULT CHARSET=latin1 CONNECTION='mysql://root@127.0.0.1:SLAVE_PORT/first_db/t1'; -INSERT INTO federated.old (id, name) values (1, 'federated.old url'); +INSERT INTO federated.old (id, name) values (1, 'federated.old-> first_db.t1, url format'); SELECT * FROM federated.old; id name -1 federated.old url +1 federated.old-> first_db.t1, url format DROP TABLE IF EXISTS federated.old2; Warnings: Note 1051 Unknown table 'old2' @@ -72,8 +88,37 @@ CREATE TABLE federated.old2 ( `name` varchar(64) NOT NULL default '' ) ENGINE="FEDERATED" DEFAULT CHARSET=latin1 +CONNECTION='mysql://root@127.0.0.1:SLAVE_PORT/first_db/t2'; +INSERT INTO federated.old2 (id, name) values (1, 'federated.old2-> first_db.t2, url format'); +SELECT * FROM federated.old2; +id name +1 federated.old2-> first_db.t2, url format +DROP TABLE IF EXISTS federated.urldb2t1; +Warnings: +Note 1051 Unknown table 'urldb2t1' +CREATE TABLE federated.urldb2t1 ( +`id` int(20) NOT NULL, +`name` varchar(64) NOT NULL default '' + ) +ENGINE="FEDERATED" DEFAULT CHARSET=latin1 CONNECTION='mysql://root@127.0.0.1:SLAVE_PORT/second_db/t1'; -INSERT INTO federated.old2 (id, name) values (1, 'federated.old2 url'); +INSERT INTO federated.urldb2t1 (id, name) values (1, 'federated.urldb2t1 -> second_db.t1, url format'); +SELECT * FROM federated.urldb2t1; +id name +1 federated.urldb2t1 -> second_db.t1, url format +DROP TABLE IF EXISTS federated.urldb2t2; +Warnings: +Note 1051 Unknown table 'urldb2t2' +CREATE TABLE federated.urldb2t2 ( +`id` int(20) NOT NULL, +`name` varchar(64) NOT NULL default '' + ) +ENGINE="FEDERATED" DEFAULT CHARSET=latin1 +CONNECTION='mysql://root@127.0.0.1:SLAVE_PORT/second_db/t2'; +INSERT INTO federated.urldb2t2 (id, name) values (1, 'federated.urldb2t2 -> second_db.t2, url format'); +SELECT * FROM federated.urldb2t2; +id name +1 federated.urldb2t2 -> second_db.t2, url format DROP TABLE IF EXISTS federated.t1; Warnings: Note 1051 Unknown table 't1' @@ -83,18 +128,38 @@ CREATE TABLE federated.t1 ( ) ENGINE="FEDERATED" DEFAULT CHARSET=latin1 CONNECTION='server_one'; -INSERT INTO federated.t1 (id, name) values (1, 'server_one, new scheme'); +INSERT INTO federated.t1 (id, name) values (1, 'server_one, new scheme, first_db.t1'); SELECT * FROM federated.t1; id name -1 federated.old url -1 server_one, new scheme +1 federated.old-> first_db.t1, url format +1 server_one, new scheme, first_db.t1 +DROP TABLE IF EXISTS federated.whatever; +Warnings: +Note 1051 Unknown table 'whatever' +CREATE TABLE federated.whatever ( +`id` int(20) NOT NULL, +`name` varchar(64) NOT NULL default '' + ) +ENGINE="FEDERATED" DEFAULT CHARSET=latin1 +CONNECTION='server_one/t1'; +INSERT INTO federated.whatever (id, name) values (1, 'server_one, new scheme, whatever, first_db.t1'); +SELECT * FROM federated.whatever; +id name +1 federated.old-> first_db.t1, url format +1 server_one, new scheme, first_db.t1 +1 server_one, new scheme, whatever, first_db.t1 ALTER SERVER 'server_one' options(DATABASE 'second_db'); -flush tables; -INSERT INTO federated.t1 (id, name) values (1, 'server_two, new scheme'); +INSERT INTO federated.t1 (id, name) values (1, 'server_two, new scheme, second_db.t1'); SELECT * FROM federated.t1; id name -1 federated.old2 url -1 server_two, new scheme +1 federated.urldb2t1 -> second_db.t1, url format +1 server_two, new scheme, second_db.t1 +INSERT INTO federated.whatever (id, name) values (1, 'server_two, new scheme, whatever, second_db.t1'); +SELECT * FROM federated.whatever; +id name +1 federated.urldb2t1 -> second_db.t1, url format +1 server_two, new scheme, second_db.t1 +1 server_two, new scheme, whatever, second_db.t1 drop table federated.t1; drop server 'server_one'; drop server 'server_two'; diff --git a/mysql-test/t/federated_server.test b/mysql-test/t/federated_server.test index 27dd494e5c5..b2075b8e262 100644 --- a/mysql-test/t/federated_server.test +++ b/mysql-test/t/federated_server.test @@ -17,6 +17,13 @@ CREATE TABLE first_db.t1 ( ) DEFAULT CHARSET=latin1; +DROP TABLE IF EXISTS first_db.t2; +CREATE TABLE first_db.t2 ( + `id` int(20) NOT NULL, + `name` varchar(64) NOT NULL default '' + ) + DEFAULT CHARSET=latin1; + use second_db; DROP TABLE IF EXISTS second_db.t1; CREATE TABLE second_db.t1 ( @@ -25,6 +32,13 @@ CREATE TABLE second_db.t1 ( ) DEFAULT CHARSET=latin1; +DROP TABLE IF EXISTS second_db.t2; +CREATE TABLE second_db.t2 ( + `id` int(20) NOT NULL, + `name` varchar(64) NOT NULL default '' + ) + DEFAULT CHARSET=latin1; + connection master; drop server if exists 'server_one'; @@ -61,7 +75,7 @@ eval CREATE TABLE federated.old ( ENGINE="FEDERATED" DEFAULT CHARSET=latin1 CONNECTION='mysql://root@127.0.0.1:$SLAVE_MYPORT/first_db/t1'; -INSERT INTO federated.old (id, name) values (1, 'federated.old url'); +INSERT INTO federated.old (id, name) values (1, 'federated.old-> first_db.t1, url format'); SELECT * FROM federated.old; @@ -72,9 +86,32 @@ eval CREATE TABLE federated.old2 ( `name` varchar(64) NOT NULL default '' ) ENGINE="FEDERATED" DEFAULT CHARSET=latin1 - CONNECTION='mysql://root@127.0.0.1:$SLAVE_MYPORT/second_db/t1'; + CONNECTION='mysql://root@127.0.0.1:$SLAVE_MYPORT/first_db/t2'; -INSERT INTO federated.old2 (id, name) values (1, 'federated.old2 url'); +INSERT INTO federated.old2 (id, name) values (1, 'federated.old2-> first_db.t2, url format'); +SELECT * FROM federated.old2; + +DROP TABLE IF EXISTS federated.urldb2t1; +--replace_result $SLAVE_MYPORT SLAVE_PORT +eval CREATE TABLE federated.urldb2t1 ( + `id` int(20) NOT NULL, + `name` varchar(64) NOT NULL default '' + ) + ENGINE="FEDERATED" DEFAULT CHARSET=latin1 + CONNECTION='mysql://root@127.0.0.1:$SLAVE_MYPORT/second_db/t1'; +INSERT INTO federated.urldb2t1 (id, name) values (1, 'federated.urldb2t1 -> second_db.t1, url format'); +SELECT * FROM federated.urldb2t1; + +DROP TABLE IF EXISTS federated.urldb2t2; +--replace_result $SLAVE_MYPORT SLAVE_PORT +eval CREATE TABLE federated.urldb2t2 ( + `id` int(20) NOT NULL, + `name` varchar(64) NOT NULL default '' + ) + ENGINE="FEDERATED" DEFAULT CHARSET=latin1 + CONNECTION='mysql://root@127.0.0.1:$SLAVE_MYPORT/second_db/t2'; +INSERT INTO federated.urldb2t2 (id, name) values (1, 'federated.urldb2t2 -> second_db.t2, url format'); +SELECT * FROM federated.urldb2t2; DROP TABLE IF EXISTS federated.t1; CREATE TABLE federated.t1 ( @@ -84,17 +121,30 @@ CREATE TABLE federated.t1 ( ENGINE="FEDERATED" DEFAULT CHARSET=latin1 CONNECTION='server_one'; -INSERT INTO federated.t1 (id, name) values (1, 'server_one, new scheme'); +INSERT INTO federated.t1 (id, name) values (1, 'server_one, new scheme, first_db.t1'); SELECT * FROM federated.t1; +DROP TABLE IF EXISTS federated.whatever; +CREATE TABLE federated.whatever ( + `id` int(20) NOT NULL, + `name` varchar(64) NOT NULL default '' + ) + ENGINE="FEDERATED" DEFAULT CHARSET=latin1 + CONNECTION='server_one/t1'; +INSERT INTO federated.whatever (id, name) values (1, 'server_one, new scheme, whatever, first_db.t1'); +SELECT * FROM federated.whatever; + ALTER SERVER 'server_one' options(DATABASE 'second_db'); -flush tables; +# FLUSH TABLES is now unneccessary -INSERT INTO federated.t1 (id, name) values (1, 'server_two, new scheme'); +INSERT INTO federated.t1 (id, name) values (1, 'server_two, new scheme, second_db.t1'); SELECT * FROM federated.t1; +INSERT INTO federated.whatever (id, name) values (1, 'server_two, new scheme, whatever, second_db.t1'); +SELECT * FROM federated.whatever; + drop table federated.t1; drop server 'server_one'; diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h index efcbdf968bf..2b1b310e67e 100644 --- a/sql/mysql_priv.h +++ b/sql/mysql_priv.h @@ -1430,6 +1430,9 @@ void close_open_tables_and_downgrade(ALTER_PARTITION_PARAM_TYPE *lpt); void mysql_wait_completed_table(ALTER_PARTITION_PARAM_TYPE *lpt, TABLE *my_table); bool close_cached_tables(THD *thd, bool wait_for_refresh, TABLE_LIST *tables, bool have_lock = FALSE); +bool close_cached_connection_tables(THD *thd, bool wait_for_refresh, + LEX_STRING *connect_string, + bool have_lock = FALSE); void copy_field_from_tmp_record(Field *field,int offset); bool fill_record(THD *thd, Field **field, List &values, bool ignore_errors); diff --git a/sql/sql_base.cc b/sql/sql_base.cc index e764c498059..44becc77865 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -858,6 +858,7 @@ void free_io_cache(TABLE *table) DBUG_VOID_RETURN; } + /* Close all tables which aren't in use by any thread @@ -969,6 +970,71 @@ bool close_cached_tables(THD *thd, bool if_wait_for_refresh, } +/* + Close all tables which match specified connection string or + if specified string is NULL, then any table with a connection string. +*/ + +bool close_cached_connection_tables(THD *thd, bool if_wait_for_refresh, + LEX_STRING *connection, bool have_lock) +{ + uint idx; + TABLE_LIST tmp, *tables= NULL; + bool result= FALSE; + DBUG_ENTER("close_cached_connections"); + DBUG_ASSERT(thd); + + bzero(&tmp, sizeof(TABLE_LIST)); + + if (!have_lock) + VOID(pthread_mutex_lock(&LOCK_open)); + + for (idx= 0; idx < table_def_cache.records; idx++) + { + TABLE_SHARE *share= (TABLE_SHARE *) hash_element(&table_def_cache, idx); + + /* Ignore if table is not open or does not have a connect_string */ + if (!share->connect_string.length || !share->ref_count) + continue; + + /* Compare the connection string */ + if (connection && + (connection->length > share->connect_string.length || + (connection->length < share->connect_string.length && + (share->connect_string.str[connection->length] != '/' && + share->connect_string.str[connection->length] != '\\')) || + strncasecmp(connection->str, share->connect_string.str, + connection->length))) + continue; + + /* close_cached_tables() only uses these elements */ + tmp.db= share->db.str; + tmp.table_name= share->table_name.str; + tmp.next_local= tables; + + tables= (TABLE_LIST *) memdup_root(thd->mem_root, (char*)&tmp, + sizeof(TABLE_LIST)); + } + + if (tables) + result= close_cached_tables(thd, FALSE, tables, TRUE); + + if (!have_lock) + VOID(pthread_mutex_unlock(&LOCK_open)); + + if (if_wait_for_refresh) + { + pthread_mutex_lock(&thd->mysys_var->mutex); + thd->mysys_var->current_mutex= 0; + thd->mysys_var->current_cond= 0; + thd->proc_info=0; + pthread_mutex_unlock(&thd->mysys_var->mutex); + } + + DBUG_RETURN(result); +} + + /* Mark all tables in the list which were used by current substatement as free for reuse. diff --git a/sql/sql_servers.cc b/sql/sql_servers.cc index 9b01a1b6889..71e141b0514 100644 --- a/sql/sql_servers.cc +++ b/sql/sql_servers.cc @@ -16,6 +16,21 @@ /* The servers are saved in the system table "servers" + + Currently, when the user performs an ALTER SERVER or a DROP SERVER + operation, it will cause all open tables which refer to the named + server connection to be flushed. This may cause some undesirable + behaviour with regard to currently running transactions. It is + expected that the DBA knows what s/he is doing when s/he performs + the ALTER SERVER or DROP SERVER operation. + + TODO: + It is desirable for us to implement a callback mechanism instead where + callbacks can be registered for specific server protocols. The callback + will be fired when such a server name has been created/altered/dropped + or when statistics are to be gathered such as how many actual connections. + Storage engines etc will be able to make use of the callback so that + currently running transactions etc will not be disrupted. */ #include "mysql_priv.h" @@ -557,6 +572,8 @@ int drop_server(THD *thd, LEX_SERVER_OPTIONS *server_options) int error; TABLE_LIST tables; TABLE *table; + LEX_STRING name= { server_options->server_name, + server_options->server_name_length }; DBUG_ENTER("drop_server"); DBUG_PRINT("info", ("server name server->server_name %s", @@ -578,14 +595,16 @@ int drop_server(THD *thd, LEX_SERVER_OPTIONS *server_options) goto end; } - error= delete_server_record(table, - server_options->server_name, - server_options->server_name_length); + error= delete_server_record(table, name.str, name.length); - /* - Perform a reload so we don't have a 'hole' in our mem_root - */ - servers_load(thd, &tables); + /* close the servers table before we call closed_cached_connection_tables */ + close_thread_tables(thd); + + if (close_cached_connection_tables(thd, TRUE, &name)) + { + push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_WARN, + ER_UNKNOWN_ERROR, "Server connection in use"); + } end: rw_unlock(&THR_LOCK_servers); @@ -975,6 +994,8 @@ int alter_server(THD *thd, LEX_SERVER_OPTIONS *server_options) { int error= ER_FOREIGN_SERVER_DOESNT_EXIST; FOREIGN_SERVER *altered, *existing; + LEX_STRING name= { server_options->server_name, + server_options->server_name_length }; DBUG_ENTER("alter_server"); DBUG_PRINT("info", ("server_options->server_name %s", server_options->server_name)); @@ -982,8 +1003,8 @@ int alter_server(THD *thd, LEX_SERVER_OPTIONS *server_options) rw_wrlock(&THR_LOCK_servers); if (!(existing= (FOREIGN_SERVER *) hash_search(&servers_cache, - (byte*) server_options->server_name, - server_options->server_name_length))) + (byte*) name.str, + name.length))) goto end; altered= (FOREIGN_SERVER *)alloc_root(&mem, @@ -993,6 +1014,15 @@ int alter_server(THD *thd, LEX_SERVER_OPTIONS *server_options) error= update_server(thd, existing, altered); + /* close the servers table before we call closed_cached_connection_tables */ + close_thread_tables(thd); + + if (close_cached_connection_tables(thd, FALSE, &name)) + { + push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_WARN, + ER_UNKNOWN_ERROR, "Server connection in use"); + } + end: DBUG_PRINT("info", ("error returned %d", error)); rw_unlock(&THR_LOCK_servers); @@ -1143,6 +1173,12 @@ void servers_free(bool end) DBUG_ENTER("servers_free"); if (!hash_inited(&servers_cache)) DBUG_VOID_RETURN; + if (!end) + { + free_root(&mem, MYF(MY_MARK_BLOCKS_FREE)); + my_hash_reset(&servers_cache); + DBUG_VOID_RETURN; + } rwlock_destroy(&THR_LOCK_servers); free_root(&mem,MYF(0)); hash_free(&servers_cache); @@ -1150,6 +1186,51 @@ void servers_free(bool end) } +/* + SYNOPSIS + + clone_server(MEM_ROOT *mem_root, FOREIGN_SERVER *orig, FOREIGN_SERVER *buff) + + Create a clone of FOREIGN_SERVER. If the supplied mem_root is of + thd->mem_root then the copy is automatically disposed at end of statement. + + NOTES + + ARGS + MEM_ROOT pointer (strings are copied into this mem root) + FOREIGN_SERVER pointer (made a copy of) + FOREIGN_SERVER buffer (if not-NULL, this pointer is returned) + + RETURN VALUE + FOREIGN_SEVER pointer (copy of one supplied FOREIGN_SERVER) +*/ + +static FOREIGN_SERVER *clone_server(MEM_ROOT *mem, const FOREIGN_SERVER *server, + FOREIGN_SERVER *buffer) +{ + DBUG_ENTER("sql_server.cc:clone_server"); + + if (!buffer) + buffer= (FOREIGN_SERVER *) alloc_root(mem, sizeof(FOREIGN_SERVER)); + + buffer->server_name= strmake_root(mem, server->server_name, + server->server_name_length); + buffer->port= server->port; + buffer->server_name_length= server->server_name_length; + + /* TODO: We need to examine which of these can really be NULL */ + buffer->db= server->db ? strdup_root(mem, server->db) : NULL; + buffer->scheme= server->scheme ? strdup_root(mem, server->scheme) : NULL; + buffer->username= server->username? strdup_root(mem, server->username): NULL; + buffer->password= server->password? strdup_root(mem, server->password): NULL; + buffer->socket= server->socket ? strdup_root(mem, server->socket) : NULL; + buffer->owner= server->owner ? strdup_root(mem, server->owner) : NULL; + buffer->host= server->host ? strdup_root(mem, server->host) : NULL; + + DBUG_RETURN(buffer); +} + + /* SYNOPSIS @@ -1163,11 +1244,11 @@ void servers_free(bool end) */ -FOREIGN_SERVER *get_server_by_name(const char *server_name) +FOREIGN_SERVER *get_server_by_name(MEM_ROOT *mem, const char *server_name, + FOREIGN_SERVER *buff) { - ulong error_num=0; uint server_name_length; - FOREIGN_SERVER *server= 0; + FOREIGN_SERVER *server; DBUG_ENTER("get_server_by_name"); DBUG_PRINT("info", ("server_name %s", server_name)); @@ -1176,7 +1257,6 @@ FOREIGN_SERVER *get_server_by_name(const char *server_name) if (! server_name || !strlen(server_name)) { DBUG_PRINT("info", ("server_name not defined!")); - error_num= 1; DBUG_RETURN((FOREIGN_SERVER *)NULL); } @@ -1190,6 +1270,10 @@ FOREIGN_SERVER *get_server_by_name(const char *server_name) server_name, server_name_length)); server= (FOREIGN_SERVER *) NULL; } + /* otherwise, make copy of server */ + else + server= clone_server(mem, server, buff); + DBUG_PRINT("info", ("unlocking servers_cache")); rw_unlock(&THR_LOCK_servers); DBUG_RETURN(server); diff --git a/sql/sql_servers.h b/sql/sql_servers.h index 9618374dcab..63c691893d1 100644 --- a/sql/sql_servers.h +++ b/sql/sql_servers.h @@ -39,4 +39,5 @@ int drop_server(THD *thd, LEX_SERVER_OPTIONS *server_options); int alter_server(THD *thd, LEX_SERVER_OPTIONS *server_options); /* lookup functions */ -FOREIGN_SERVER *get_server_by_name(const char *server_name); +FOREIGN_SERVER *get_server_by_name(MEM_ROOT *mem, const char *server_name, + FOREIGN_SERVER *server_buffer); diff --git a/storage/federated/ha_federated.cc b/storage/federated/ha_federated.cc index 14ffe5da984..e60dc47f48c 100644 --- a/storage/federated/ha_federated.cc +++ b/storage/federated/ha_federated.cc @@ -43,23 +43,55 @@ The create table will simply create the .frm file, and within the "CREATE TABLE" SQL, there SHALL be any of the following : - comment=scheme://username:password@hostname:port/database/tablename - comment=scheme://username@hostname/database/tablename - comment=scheme://username:password@hostname/database/tablename - comment=scheme://username:password@hostname/database/tablename + connection=scheme://username:password@hostname:port/database/tablename + connection=scheme://username@hostname/database/tablename + connection=scheme://username:password@hostname/database/tablename + connection=scheme://username:password@hostname/database/tablename + + - OR - + + As of 5.1 (See worklog #3031), federated now allows you to use a non-url + format, taking advantage of mysql.servers: + + connection="connection_one" + connection="connection_one/table_foo" An example would be: - comment=mysql://username:password@hostname:port/database/tablename + connection=mysql://username:password@hostname:port/database/tablename - ***IMPORTANT*** + or, if we had: - This is a first release, conceptual release - Only 'mysql://' is supported at this release. + create server 'server_one' foreign data wrapper 'mysql' options + (HOST '127.0.0.1', + DATABASE 'db1', + USER 'root', + PASSWORD '', + PORT 3306, + SOCKET '', + OWNER 'root'); + CREATE TABLE federated.t1 ( + `id` int(20) NOT NULL, + `name` varchar(64) NOT NULL default '' + ) + ENGINE="FEDERATED" DEFAULT CHARSET=latin1 + CONNECTION='server_one'; - This comment connection string is necessary for the handler to be - able to connect to the foreign server. + So, this will have been the equivalent of + + CONNECTION="mysql://root@127.0.0.1:3306/db1/t1" + + Then, we can also change the server to point to a new schema: + + ALTER SERVER 'server_one' options(DATABASE 'db2'); + + All subsequent calls will now be against db2.t1! Guess what? You don't + have to perform an alter table! + + This connecton="connection string" is necessary for the handler to be + able to connect to the foreign server, either by URL, or by server + name. The basic flow is this: @@ -166,7 +198,7 @@ KEY other_key (other)) ENGINE="FEDERATED" DEFAULT CHARSET=latin1 - COMMENT='root@127.0.0.1:9306/federated/test_federated'; + CONNECTION='mysql://root@127.0.0.1:9306/federated/test_federated'; Notice the "COMMENT" and "ENGINE" field? This is where you respectively set the engine type, "FEDERATED" and foreign @@ -263,7 +295,7 @@ To run these tests, go into ./mysql-test (based in the directory you built the server in) - ./mysql-test-run federatedd + ./mysql-test-run federated To run the test, or if you want to run the test and have debug info: @@ -311,7 +343,7 @@ ------------- These were the files that were modified or created for this - Federated handler to work: + Federated handler to work, in 5.0: ./configure.in ./sql/Makefile.am @@ -329,6 +361,13 @@ ./sql/ha_federated.cc ./sql/ha_federated.h + In 5.1 + + my:~/mysql-build/mysql-5.1-bkbits patg$ ls storage/federated/ + CMakeLists.txt Makefile.in ha_federated.h plug.in + Makefile SCCS libfederated.a + Makefile.am ha_federated.cc libfederated_a-ha_federated.o + */ @@ -547,42 +586,39 @@ static int parse_url_error(FEDERATED_SHARE *share, TABLE *table, int error_num) int buf_len; DBUG_ENTER("ha_federated parse_url_error"); - if (share->connection_string) - { - DBUG_PRINT("info", - ("error: parse_url. Returning error code %d \ - freeing share->connection_string %lx", - error_num, (long unsigned int) share->connection_string)); - my_free((gptr) share->connection_string, MYF(0)); - share->connection_string= 0; - } buf_len= min(table->s->connect_string.length, FEDERATED_QUERY_BUFFER_SIZE-1); strmake(buf, table->s->connect_string.str, buf_len); my_error(error_num, MYF(0), buf); DBUG_RETURN(error_num); } + /* retrieve server object which contains server meta-data from the system table given a server's name, set share connection parameter members */ -int get_connection(FEDERATED_SHARE *share) +int get_connection(MEM_ROOT *mem_root, FEDERATED_SHARE *share) { int error_num= ER_FOREIGN_SERVER_DOESNT_EXIST; char error_buffer[FEDERATED_QUERY_BUFFER_SIZE]; - FOREIGN_SERVER *server; + FOREIGN_SERVER *server, server_buffer; DBUG_ENTER("ha_federated::get_connection"); + /* + get_server_by_name() clones the server if exists and allocates + copies of strings in the supplied mem_root + */ if (!(server= - get_server_by_name(share->connection_string))) + get_server_by_name(mem_root, share->connection_string, &server_buffer))) { DBUG_PRINT("info", ("get_server_by_name returned > 0 error condition!")); /* need to come up with error handling */ error_num=1; goto error; } - DBUG_PRINT("info", ("get_server_by_name returned server at %lx", (long unsigned int) server)); + DBUG_PRINT("info", ("get_server_by_name returned server at %lx", + (long unsigned int) server)); /* Most of these should never be empty strings, error handling will @@ -591,29 +627,22 @@ int get_connection(FEDERATED_SHARE *share) except there are errors in the trace file of the share being overrun at the address of the share. */ - if (server->server_name) - share->server_name= server->server_name; - share->server_name_length= server->server_name_length ? - server->server_name_length : 0; - if (server->username) - share->username= server->username; - if (server->password) - share->password= server->password; - if (server->db) - share->database= server->db; - - share->port= server->port ? (ushort) server->port : MYSQL_PORT; - - if (server->host) - share->hostname= server->host; - if (server->socket) - share->socket= server->socket; - else if (strcmp(share->hostname, my_localhost) == 0) - share->socket= my_strdup(MYSQL_UNIX_ADDR, MYF(0)); - if (server->scheme) - share->scheme= server->scheme; - else - share->scheme= NULL; + share->server_name_length= server->server_name_length; + share->server_name= server->server_name; + share->username= server->username; + share->password= server->password; + share->database= server->db; +#ifndef I_AM_PARANOID + share->port= server->port > 0 && server->port < 65536 ? +#else + share->port= server->port > 1023 && server->port < 65536 ? +#endif + (ushort) server->port : MYSQL_PORT; + share->hostname= server->host; + if (!(share->socket= server->socket) && + !strcmp(share->hostname, my_localhost)) + share->socket= (char *) MYSQL_UNIX_ADDR; + share->scheme= server->scheme; DBUG_PRINT("info", ("share->username %s", share->username)); DBUG_PRINT("info", ("share->password %s", share->password)); @@ -636,6 +665,7 @@ error: SYNOPSIS parse_url() + mem_root MEM_ROOT pointer for memory allocation share pointer to FEDERATED share table pointer to current TABLE class table_create_flag determines what error to throw @@ -685,7 +715,7 @@ error: */ -static int parse_url(FEDERATED_SHARE *share, TABLE *table, +static int parse_url(MEM_ROOT *mem_root, FEDERATED_SHARE *share, TABLE *table, uint table_create_flag) { uint error_num= (table_create_flag ? @@ -699,20 +729,19 @@ static int parse_url(FEDERATED_SHARE *share, TABLE *table, DBUG_PRINT("info", ("Length: %d", table->s->connect_string.length)); DBUG_PRINT("info", ("String: '%.*s'", table->s->connect_string.length, table->s->connect_string.str)); - share->connection_string= my_strndup(table->s->connect_string.str, - table->s->connect_string.length, - MYF(0)); + share->connection_string= strmake_root(mem_root, table->s->connect_string.str, + table->s->connect_string.length); - // Add a null for later termination of table name - share->connection_string[table->s->connect_string.length]= 0; DBUG_PRINT("info",("parse_url alloced share->connection_string %lx", (long unsigned int) share->connection_string)); DBUG_PRINT("info",("share->connection_string %s",share->connection_string)); - /* No delimiters, must be a straight connection name */ - if ( (!strchr(share->connection_string, '/')) && - (!strchr(share->connection_string, '@')) && - (!strchr(share->connection_string, ';'))) + /* + No :// or @ in connection string. Must be a straight connection name of + either "servername" or "servername/tablename" + */ + if ( (!strstr(share->connection_string, "://") && + (!strchr(share->connection_string, '@')))) { DBUG_PRINT("info", @@ -721,17 +750,51 @@ static int parse_url(FEDERATED_SHARE *share, TABLE *table, share->connection_string, (long unsigned int) share->connection_string)); + /* ok, so we do a little parsing, but not completely! */ share->parsed= FALSE; - if ((error_num= get_connection(share))) - goto error; - /* - connection specifies everything but, resort to - expecting remote and foreign table names to match + If there is a single '/' in the connection string, this means the user is + specifying a table name */ - share->table_name= table->s->table_name.str; - share->table_name_length= table->s->table_name.length; - share->table_name[share->table_name_length]= '\0'; + + if ((share->table_name= strchr(share->connection_string, '/'))) + { + share->connection_string[share->table_name - share->connection_string]= '\0'; + share->table_name++; + share->table_name_length= strlen(share->table_name); + + DBUG_PRINT("info", + ("internal format, parsed table_name share->connection_string \ + %s share->table_name %s", + share->connection_string, share->table_name)); + + /* + there better not be any more '/'s ! + */ + if (strchr(share->table_name, '/')) + goto error; + + } + /* + otherwise, straight server name, use tablename of federated table + as remote table name + */ + else + { + /* + connection specifies everything but, resort to + expecting remote and foreign table names to match + */ + share->table_name= strmake_root(mem_root, table->s->table_name.str, + (share->table_name_length= table->s->table_name.length)); + DBUG_PRINT("info", + ("internal format, default table_name share->connection_string \ + %s share->table_name %s", + share->connection_string, share->table_name)); + } + + if ((error_num= get_connection(mem_root, share))) + goto error; } else { @@ -817,7 +880,7 @@ Then password is a null string, so set to NULL if (!share->port) { if (strcmp(share->hostname, my_localhost) == 0) - share->socket= my_strdup(MYSQL_UNIX_ADDR, MYF(0)); + share->socket= (char *) MYSQL_UNIX_ADDR; else share->port= MYSQL_PORT; } @@ -1421,22 +1484,26 @@ err: static FEDERATED_SHARE *get_share(const char *table_name, TABLE *table) { - char *select_query; char query_buffer[FEDERATED_QUERY_BUFFER_SIZE]; Field **field; String query(query_buffer, sizeof(query_buffer), &my_charset_bin); FEDERATED_SHARE *share= NULL, tmp_share; + MEM_ROOT mem_root; + DBUG_ENTER("ha_federated.cc::get_share"); + /* In order to use this string, we must first zero it's length, or it will contain garbage */ query.length(0); + init_alloc_root(&mem_root, 256, 0); + pthread_mutex_lock(&federated_mutex); tmp_share.share_key= table_name; tmp_share.share_key_length= strlen(table_name); - if (parse_url(&tmp_share, table, 0)) + if (parse_url(&mem_root, &tmp_share, table, 0)) goto error; /* TODO: change tmp_share.scheme to LEX_STRING object */ @@ -1457,24 +1524,17 @@ static FEDERATED_SHARE *get_share(const char *table_name, TABLE *table) query.length(query.length() - sizeof_trailing_comma); query.append(STRING_WITH_LEN(" FROM `")); + query.append(tmp_share.table_name, tmp_share.table_name_length); + query.append(STRING_WITH_LEN("`")); + DBUG_PRINT("info", ("calling alloc_root")); - if (!(share= (FEDERATED_SHARE *) - my_multi_malloc(MYF(MY_WME), - &share, sizeof(*share), - &select_query, - query.length()+table->s->connect_string.length+1, - NullS))) + if (!(share= (FEDERATED_SHARE *) memdup_root(&mem_root, (char*)&tmp_share, sizeof(*share))) || + !(share->select_query= (char*) strmake_root(&mem_root, query.ptr(), query.length()))) goto error; - memcpy(share, &tmp_share, sizeof(tmp_share)); - - share->table_name_length= strlen(share->table_name); - /* TODO: share->table_name to LEX_STRING object */ - query.append(share->table_name, share->table_name_length); - query.append(STRING_WITH_LEN("`")); - share->select_query= select_query; - strmov(share->select_query, query.ptr()); share->use_count= 0; + share->mem_root= mem_root; + DBUG_PRINT("info", ("share->select_query %s", share->select_query)); @@ -1483,17 +1543,18 @@ static FEDERATED_SHARE *get_share(const char *table_name, TABLE *table) thr_lock_init(&share->lock); pthread_mutex_init(&share->mutex, MY_MUTEX_INIT_FAST); } + else + free_root(&mem_root, MYF(0)); /* prevents memory leak */ + share->use_count++; pthread_mutex_unlock(&federated_mutex); - return share; + DBUG_RETURN(share); error: pthread_mutex_unlock(&federated_mutex); - my_free((gptr) tmp_share.connection_string, MYF(MY_ALLOW_ZERO_PTR)); - tmp_share.connection_string= 0; - my_free((gptr) share, MYF(MY_ALLOW_ZERO_PTR)); - return NULL; + free_root(&mem_root, MYF(0)); + DBUG_RETURN(NULL); } @@ -1505,23 +1566,16 @@ error: static int free_share(FEDERATED_SHARE *share) { + MEM_ROOT mem_root= share->mem_root; DBUG_ENTER("free_share"); pthread_mutex_lock(&federated_mutex); if (!--share->use_count) { hash_delete(&federated_open_tables, (byte*) share); - if (share->parsed) - my_free((gptr) share->socket, MYF(MY_ALLOW_ZERO_PTR)); - /*if (share->connection_string) - { - */ - my_free((gptr) share->connection_string, MYF(MY_ALLOW_ZERO_PTR)); - share->connection_string= 0; - /*}*/ thr_lock_delete(&share->lock); VOID(pthread_mutex_destroy(&share->mutex)); - my_free((gptr) share, MYF(0)); + free_root(&mem_root, MYF(0)); } pthread_mutex_unlock(&federated_mutex); @@ -1590,6 +1644,8 @@ int ha_federated::open(const char *name, int mode, uint test_if_locked) mysql_options(mysql,MYSQL_SET_CHARSET_NAME, this->table->s->table_charset->csname); + DBUG_PRINT("info", ("calling mysql_real_connect hostname %s user %s", + share->hostname, share->username)); if (!mysql || !mysql_real_connect(mysql, share->hostname, share->username, @@ -2832,15 +2888,13 @@ int ha_federated::create(const char *name, TABLE *table_arg, HA_CREATE_INFO *create_info) { int retval; + THD *thd= current_thd; FEDERATED_SHARE tmp_share; // Only a temporary share, to test the url DBUG_ENTER("ha_federated::create"); - if (!(retval= parse_url(&tmp_share, table_arg, 1))) + if (!(retval= parse_url(thd->mem_root, &tmp_share, table_arg, 1))) retval= check_foreign_data_source(&tmp_share, 1); - /* free this because strdup created it in parse_url */ - my_free((gptr) tmp_share.connection_string, MYF(MY_ALLOW_ZERO_PTR)); - tmp_share.connection_string= 0; DBUG_RETURN(retval); } diff --git a/storage/federated/ha_federated.h b/storage/federated/ha_federated.h index bbc2b2fe9f8..4d2eefdd986 100644 --- a/storage/federated/ha_federated.h +++ b/storage/federated/ha_federated.h @@ -43,6 +43,8 @@ The example implements the minimum of what you will probably need. */ typedef struct st_federated_share { + MEM_ROOT mem_root; + bool parsed; /* this key is unique db/tablename */ const char *share_key; @@ -67,6 +69,7 @@ typedef struct st_federated_share { char *sport; int share_key_length; ushort port; + uint table_name_length, server_name_length, connect_string_length, use_count; pthread_mutex_t mutex; THR_LOCK lock; From fa82ebd2d650fe92119ac72169460b80da84e60f Mon Sep 17 00:00:00 2001 From: unknown Date: Sun, 25 Mar 2007 00:24:39 +0400 Subject: [PATCH 10/12] BUG#24566 - Incorrect key file for table ( the size of table is more than 2G) Accessing a file that is bigger than 2G may report that read/write operation failed. This may affect anything that uses my_pread/my_pwrite functions, e.g. MyISAM, ARCHIVE, binary log. For MyISAM INSERT may report that table is crashed when writing to a table that is bigger than 2G. This is fixed by using proper offset type in my_pread/my_pwrite functions on systems that do not have native pread/pwrite calls. Affects systems that do not have native pread/pwrite calls, e.g. Windows. No test case for this fix, since it requires huge table. mysys/my_pread.c: Use proper offset type for restoring position on systems that do not have native pread/pwrite calls. --- mysys/my_pread.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mysys/my_pread.c b/mysys/my_pread.c index 7a09c21e039..f8f0fa49c10 100644 --- a/mysys/my_pread.c +++ b/mysys/my_pread.c @@ -37,7 +37,7 @@ uint my_pread(File Filedes, byte *Buffer, uint Count, my_off_t offset, errno=0; /* Linux doesn't reset this */ #endif #ifndef HAVE_PREAD - off_t old_offset; + os_off_t old_offset; pthread_mutex_lock(&my_file_info[Filedes].mutex); /* @@ -45,7 +45,7 @@ uint my_pread(File Filedes, byte *Buffer, uint Count, my_off_t offset, before seeking to the given offset */ - error= (old_offset= (off_t)lseek(Filedes, 0L, MY_SEEK_CUR)) == -1L || + error= (old_offset= lseek(Filedes, 0L, MY_SEEK_CUR)) == -1L || lseek(Filedes, offset, MY_SEEK_SET) == -1L; if (!error) /* Seek was successful */ @@ -116,7 +116,7 @@ uint my_pwrite(int Filedes, const byte *Buffer, uint Count, my_off_t offset, { #ifndef HAVE_PREAD int error= 0; - off_t old_offset; + os_off_t old_offset; writenbytes= (uint) -1; pthread_mutex_lock(&my_file_info[Filedes].mutex); @@ -124,7 +124,7 @@ uint my_pwrite(int Filedes, const byte *Buffer, uint Count, my_off_t offset, As we cannot change the file pointer, we save the old position, before seeking to the given offset */ - error= ((old_offset= (off_t)lseek(Filedes, 0L, MY_SEEK_CUR)) == -1L || + error= ((old_offset= lseek(Filedes, 0L, MY_SEEK_CUR)) == -1L || lseek(Filedes, offset, MY_SEEK_SET) == -1L); if (!error) /* Seek was successful */ From 1fd0ba8909465f70c3b4e5d4dea4dc56b132896a Mon Sep 17 00:00:00 2001 From: unknown Date: Tue, 27 Mar 2007 10:49:48 +0200 Subject: [PATCH 11/12] Bug#24985 - UTF8 ENUM primary key on MEMORY using BTREE causes incorrect duplicate entries Keys for BTREE indexes on ENUM and SET columns of MEMORY tables with character set UTF8 were computed incorrectly. Many different column values got the same key value. Apart of possible performance problems, it made unique indexes of this type unusable because it rejected many different values as duplicates. The problem was that multibyte character detection was tried on the internal numeric column value. Many values were not identified as characters. Their key value became blank filled. Thanks to Alexander Barkov and Ramil Kalimullin for the patch, which sets the character set of ENUM and SET key segments to the pseudo binary character set. mysql-test/r/heap_btree.result: Bug#24985 - UTF8 ENUM primary key on MEMORY using BTREE causes incorrect duplicate entries Added test result. mysql-test/t/heap_btree.test: Bug#24985 - UTF8 ENUM primary key on MEMORY using BTREE causes incorrect duplicate entries Added test. sql/ha_heap.cc: Bug#24985 - UTF8 ENUM primary key on MEMORY using BTREE causes incorrect duplicate entries Set key segment charset to my_charset_bin for ENUM and SET columns. --- mysql-test/r/heap_btree.result | 12 ++++++++++++ mysql-test/t/heap_btree.test | 17 +++++++++++++++++ sql/ha_heap.cc | 5 ++++- 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/mysql-test/r/heap_btree.result b/mysql-test/r/heap_btree.result index e6492e90b80..512a8a52845 100644 --- a/mysql-test/r/heap_btree.result +++ b/mysql-test/r/heap_btree.result @@ -280,4 +280,16 @@ a 1 1 drop table t1; +CREATE TABLE t1 ( +c1 ENUM('1', '2'), +UNIQUE USING BTREE(c1) +) ENGINE= MEMORY DEFAULT CHARSET= utf8; +INSERT INTO t1 VALUES('1'), ('2'); +DROP TABLE t1; +CREATE TABLE t1 ( +c1 SET('1', '2'), +UNIQUE USING BTREE(c1) +) ENGINE= MEMORY DEFAULT CHARSET= utf8; +INSERT INTO t1 VALUES('1'), ('2'); +DROP TABLE t1; End of 4.1 tests diff --git a/mysql-test/t/heap_btree.test b/mysql-test/t/heap_btree.test index 9aa820becd9..eb4672473f6 100644 --- a/mysql-test/t/heap_btree.test +++ b/mysql-test/t/heap_btree.test @@ -182,4 +182,21 @@ delete from t1 where a >= 2; select a from t1 order by a; drop table t1; +# +# Bug#24985 - UTF8 ENUM primary key on MEMORY using BTREE +# causes incorrect duplicate entries +# +CREATE TABLE t1 ( + c1 ENUM('1', '2'), + UNIQUE USING BTREE(c1) +) ENGINE= MEMORY DEFAULT CHARSET= utf8; +INSERT INTO t1 VALUES('1'), ('2'); +DROP TABLE t1; +CREATE TABLE t1 ( + c1 SET('1', '2'), + UNIQUE USING BTREE(c1) +) ENGINE= MEMORY DEFAULT CHARSET= utf8; +INSERT INTO t1 VALUES('1'), ('2'); +DROP TABLE t1; + --echo End of 4.1 tests diff --git a/sql/ha_heap.cc b/sql/ha_heap.cc index 3e981087df7..dd3a84aaaee 100644 --- a/sql/ha_heap.cc +++ b/sql/ha_heap.cc @@ -549,7 +549,10 @@ int ha_heap::create(const char *name, TABLE *table_arg, seg->start= (uint) key_part->offset; seg->length= (uint) key_part->length; seg->flag = 0; - seg->charset= field->charset(); + if (field->flags & (ENUM_FLAG | SET_FLAG)) + seg->charset= &my_charset_bin; + else + seg->charset= field->charset(); if (field->null_ptr) { seg->null_bit= field->null_bit; From de3c37195691a19ac504dd60daf516219d59c503 Mon Sep 17 00:00:00 2001 From: unknown Date: Tue, 27 Mar 2007 12:39:31 +0200 Subject: [PATCH 12/12] Bug#24985 - UTF8 ENUM primary key on MEMORY using BTREE causes incorrect duplicate entries After merge fix --- mysql-test/t/heap_btree.test | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/mysql-test/t/heap_btree.test b/mysql-test/t/heap_btree.test index 14b1779bd1a..d2891943a4e 100644 --- a/mysql-test/t/heap_btree.test +++ b/mysql-test/t/heap_btree.test @@ -182,6 +182,23 @@ delete from t1 where a >= 2; select a from t1 order by a; drop table t1; +# +# Bug#24985 - UTF8 ENUM primary key on MEMORY using BTREE +# causes incorrect duplicate entries +# +CREATE TABLE t1 ( + c1 ENUM('1', '2'), + UNIQUE USING BTREE(c1) +) ENGINE= MEMORY DEFAULT CHARSET= utf8; +INSERT INTO t1 VALUES('1'), ('2'); +DROP TABLE t1; +CREATE TABLE t1 ( + c1 SET('1', '2'), + UNIQUE USING BTREE(c1) +) ENGINE= MEMORY DEFAULT CHARSET= utf8; +INSERT INTO t1 VALUES('1'), ('2'); +DROP TABLE t1; + --echo End of 4.1 tests # @@ -204,22 +221,4 @@ CREATE TABLE t1 (a INT, UNIQUE USING BTREE(a)) ENGINE=MEMORY; INSERT INTO t1 VALUES(NULL),(NULL); DROP TABLE t1; -# -# Bug#24985 - UTF8 ENUM primary key on MEMORY using BTREE -# causes incorrect duplicate entries -# -CREATE TABLE t1 ( - c1 ENUM('1', '2'), - UNIQUE USING BTREE(c1) -) ENGINE= MEMORY DEFAULT CHARSET= utf8; -INSERT INTO t1 VALUES('1'), ('2'); -DROP TABLE t1; -CREATE TABLE t1 ( - c1 SET('1', '2'), - UNIQUE USING BTREE(c1) -) ENGINE= MEMORY DEFAULT CHARSET= utf8; -INSERT INTO t1 VALUES('1'), ('2'); -DROP TABLE t1; - ---echo End of 4.1 tests --echo End of 5.0 tests