From 6dfb184f092617919f4399abbc25fbdc01d4ed2b Mon Sep 17 00:00:00 2001 From: "svoj@mysql.com/june.mysql.com" <> Date: Tue, 18 Mar 2008 20:25:34 +0400 Subject: [PATCH 1/5] BUG#34768 - nondeterministic INSERT using LIMIT logged in stmt mode if binlog_format=mixed Statement-based replication of DELETE ... LIMIT, UPDATE ... LIMIT, INSERT ... SELECT ... LIMIT is not safe as order of rows is not defined. With this fix, we issue a warning that this statement is not safe to replicate in statement mode, or go to row-based mode in mixed mode. Note that we may consider a statement as safe if ORDER BY primary_key is present. However it may confuse users to see very similiar statements replicated differently. Note 2: regular UPDATE statement (w/o LIMIT) is unsafe as well, but this patch doesn't address this issue. See comment from Kristian posted 18 Mar 10:55. --- .../suite/binlog/r/binlog_stm_ps.result | 3 +- .../suite/binlog/r/binlog_unsafe.result | 28 +++++++++++++++++++ mysql-test/suite/binlog/t/binlog_unsafe.test | 22 ++++++++++++++- sql/sql_delete.cc | 13 +++++++++ sql/sql_insert.cc | 13 +++++++++ sql/sql_update.cc | 13 +++++++++ 6 files changed, 90 insertions(+), 2 deletions(-) diff --git a/mysql-test/suite/binlog/r/binlog_stm_ps.result b/mysql-test/suite/binlog/r/binlog_stm_ps.result index 47934665116..2128367deb5 100644 --- a/mysql-test/suite/binlog/r/binlog_stm_ps.result +++ b/mysql-test/suite/binlog/r/binlog_stm_ps.result @@ -16,5 +16,6 @@ master-bin.000001 # Query # # use `test`; create table t1 (a int) master-bin.000001 # User var # # @`a`=98 master-bin.000001 # Query # # use `test`; insert into t1 values (@a),(98) master-bin.000001 # Query # # use `test`; insert into t1 values (99) -master-bin.000001 # Query # # use `test`; insert into t1 select 100 limit 100 +master-bin.000001 # Table_map # # table_id: # (test.t1) +master-bin.000001 # Write_rows # # table_id: # flags: STMT_END_F drop table t1; diff --git a/mysql-test/suite/binlog/r/binlog_unsafe.result b/mysql-test/suite/binlog/r/binlog_unsafe.result index 47284ed8bc3..9d7f359ef15 100644 --- a/mysql-test/suite/binlog/r/binlog_unsafe.result +++ b/mysql-test/suite/binlog/r/binlog_unsafe.result @@ -11,3 +11,31 @@ Level Warning Code 1592 Message Statement is not safe to log in statement format. DROP TABLE t1,t2,t3; +CREATE TABLE t1(a INT, b INT, KEY(a), PRIMARY KEY(b)); +INSERT INTO t1 SELECT * FROM t1 LIMIT 1; +Warnings: +Warning 1592 Statement is not safe to log in statement format. +REPLACE INTO t1 SELECT * FROM t1 LIMIT 1; +Warnings: +Warning 1592 Statement is not safe to log in statement format. +UPDATE t1 SET a=1 LIMIT 1; +Warnings: +Warning 1592 Statement is not safe to log in statement format. +DELETE FROM t1 LIMIT 1; +Warnings: +Warning 1592 Statement is not safe to log in statement format. +CREATE PROCEDURE p1() +BEGIN +INSERT INTO t1 SELECT * FROM t1 LIMIT 1; +REPLACE INTO t1 SELECT * FROM t1 LIMIT 1; +UPDATE t1 SET a=1 LIMIT 1; +DELETE FROM t1 LIMIT 1; +END| +CALL p1(); +Warnings: +Warning 1592 Statement is not safe to log in statement format. +Warning 1592 Statement is not safe to log in statement format. +Warning 1592 Statement is not safe to log in statement format. +Warning 1592 Statement is not safe to log in statement format. +DROP PROCEDURE p1; +DROP TABLE t1; diff --git a/mysql-test/suite/binlog/t/binlog_unsafe.test b/mysql-test/suite/binlog/t/binlog_unsafe.test index f34c22dc5f7..de15f5f6f14 100644 --- a/mysql-test/suite/binlog/t/binlog_unsafe.test +++ b/mysql-test/suite/binlog/t/binlog_unsafe.test @@ -15,4 +15,24 @@ query_vertical SHOW WARNINGS; DROP TABLE t1,t2,t3; - +# +# BUG#34768 - nondeterministic INSERT using LIMIT logged in stmt mode if +# binlog_format=mixed +# +CREATE TABLE t1(a INT, b INT, KEY(a), PRIMARY KEY(b)); +INSERT INTO t1 SELECT * FROM t1 LIMIT 1; +REPLACE INTO t1 SELECT * FROM t1 LIMIT 1; +UPDATE t1 SET a=1 LIMIT 1; +DELETE FROM t1 LIMIT 1; +delimiter |; +CREATE PROCEDURE p1() +BEGIN + INSERT INTO t1 SELECT * FROM t1 LIMIT 1; + REPLACE INTO t1 SELECT * FROM t1 LIMIT 1; + UPDATE t1 SET a=1 LIMIT 1; + DELETE FROM t1 LIMIT 1; +END| +delimiter ;| +CALL p1(); +DROP PROCEDURE p1; +DROP TABLE t1; diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index 990f7713561..abf25e96be0 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -418,6 +418,19 @@ bool mysql_prepare_delete(THD *thd, TABLE_LIST *table_list, Item **conds) DBUG_ENTER("mysql_prepare_delete"); List all_fields; + /* + Statement-based replication of DELETE ... LIMIT is not safe as order of + rows is not defined, so in mixed mode we go to row-based. + + Note that we may consider a statement as safe if ORDER BY primary_key + is present. However it may confuse users to see very similiar statements + replicated differently. + */ + if (thd->lex->current_select->select_limit) + { + thd->lex->set_stmt_unsafe(); + thd->set_current_stmt_binlog_row_based_if_mixed(); + } thd->lex->allow_sum_func= 0; if (setup_tables_and_check_access(thd, &thd->lex->select_lex.context, &thd->lex->select_lex.top_join_list, diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index 2be932a6040..58acf40964b 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -2763,6 +2763,19 @@ bool mysql_insert_select_prepare(THD *thd) TABLE_LIST *first_select_leaf_table; DBUG_ENTER("mysql_insert_select_prepare"); + /* + Statement-based replication of INSERT ... SELECT ... LIMIT is not safe + as order of rows is not defined, so in mixed mode we go to row-based. + + Note that we may consider a statement as safe if ORDER BY primary_key + is present or we SELECT a constant. However it may confuse users to + see very similiar statements replicated differently. + */ + if (lex->current_select->select_limit) + { + lex->set_stmt_unsafe(); + thd->set_current_stmt_binlog_row_based_if_mixed(); + } /* SELECT_LEX do not belong to INSERT statement, so we can't add WHERE clause if table is VIEW diff --git a/sql/sql_update.cc b/sql/sql_update.cc index 7c9ead7591c..77d5fd7421c 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -859,6 +859,19 @@ bool mysql_prepare_update(THD *thd, TABLE_LIST *table_list, SELECT_LEX *select_lex= &thd->lex->select_lex; DBUG_ENTER("mysql_prepare_update"); + /* + Statement-based replication of UPDATE ... LIMIT is not safe as order of + rows is not defined, so in mixed mode we go to row-based. + + Note that we may consider a statement as safe if ORDER BY primary_key + is present. However it may confuse users to see very similiar statements + replicated differently. + */ + if (thd->lex->current_select->select_limit) + { + thd->lex->set_stmt_unsafe(); + thd->set_current_stmt_binlog_row_based_if_mixed(); + } #ifndef NO_EMBEDDED_ACCESS_CHECKS table_list->grant.want_privilege= table->grant.want_privilege= (SELECT_ACL & ~table->grant.privilege); From 0d5bd0c68eb1a10b5a43688ff874fdb8f7ae07c3 Mon Sep 17 00:00:00 2001 From: "svoj@mysql.com/june.mysql.com" <> Date: Thu, 20 Mar 2008 11:40:26 +0400 Subject: [PATCH 2/5] BUG#34790 - 'create server' doesn't handle out of memory scenario well enough CREATE SERVER may cause server crash if there is not enough memory to execute this operation. Fixed that create_server() and prepare_server_struct_for_insert() didn't check return value of functions that allocate memory. As this is out of memory issue fix, not test case available. --- sql/sql_servers.cc | 68 ++++++++++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 26 deletions(-) diff --git a/sql/sql_servers.cc b/sql/sql_servers.cc index 8203ca92eed..6255901d782 100644 --- a/sql/sql_servers.cc +++ b/sql/sql_servers.cc @@ -55,8 +55,8 @@ static bool get_server_from_table_to_cache(TABLE *table); 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); +static FOREIGN_SERVER * +prepare_server_struct_for_insert(LEX_SERVER_OPTIONS *server_options); /* drop functions */ static int delete_server_record(TABLE *table, char *server_name, @@ -966,10 +966,14 @@ int create_server(THD *thd, LEX_SERVER_OPTIONS *server_options) server_options->server_name_length)) goto end; - server= (FOREIGN_SERVER *)alloc_root(&mem, - sizeof(FOREIGN_SERVER)); - prepare_server_struct_for_insert(server_options, server); + if (!(server= prepare_server_struct_for_insert(server_options))) + { + /* purecov: begin inspected */ + error= ER_OUT_OF_RESOURCES; + goto end; + /* purecov: end */ + } error= insert_server(thd, server); @@ -1040,52 +1044,64 @@ end: SYNOPSIS prepare_server_struct_for_insert() LEX_SERVER_OPTIONS *server_options - FOREIGN_SERVER *server NOTES + As FOREIGN_SERVER members are allocated on mem_root, we do not need to + free them in case of error. RETURN VALUE - none + On success filled FOREIGN_SERVER, or NULL in case out of memory. */ -static void -prepare_server_struct_for_insert(LEX_SERVER_OPTIONS *server_options, - FOREIGN_SERVER *server) +static FOREIGN_SERVER * +prepare_server_struct_for_insert(LEX_SERVER_OPTIONS *server_options) { char *unset_ptr= (char*)""; + FOREIGN_SERVER *server; DBUG_ENTER("prepare_server_struct"); + if (!(server= (FOREIGN_SERVER *)alloc_root(&mem, sizeof(FOREIGN_SERVER)))) + DBUG_RETURN(NULL); /* purecov: inspected */ + /* these two MUST be set */ - server->server_name= strdup_root(&mem, server_options->server_name); + if (!(server->server_name= strdup_root(&mem, server_options->server_name))) + DBUG_RETURN(NULL); /* purecov: inspected */ server->server_name_length= server_options->server_name_length; - server->host= server_options->host ? - strdup_root(&mem, server_options->host) : unset_ptr; + if (!(server->host= server_options->host ? + strdup_root(&mem, server_options->host) : unset_ptr)) + DBUG_RETURN(NULL); /* purecov: inspected */ - server->db= server_options->db ? - strdup_root(&mem, server_options->db) : unset_ptr; + if (!(server->db= server_options->db ? + strdup_root(&mem, server_options->db) : unset_ptr)) + DBUG_RETURN(NULL); /* purecov: inspected */ - server->username= server_options->username ? - strdup_root(&mem, server_options->username) : unset_ptr; + if (!(server->username= server_options->username ? + strdup_root(&mem, server_options->username) : unset_ptr)) + DBUG_RETURN(NULL); /* purecov: inspected */ - server->password= server_options->password ? - strdup_root(&mem, server_options->password) : unset_ptr; + if (!(server->password= server_options->password ? + strdup_root(&mem, server_options->password) : unset_ptr)) + DBUG_RETURN(NULL); /* purecov: inspected */ /* set to 0 if not specified */ server->port= server_options->port > -1 ? server_options->port : 0; - server->socket= server_options->socket ? - strdup_root(&mem, server_options->socket) : unset_ptr; + if (!(server->socket= server_options->socket ? + strdup_root(&mem, server_options->socket) : unset_ptr)) + DBUG_RETURN(NULL); /* purecov: inspected */ - server->scheme= server_options->scheme ? - strdup_root(&mem, server_options->scheme) : unset_ptr; + if (!(server->scheme= server_options->scheme ? + strdup_root(&mem, server_options->scheme) : unset_ptr)) + DBUG_RETURN(NULL); /* purecov: inspected */ - server->owner= server_options->owner ? - strdup_root(&mem, server_options->owner) : unset_ptr; + if (!(server->owner= server_options->owner ? + strdup_root(&mem, server_options->owner) : unset_ptr)) + DBUG_RETURN(NULL); /* purecov: inspected */ - DBUG_VOID_RETURN; + DBUG_RETURN(server); } /* From 5309d2a521e4220ac6771651042cd518ef212358 Mon Sep 17 00:00:00 2001 From: "svoj@mysql.com/june.mysql.com" <> Date: Thu, 20 Mar 2008 11:57:30 +0400 Subject: [PATCH 3/5] BUG#34789 - drop server/create server leaks memory ! When CREATE SERVER is issued, it allocates memory on memory root to store cached server structure. When DROP SERVER is issued, it doesn't release this memory, as it is impossible with the memory root. We use the same allocation strategy for plugins and acl. The problem here that there was no way (except for the server restart) to force 'servers' code to release this memory. With this fix it is possible to release unused server cache memory by FLUSH PRIVILEGES. No test case for this fix. --- sql/sql_parse.cc | 2 ++ sql/sql_servers.cc | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index f2e0065f860..046eae4e654 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -6486,6 +6486,8 @@ bool reload_acl_and_cache(THD *thd, ulong options, TABLE_LIST *tables, result= 1; if (grant_reload(thd)) result= 1; + if (servers_reload(thd)) + result= 1; } if (tmp_thd) { diff --git a/sql/sql_servers.cc b/sql/sql_servers.cc index 8203ca92eed..42abd9b4eb5 100644 --- a/sql/sql_servers.cc +++ b/sql/sql_servers.cc @@ -166,6 +166,9 @@ end: RETURN VALUES FALSE Success TRUE Error + + TODO + Revert back to old list if we failed to load new one. */ static bool servers_load(THD *thd, TABLE_LIST *tables) @@ -175,10 +178,9 @@ static bool servers_load(THD *thd, TABLE_LIST *tables) bool return_val= TRUE; DBUG_ENTER("servers_load"); - /* 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)); my_hash_reset(&servers_cache); + free_root(&mem, MYF(0)); + init_alloc_root(&mem, ACL_ALLOC_BLOCK_SIZE, 0); init_read_record(&read_record_info,thd,table=tables[0].table,NULL,1,0); while (!(read_record_info.read_record(&read_record_info))) From b799ea244cf9b95dd30d5b0a6c5fcc84e08cad53 Mon Sep 17 00:00:00 2001 From: "svoj@mysql.com/june.mysql.com" <> Date: Tue, 25 Mar 2008 17:28:12 +0400 Subject: [PATCH 4/5] BUG#34768 - nondeterministic INSERT using LIMIT logged in stmt mode if binlog_format=mixed Addition to fix for BUG#34768: fixed test case failures discovered by pushbuild. --- mysql-test/suite/binlog/r/binlog_stm_ps.result | 5 +++-- mysql-test/suite/binlog/t/binlog_stm_ps.test | 2 +- mysql-test/suite/rpl/t/rpl_optimize.test | 2 ++ mysql-test/suite/rpl/t/rpl_user_variables.test | 2 ++ 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/mysql-test/suite/binlog/r/binlog_stm_ps.result b/mysql-test/suite/binlog/r/binlog_stm_ps.result index 2128367deb5..1cf7429987e 100644 --- a/mysql-test/suite/binlog/r/binlog_stm_ps.result +++ b/mysql-test/suite/binlog/r/binlog_stm_ps.result @@ -10,12 +10,13 @@ execute s using @a; prepare s from "insert into t1 select 100 limit ?"; set @a=100; execute s using @a; +Warnings: +Warning 1592 Statement is not safe to log in statement format. show binlog events from ; Log_name Pos Event_type Server_id End_log_pos Info master-bin.000001 # Query # # use `test`; create table t1 (a int) master-bin.000001 # User var # # @`a`=98 master-bin.000001 # Query # # use `test`; insert into t1 values (@a),(98) master-bin.000001 # Query # # use `test`; insert into t1 values (99) -master-bin.000001 # Table_map # # table_id: # (test.t1) -master-bin.000001 # Write_rows # # table_id: # flags: STMT_END_F +master-bin.000001 # Query # # use `test`; insert into t1 select 100 limit 100 drop table t1; diff --git a/mysql-test/suite/binlog/t/binlog_stm_ps.test b/mysql-test/suite/binlog/t/binlog_stm_ps.test index 83add5af3d7..55e3d30ad23 100644 --- a/mysql-test/suite/binlog/t/binlog_stm_ps.test +++ b/mysql-test/suite/binlog/t/binlog_stm_ps.test @@ -1,7 +1,7 @@ # This test is to verify replication with PS -- source include/not_embedded.inc --- source include/have_binlog_format_mixed_or_statement.inc +-- source include/have_binlog_format_statement.inc -- disable_query_log reset master; # get rid of previous tests binlog diff --git a/mysql-test/suite/rpl/t/rpl_optimize.test b/mysql-test/suite/rpl/t/rpl_optimize.test index 80f0c052fc8..f4582ba1167 100644 --- a/mysql-test/suite/rpl/t/rpl_optimize.test +++ b/mysql-test/suite/rpl/t/rpl_optimize.test @@ -31,7 +31,9 @@ INSERT INTO t1 (a) SELECT null FROM t1; save_master_pos; # a few updates to force OPTIMIZE to do something update t1 set b=(a/2*rand()); +--disable_warnings delete from t1 order by b limit 10000; +--enable_warnings connection slave; sync_with_master; diff --git a/mysql-test/suite/rpl/t/rpl_user_variables.test b/mysql-test/suite/rpl/t/rpl_user_variables.test index 21b2063c9f2..8d570f28f64 100644 --- a/mysql-test/suite/rpl/t/rpl_user_variables.test +++ b/mysql-test/suite/rpl/t/rpl_user_variables.test @@ -315,7 +315,9 @@ create table t1(a int); insert into t1 values (1),(2); prepare s1 from 'insert into t1 select a from t1 limit ?'; set @x='1.1'; +--disable_warnings execute s1 using @x; +--enable_warnings select * from t1; sync_slave_with_master; connection slave; From 88247fd271d0c93a102d0fe0a5a6085496e13b8b Mon Sep 17 00:00:00 2001 From: "svoj@mysql.com/june.mysql.com" <> Date: Tue, 25 Mar 2008 17:37:53 +0400 Subject: [PATCH 5/5] BUG#34789 - drop server/create server leaks memory ! BUG#34790 - 'create server' doesn't handle out of memory scenario well enough This is an addition to fixes for these bugs, which makes gcov happy. --- mysql-test/r/federated.result | 2 ++ mysql-test/t/federated.test | 6 ++++++ sql/sql_parse.cc | 2 +- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/mysql-test/r/federated.result b/mysql-test/r/federated.result index 65fafcb2166..21743c3a8a6 100644 --- a/mysql-test/r/federated.result +++ b/mysql-test/r/federated.result @@ -2092,6 +2092,8 @@ DROP TABLE t1; DROP TABLE t1; CREATE TABLE t1 (a INT) ENGINE=federated CONNECTION='mysql://@:://'; DROP TABLE t1; +create server 's1' foreign data wrapper 'mysql' options (port 3306); +drop server 's1'; End of 5.1 tests DROP TABLE IF EXISTS federated.t1; DROP DATABASE IF EXISTS federated; diff --git a/mysql-test/t/federated.test b/mysql-test/t/federated.test index 0fd2290d619..6540cf8648e 100644 --- a/mysql-test/t/federated.test +++ b/mysql-test/t/federated.test @@ -1823,5 +1823,11 @@ DROP TABLE t1; CREATE TABLE t1 (a INT) ENGINE=federated CONNECTION='mysql://@:://'; DROP TABLE t1; +# +# Coverage testing of CREATE SERVER. +# +create server 's1' foreign data wrapper 'mysql' options (port 3306); +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 046eae4e654..a2708632a56 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -6487,7 +6487,7 @@ bool reload_acl_and_cache(THD *thd, ulong options, TABLE_LIST *tables, if (grant_reload(thd)) result= 1; if (servers_reload(thd)) - result= 1; + result= 1; /* purecov: inspected */ } if (tmp_thd) {