From 1f4edce15b2ba3846d6884c810b303c891d00e40 Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Tue, 17 Oct 2006 12:10:51 -0700 Subject: [PATCH 01/23] Bug#20028 (Function with select return no data) This patch reverts a change introduced by Bug 6951, which incorrectly set thd->abort_on_warning for stored procedures. As per internal discussions about the SQL_MODE=TRADITIONAL, the correct behavior is to *not* abort on warnings even inside an INSERT/UPDATE trigger. Tests for Stored Procedures, Stored Functions, Triggers involving SQL_MODE have been included or revised, to reflect the intended behavior. --- mysql-test/include/sp-vars.inc | 9 +++ mysql-test/r/sp-vars.result | 23 ++++++ mysql-test/r/sp.result | 130 ++++++++++++++++++++++++++++++++ mysql-test/r/trigger.result | 64 +++++++++++++++- mysql-test/t/sp-vars.test | 10 +++ mysql-test/t/sp.test | 134 +++++++++++++++++++++++++++++++++ mysql-test/t/trigger.test | 63 +++++++++++++++- sql/sp_head.cc | 3 +- 8 files changed, 428 insertions(+), 8 deletions(-) diff --git a/mysql-test/include/sp-vars.inc b/mysql-test/include/sp-vars.inc index 3e02c9d1709..4bac883ee0e 100644 --- a/mysql-test/include/sp-vars.inc +++ b/mysql-test/include/sp-vars.inc @@ -119,4 +119,13 @@ END| --------------------------------------------------------------------------- +CREATE FUNCTION sp_vars_div_zero() RETURNS INTEGER +BEGIN + DECLARE div_zero INTEGER; + SELECT 1/0 INTO div_zero; + RETURN div_zero; +END| + +--------------------------------------------------------------------------- + delimiter ;| diff --git a/mysql-test/r/sp-vars.result b/mysql-test/r/sp-vars.result index 83ee188bfa4..2ff7bfa9e8a 100644 --- a/mysql-test/r/sp-vars.result +++ b/mysql-test/r/sp-vars.result @@ -4,6 +4,7 @@ DROP FUNCTION IF EXISTS sp_vars_check_ret1; DROP FUNCTION IF EXISTS sp_vars_check_ret2; DROP FUNCTION IF EXISTS sp_vars_check_ret3; DROP FUNCTION IF EXISTS sp_vars_check_ret4; +DROP FUNCTION IF EXISTS sp_vars_div_zero; SET @@sql_mode = 'ansi'; CREATE PROCEDURE sp_vars_check_dflt() BEGIN @@ -88,6 +89,12 @@ CREATE FUNCTION sp_vars_check_ret4() RETURNS DECIMAL(64, 2) BEGIN RETURN 12 * 10 + 34 + 0.1234; END| +CREATE FUNCTION sp_vars_div_zero() RETURNS INTEGER +BEGIN +DECLARE div_zero INTEGER; +SELECT 1/0 INTO div_zero; +RETURN div_zero; +END| --------------------------------------------------------------- Calling the routines, created in ANSI mode. @@ -172,6 +179,9 @@ sp_vars_check_ret4() 154.12 Warnings: Note 1265 Data truncated for column 'sp_vars_check_ret4()' at row 1 +SELECT sp_vars_div_zero(); +sp_vars_div_zero() +NULL SET @@sql_mode = 'traditional'; --------------------------------------------------------------- @@ -257,12 +267,16 @@ sp_vars_check_ret4() 154.12 Warnings: Note 1265 Data truncated for column 'sp_vars_check_ret4()' at row 1 +SELECT sp_vars_div_zero(); +sp_vars_div_zero() +NULL DROP PROCEDURE sp_vars_check_dflt; DROP PROCEDURE sp_vars_check_assignment; DROP FUNCTION sp_vars_check_ret1; DROP FUNCTION sp_vars_check_ret2; DROP FUNCTION sp_vars_check_ret3; DROP FUNCTION sp_vars_check_ret4; +DROP FUNCTION sp_vars_div_zero; CREATE PROCEDURE sp_vars_check_dflt() BEGIN DECLARE v1 TINYINT DEFAULT 1e200; @@ -346,6 +360,12 @@ CREATE FUNCTION sp_vars_check_ret4() RETURNS DECIMAL(64, 2) BEGIN RETURN 12 * 10 + 34 + 0.1234; END| +CREATE FUNCTION sp_vars_div_zero() RETURNS INTEGER +BEGIN +DECLARE div_zero INTEGER; +SELECT 1/0 INTO div_zero; +RETURN div_zero; +END| --------------------------------------------------------------- Calling the routines, created in TRADITIONAL mode. @@ -366,6 +386,8 @@ sp_vars_check_ret4() 154.12 Warnings: Note 1265 Data truncated for column 'sp_vars_check_ret4()' at row 1 +SELECT sp_vars_div_zero(); +ERROR 22012: Division by 0 SET @@sql_mode = 'ansi'; DROP PROCEDURE sp_vars_check_dflt; DROP PROCEDURE sp_vars_check_assignment; @@ -373,6 +395,7 @@ DROP FUNCTION sp_vars_check_ret1; DROP FUNCTION sp_vars_check_ret2; DROP FUNCTION sp_vars_check_ret3; DROP FUNCTION sp_vars_check_ret4; +DROP FUNCTION sp_vars_div_zero; --------------------------------------------------------------- BIT data type tests diff --git a/mysql-test/r/sp.result b/mysql-test/r/sp.result index 061b754b6cd..d82b63a2217 100644 --- a/mysql-test/r/sp.result +++ b/mysql-test/r/sp.result @@ -5457,4 +5457,134 @@ CAD CHF DROP FUNCTION bug21493| DROP TABLE t3,t4| +drop function if exists func_20028_a| +drop function if exists func_20028_b| +drop function if exists func_20028_c| +drop procedure if exists proc_20028_a| +drop procedure if exists proc_20028_b| +drop procedure if exists proc_20028_c| +drop table if exists table_20028| +create table table_20028 (i int)| +SET @save_sql_mode=@@sql_mode| +SET sql_mode=''| +create function func_20028_a() returns integer +begin +declare temp integer; +select i into temp from table_20028 limit 1; +return ifnull(temp, 0); +end| +create function func_20028_b() returns integer +begin +return func_20028_a(); +end| +create function func_20028_c() returns integer +begin +declare div_zero integer; +set SQL_MODE='TRADITIONAL'; +select 1/0 into div_zero; +return div_zero; +end| +create procedure proc_20028_a() +begin +declare temp integer; +select i into temp from table_20028 limit 1; +end| +create procedure proc_20028_b() +begin +call proc_20028_a(); +end| +create procedure proc_20028_c() +begin +declare div_zero integer; +set SQL_MODE='TRADITIONAL'; +select 1/0 into div_zero; +end| +select func_20028_a()| +func_20028_a() +0 +Warnings: +Warning 1329 No data - zero rows fetched, selected, or processed +select func_20028_b()| +func_20028_b() +0 +Warnings: +Warning 1329 No data - zero rows fetched, selected, or processed +select func_20028_c()| +ERROR 22012: Division by 0 +call proc_20028_a()| +Warnings: +Warning 1329 No data - zero rows fetched, selected, or processed +call proc_20028_b()| +Warnings: +Warning 1329 No data - zero rows fetched, selected, or processed +call proc_20028_c()| +ERROR 22012: Division by 0 +SET sql_mode='TRADITIONAL'| +drop function func_20028_a| +drop function func_20028_b| +drop function func_20028_c| +drop procedure proc_20028_a| +drop procedure proc_20028_b| +drop procedure proc_20028_c| +create function func_20028_a() returns integer +begin +declare temp integer; +select i into temp from table_20028 limit 1; +return ifnull(temp, 0); +end| +create function func_20028_b() returns integer +begin +return func_20028_a(); +end| +create function func_20028_c() returns integer +begin +declare div_zero integer; +set SQL_MODE=''; +select 1/0 into div_zero; +return div_zero; +end| +create procedure proc_20028_a() +begin +declare temp integer; +select i into temp from table_20028 limit 1; +end| +create procedure proc_20028_b() +begin +call proc_20028_a(); +end| +create procedure proc_20028_c() +begin +declare div_zero integer; +set SQL_MODE=''; +select 1/0 into div_zero; +end| +select func_20028_a()| +func_20028_a() +0 +Warnings: +Warning 1329 No data - zero rows fetched, selected, or processed +select func_20028_b()| +func_20028_b() +0 +Warnings: +Warning 1329 No data - zero rows fetched, selected, or processed +select func_20028_c()| +func_20028_c() +NULL +call proc_20028_a()| +Warnings: +Warning 1329 No data - zero rows fetched, selected, or processed +call proc_20028_b()| +Warnings: +Warning 1329 No data - zero rows fetched, selected, or processed +call proc_20028_c()| +SET @@sql_mode=@save_sql_mode| +drop function func_20028_a| +drop function func_20028_b| +drop function func_20028_c| +drop procedure proc_20028_a| +drop procedure proc_20028_b| +drop procedure proc_20028_c| +drop table table_20028| +End of 5.0 tests drop table t1,t2; diff --git a/mysql-test/r/trigger.result b/mysql-test/r/trigger.result index c687d4c49c8..b849b0d7576 100644 --- a/mysql-test/r/trigger.result +++ b/mysql-test/r/trigger.result @@ -1073,10 +1073,11 @@ SELECT @x; NULL SET @x=2; UPDATE t1 SET i1 = @x; -ERROR 22012: Division by 0 +Warnings: +Error 1365 Division by 0 SELECT @x; @x -2 +NULL SET SQL_MODE=''; SET @x=3; INSERT INTO t1 VALUES (@x); @@ -1085,10 +1086,12 @@ SELECT @x; NULL SET @x=4; UPDATE t1 SET i1 = @x; -ERROR 22012: Division by 0 +Warnings: +Error 1365 Division by 0 +Error 1365 Division by 0 SELECT @x; @x -4 +NULL SET @@sql_mode=@save_sql_mode; DROP TRIGGER t1_ai; DROP TRIGGER t1_au; @@ -1173,4 +1176,57 @@ TRIGGER t2_bi BEFORE INSERT ON t2 FOR EACH ROW SET @a = 2; ERROR HY000: String '1234567890abcdefghij1234567890abcdefghij1234567890abcdefghijQWERTY' is too long for host name (should be no longer than 60) DROP TABLE t1; DROP TABLE t2; +drop table if exists t1; +drop table if exists t2; +drop table if exists t3; +drop table if exists t4; +SET @save_sql_mode=@@sql_mode; +SET sql_mode='TRADITIONAL'| +create table t1 (id int(10) not null primary key, v int(10) )| +create table t2 (id int(10) not null primary key, v int(10) )| +create table t3 (id int(10) not null primary key, v int(10) )| +create table t4 (c int)| +create trigger t4_bi before insert on t4 for each row set @t4_bi_called:=1| +create trigger t4_bu before update on t4 for each row set @t4_bu_called:=1| +insert into t1 values(10, 10)| +set @a:=1/0| +Warnings: +Error 1365 Division by 0 +select 1/0 from t1| +1/0 +NULL +Warnings: +Error 1365 Division by 0 +create trigger t1_bi before insert on t1 for each row set @a:=1/0| +insert into t1 values(20, 20)| +Warnings: +Error 1365 Division by 0 +drop trigger t1_bi| +create trigger t1_bi before insert on t1 for each row +begin +insert into t2 values (new.id, new.v); +update t2 set v=v+1 where id= new.id; +replace t3 values (new.id, 0); +update t2, t3 set t2.v=new.v, t3.v=new.v where t2.id=t3.id; +create temporary table t5 select * from t1; +delete from t5; +insert into t5 select * from t1; +insert into t4 values (0); +set @check= (select count(*) from t5); +update t4 set c= @check; +drop temporary table t5; +set @a:=1/0; +end| +set @check=0, @t4_bi_called=0, @t4_bu_called=0| +insert into t1 values(30, 30)| +Warnings: +Error 1365 Division by 0 +select @check, @t4_bi_called, @t4_bu_called| +@check @t4_bi_called @t4_bu_called +2 1 1 +SET @@sql_mode=@save_sql_mode; +drop table t1; +drop table t2; +drop table t3; +drop table t4; End of 5.0 tests diff --git a/mysql-test/t/sp-vars.test b/mysql-test/t/sp-vars.test index 48dbd4de7aa..7cf92dc5d0d 100644 --- a/mysql-test/t/sp-vars.test +++ b/mysql-test/t/sp-vars.test @@ -15,6 +15,7 @@ DROP FUNCTION IF EXISTS sp_vars_check_ret1; DROP FUNCTION IF EXISTS sp_vars_check_ret2; DROP FUNCTION IF EXISTS sp_vars_check_ret3; DROP FUNCTION IF EXISTS sp_vars_check_ret4; +DROP FUNCTION IF EXISTS sp_vars_div_zero; --enable_warnings @@ -49,6 +50,8 @@ SELECT sp_vars_check_ret3(); SELECT sp_vars_check_ret4(); +SELECT sp_vars_div_zero(); + # Check that changing sql_mode after creating a store procedure does not # matter. @@ -72,6 +75,8 @@ SELECT sp_vars_check_ret3(); SELECT sp_vars_check_ret4(); +SELECT sp_vars_div_zero(); + # Create the procedure in TRADITIONAL mode. Check that error will be thrown on # execution. @@ -81,6 +86,7 @@ DROP FUNCTION sp_vars_check_ret1; DROP FUNCTION sp_vars_check_ret2; DROP FUNCTION sp_vars_check_ret3; DROP FUNCTION sp_vars_check_ret4; +DROP FUNCTION sp_vars_div_zero; --source include/sp-vars.inc @@ -110,6 +116,9 @@ SELECT sp_vars_check_ret3(); SELECT sp_vars_check_ret4(); +--error ER_DIVISION_BY_ZERO +SELECT sp_vars_div_zero(); + SET @@sql_mode = 'ansi'; # @@ -122,6 +131,7 @@ DROP FUNCTION sp_vars_check_ret1; DROP FUNCTION sp_vars_check_ret2; DROP FUNCTION sp_vars_check_ret3; DROP FUNCTION sp_vars_check_ret4; +DROP FUNCTION sp_vars_div_zero; ########################################################################### # diff --git a/mysql-test/t/sp.test b/mysql-test/t/sp.test index 4707a9b0d30..c03d98cdabf 100644 --- a/mysql-test/t/sp.test +++ b/mysql-test/t/sp.test @@ -6389,6 +6389,140 @@ SELECT bug21493(Member_ID) FROM t3| DROP FUNCTION bug21493| DROP TABLE t3,t4| +# +# Bug#20028 Function with select return no data +# + +--disable_warnings +drop function if exists func_20028_a| +drop function if exists func_20028_b| +drop function if exists func_20028_c| +drop procedure if exists proc_20028_a| +drop procedure if exists proc_20028_b| +drop procedure if exists proc_20028_c| +drop table if exists table_20028| +--enable_warnings + +create table table_20028 (i int)| + +SET @save_sql_mode=@@sql_mode| + +SET sql_mode=''| + +create function func_20028_a() returns integer +begin + declare temp integer; + select i into temp from table_20028 limit 1; + return ifnull(temp, 0); +end| + +create function func_20028_b() returns integer +begin + return func_20028_a(); +end| + +create function func_20028_c() returns integer +begin + declare div_zero integer; + set SQL_MODE='TRADITIONAL'; + select 1/0 into div_zero; + return div_zero; +end| + +create procedure proc_20028_a() +begin + declare temp integer; + select i into temp from table_20028 limit 1; +end| + +create procedure proc_20028_b() +begin + call proc_20028_a(); +end| + +create procedure proc_20028_c() +begin + declare div_zero integer; + set SQL_MODE='TRADITIONAL'; + select 1/0 into div_zero; +end| + +select func_20028_a()| +select func_20028_b()| +--error ER_DIVISION_BY_ZERO +select func_20028_c()| +call proc_20028_a()| +call proc_20028_b()| +--error ER_DIVISION_BY_ZERO +call proc_20028_c()| + +SET sql_mode='TRADITIONAL'| + +drop function func_20028_a| +drop function func_20028_b| +drop function func_20028_c| +drop procedure proc_20028_a| +drop procedure proc_20028_b| +drop procedure proc_20028_c| + +create function func_20028_a() returns integer +begin + declare temp integer; + select i into temp from table_20028 limit 1; + return ifnull(temp, 0); +end| + +create function func_20028_b() returns integer +begin + return func_20028_a(); +end| + +create function func_20028_c() returns integer +begin + declare div_zero integer; + set SQL_MODE=''; + select 1/0 into div_zero; + return div_zero; +end| + +create procedure proc_20028_a() +begin + declare temp integer; + select i into temp from table_20028 limit 1; +end| + +create procedure proc_20028_b() +begin + call proc_20028_a(); +end| + +create procedure proc_20028_c() +begin + declare div_zero integer; + set SQL_MODE=''; + select 1/0 into div_zero; +end| + +select func_20028_a()| +select func_20028_b()| +select func_20028_c()| +call proc_20028_a()| +call proc_20028_b()| +call proc_20028_c()| + +SET @@sql_mode=@save_sql_mode| + +drop function func_20028_a| +drop function func_20028_b| +drop function func_20028_c| +drop procedure proc_20028_a| +drop procedure proc_20028_b| +drop procedure proc_20028_c| +drop table table_20028| + +--echo End of 5.0 tests + + # # BUG#NNNN: New bug synopsis # diff --git a/mysql-test/t/trigger.test b/mysql-test/t/trigger.test index 2a145e1eeaa..17a2eab5d62 100644 --- a/mysql-test/t/trigger.test +++ b/mysql-test/t/trigger.test @@ -1274,7 +1274,6 @@ INSERT INTO t1 VALUES (@x); SELECT @x; SET @x=2; ---error ER_DIVISION_BY_ZERO UPDATE t1 SET i1 = @x; SELECT @x; @@ -1285,7 +1284,6 @@ INSERT INTO t1 VALUES (@x); SELECT @x; SET @x=4; ---error ER_DIVISION_BY_ZERO UPDATE t1 SET i1 = @x; SELECT @x; @@ -1420,5 +1418,66 @@ CREATE DEFINER=some_user_name@1234567890abcdefghij1234567890abcdefghij1234567890 DROP TABLE t1; DROP TABLE t2; +# +# Bug#20028 Function with select return no data +# + +--disable_warnings +drop table if exists t1; +drop table if exists t2; +drop table if exists t3; +drop table if exists t4; +--enable_warnings + +SET @save_sql_mode=@@sql_mode; + +delimiter |; +SET sql_mode='TRADITIONAL'| +create table t1 (id int(10) not null primary key, v int(10) )| +create table t2 (id int(10) not null primary key, v int(10) )| +create table t3 (id int(10) not null primary key, v int(10) )| +create table t4 (c int)| + +create trigger t4_bi before insert on t4 for each row set @t4_bi_called:=1| +create trigger t4_bu before update on t4 for each row set @t4_bu_called:=1| + +insert into t1 values(10, 10)| +set @a:=1/0| +select 1/0 from t1| + +create trigger t1_bi before insert on t1 for each row set @a:=1/0| + +insert into t1 values(20, 20)| + +drop trigger t1_bi| +create trigger t1_bi before insert on t1 for each row +begin + insert into t2 values (new.id, new.v); + update t2 set v=v+1 where id= new.id; + replace t3 values (new.id, 0); + update t2, t3 set t2.v=new.v, t3.v=new.v where t2.id=t3.id; + create temporary table t5 select * from t1; + delete from t5; + insert into t5 select * from t1; + insert into t4 values (0); + set @check= (select count(*) from t5); + update t4 set c= @check; + drop temporary table t5; + + set @a:=1/0; +end| + +set @check=0, @t4_bi_called=0, @t4_bu_called=0| +insert into t1 values(30, 30)| +select @check, @t4_bi_called, @t4_bu_called| + +delimiter ;| + +SET @@sql_mode=@save_sql_mode; + +drop table t1; +drop table t2; +drop table t3; +drop table t4; --echo End of 5.0 tests diff --git a/sql/sp_head.cc b/sql/sp_head.cc index 9761de625be..a06bfe28a6f 100644 --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -991,8 +991,7 @@ sp_head::execute(THD *thd) save_sql_mode= thd->variables.sql_mode; thd->variables.sql_mode= m_sql_mode; save_abort_on_warning= thd->abort_on_warning; - thd->abort_on_warning= - (m_sql_mode & (MODE_STRICT_TRANS_TABLES | MODE_STRICT_ALL_TABLES)); + thd->abort_on_warning= 0; /* It is also more efficient to save/restore current thd->lex once when From 3312c2a34d7d1713b15f2c0ddbf81fe9d64f0550 Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Wed, 18 Oct 2006 18:38:58 -0700 Subject: [PATCH 02/23] manual merge --- mysql-test/r/sp.result | 44 +++++++++++++++++++++++++++++++++++-- mysql-test/r/trigger.result | 12 ++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/mysql-test/r/sp.result b/mysql-test/r/sp.result index d82b63a2217..1bb4b3a405b 100644 --- a/mysql-test/r/sp.result +++ b/mysql-test/r/sp.result @@ -5394,6 +5394,21 @@ Procedure sql_mode Create Procedure bug21416 CREATE DEFINER=`root`@`localhost` PROCEDURE `bug21416`() show create procedure bug21416 drop procedure bug21416| +DROP PROCEDURE IF EXISTS bug21414| +CREATE PROCEDURE bug21414() SELECT 1| +FLUSH TABLES WITH READ LOCK| +DROP PROCEDURE bug21414| +ERROR HY000: Can't execute the query because you have a conflicting read lock +UNLOCK TABLES| +The following should succeed. +DROP PROCEDURE bug21414| +set names utf8| +drop database if exists това_е_дълго_име_за_база_данни_нали| +create database това_е_дълго_име_за_база_данни_нали| +INSERT INTO mysql.proc VALUES ('това_е_дълго_име_за_база_данни_нали','това_е_процедура_с_доста_дълго_име_нали_и_още_по_дълго','PROCEDURE','това_е_процедура_с_доста_дълго_име_нали_и_още_по_дълго','SQL','CONTAINS_SQL','NO','DEFINER','','','bad_body','root@localhost',now(), now(),'','')| +call това_е_дълго_име_за_база_данни_нали.това_е_процедура_с_доста_дълго_име_нали_и_още_по_дълго()| +ERROR HY000: Failed to load routine това_е_дълго_име_за_база_данни_нали.това_е_процедура_с_доста_дълго_име_нали_и_още_по_дълго. The table mysql.proc is missing, corrupt, or contains bad data (internal code -6) +drop database това_е_дълго_име_за_база_данни_нали| CREATE TABLE t3 ( Member_ID varchar(15) NOT NULL, PRIMARY KEY (Member_ID) @@ -5430,8 +5445,6 @@ INSERT INTO t4(Member_ID, Action, Action_Date, Track) VALUES ('666666', 'Enrolled', '2006-05-12', 'CHF' ), ('666666', 'Disenrolled', '2006-06-01', 'CAD' )| DROP FUNCTION IF EXISTS bug21493| -Warnings: -Note 1305 FUNCTION bug21493 does not exist CREATE FUNCTION bug21493(paramMember VARCHAR(15)) RETURNS varchar(45) BEGIN DECLARE tracks VARCHAR(45); @@ -5586,5 +5599,32 @@ drop procedure proc_20028_a| drop procedure proc_20028_b| drop procedure proc_20028_c| drop table table_20028| +drop procedure if exists proc_21462_a| +drop procedure if exists proc_21462_b| +create procedure proc_21462_a() +begin +select "Called A"; +end| +create procedure proc_21462_b(x int) +begin +select "Called B"; +end| +call proc_21462_a| +Called A +Called A +call proc_21462_a()| +Called A +Called A +call proc_21462_a(1)| +ERROR 42000: Incorrect number of arguments for PROCEDURE test.proc_21462_a; expected 0, got 1 +call proc_21462_b| +ERROR 42000: Incorrect number of arguments for PROCEDURE test.proc_21462_b; expected 1, got 0 +call proc_21462_b()| +ERROR 42000: Incorrect number of arguments for PROCEDURE test.proc_21462_b; expected 1, got 0 +call proc_21462_b(1)| +Called B +Called B +drop procedure proc_21462_a| +drop procedure proc_21462_b| End of 5.0 tests drop table t1,t2; diff --git a/mysql-test/r/trigger.result b/mysql-test/r/trigger.result index b849b0d7576..5d643057666 100644 --- a/mysql-test/r/trigger.result +++ b/mysql-test/r/trigger.result @@ -1229,4 +1229,16 @@ drop table t1; drop table t2; drop table t3; drop table t4; +drop table if exists t1; +create table t1 (i int, j int key); +insert into t1 values (1,1), (2,2), (3,3); +create trigger t1_bu before update on t1 for each row +set new.j = new.j + 10; +update t1 set i= i+ 10 where j > 2; +select * from t1; +i j +1 1 +2 2 +13 13 +drop table t1; End of 5.0 tests From ce5a3fcca888bc9233f908da878710d3b81c8a70 Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Fri, 17 Nov 2006 12:14:29 -0700 Subject: [PATCH 03/23] Bug#19194 (Right recursion in parser for CASE causes excessive stack usage, limitation) Note to the reviewer ==================== Warning: reviewing this patch is somewhat involved. Due to the nature of several issues all affecting the same area, fixing separately each issue is not practical, since each fix can not be implemented and tested independently. In particular, the issues with - rule recursion - nested case statements - forward jump resolution (backpatch list) are tightly coupled (see below). Definitions =========== The expression CASE expr WHEN expr THEN expr WHEN expr THEN expr ... END is a "Simple Case Expression". The expression CASE WHEN expr THEN expr WHEN expr THEN expr ... END is a "Searched Case Expression". The statement CASE expr WHEN expr THEN stmts WHEN expr THEN stmts ... END CASE is a "Simple Case Statement". The statement CASE WHEN expr THEN stmts WHEN expr THEN stmts ... END CASE is a "Searched Case Statement". A "Left Recursive" rule is like list: element | list element ; A "Right Recursive" rule is like list: element | element list ; Left and right recursion produces the same language, the difference only affects the *order* in which the text is parsed. In a descendant parser (usually written manually), right recursion works very well, and is typically implemented with a while loop. In an ascendant parser (yacc/bison) left recursion works very well, and is implemented naturally by the parser stack. In both cases, using the wrong type or recursion is very bad and should be avoided, as it causes technical issues with the parser implementation. Before this change ================== The "Simple Case Expression" and "Searched Case Expression" were both implemented by the "when_list" and "when_list2" rules, which are left recursive (ok). These rules, however, used lex->when_list instead of using the parser stack, which is more complex that necessary, and potentially dangerous because of other rules using THD::reset_lex. The "Simple Case Statement" and "Searched Case Statements" were implemented by the "sp_case", "sp_whens" and in part by "sp_proc_stmt" rules. Both cases were right recursive (bad). The grammar involved was convoluted, and is assumed to be the results of tweaks to get the code generation to work, but is not what someone would naturally write. In addition, using a common rule for both "Simple" and "Searched" case statements was implemented with sp_head::m_flags |= IN_SIMPLE_CASE, which is a flag and not a stack, and therefore does not take into account *nested* case statements. This leads to incorrect generated code, and either a server crash or an incorrect result. With regards to the backpatch mechanism, a *different* backpatch list was created for each jump from "WHEN expr THEN stmt" to "END CASE", which relied on the grammar to be right recursive. This is a mis-use of the backpatch list, since this list can resolve multiple references to the same target at once. The optimizer algorithm used to detect dead code in the "assembly" SQL instructions, implemented by sp_head::opt_mark(uint ip), was recursive in some cases (a conditional jump pointing forward to another conditional jump). In case of specially crafted code, like - a long list of "IF expr THEN stmt END IF" - a long CASE statement this would actually cause a server crash with a stack overflow. In general, having a stack that grows proportionally with user data (the SQL code given by the client in a CREATE PROCEDURE) is to be avoided. In debug builds only, creating a SP / SF / Trigger which had a significant amount of code would spend --literally-- several minutes in sp_head::create, because of the debug code involved with DBUG_PRINT("info", ("Code %s ... There are several issues with this code: - in a CASE with 5 000 WHEN, there are 15 000 instructions generated, which create a sting representation of the code which is 500 000 bytes long, - using a String instead of an io stream causes performances to degrade to a total server freeze, as time is spent doing realloc of a buffer always too short, - Printing a 500 000 long string in the debug log is too verbose, - Generating this string even when DBUG_PRINT is off is useless, - Having code that potentially can affect the server behavior, used with #ifdef / #endif is useful in some cases, but is also a bad practice. After this change ================= "Case Expressions" (both simple and searched) have been simplified to not use LEX::when_list, which has been removed. Considering all the issues affecting case statements, the grammar for these has been totally re written. The existing actions, used to generate "assembly" sp_inst* code, have been preserved but moved in the new grammar, with the following changes: a) Bison rules are no longer shared between "Simple" and "Searched" case statements, because a stack instead of a flag is required to handle them. Nested statements are handled naturally by the parser stack, which by definition uses the correct rule in the correct context. Nested statements of the opposite type (simple vs searched) works correctly. The flag sp_head::IN_SIMPLE_CASE is no longer used. This is a step towards resolution of WL#2999, which correctly identified that temporary parsing flags do not belong to sp_head. The code in the action is shared by mean of the case_stmt_action_xxx() helpers. b) The backpatch mechanism, used to resolve forward jumps in the generated code, has been changed to: - create a label for the instruction following 'END CASE', - register each jump at the end of a "WHEN expr THEN stmt" in a *unique* backpatch list associated with the 'END CASE' label - resolve all the forward jumps for this label at once. In addition, the code involving backpatch has been commented, so that a reader can now understand by reading matching "Registering" and "Resolving" comments how the forward jumps are resolved and what target they resolve to, as this is far from evident when reading the code alone. The implementation of sp_head::opt_mark() has been revised to avoid recursive calls from jump instructions, and instead add the jump location to the list of paths to explore during the flow analysis of the instruction graph, with a call to sp_head::add_mark_lead(). In addition, the flow analysis will stop if an instruction has already been marked as reachable, which the previous code failed to do in the recursive case. sp_head::opt_mark() is now private, to prevent new calls to this method from being introduced. The debug code present in sp_head::create() has been removed. Considering that SHOW PROCEDURE CODE is also available in debug builds, and can be used anytime regardless of the trace level, as opposed to "CREATE PROCEDURE" time and only if the trace was on, removing the code actually makes debugging easier (usable trace). Tests have been written to cover the parser overflow (big CASE), and to cover nested CASE statements. --- mysql-test/r/sp-code.result | 415 +++++++++++++++++++++++++++++ mysql-test/r/sp_stress_case.result | 42 +++ mysql-test/t/sp-code.test | 235 ++++++++++++++++ mysql-test/t/sp_stress_case.sh | 68 +++++ mysql-test/t/sp_stress_case.test | 35 +++ sql/sp_head.cc | 100 ++++--- sql/sp_head.h | 39 ++- sql/sql_lex.cc | 1 - sql/sql_lex.h | 1 - sql/sql_yacc.yy | 373 ++++++++++++++++++-------- 10 files changed, 1138 insertions(+), 171 deletions(-) create mode 100644 mysql-test/r/sp_stress_case.result create mode 100755 mysql-test/t/sp_stress_case.sh create mode 100644 mysql-test/t/sp_stress_case.test diff --git a/mysql-test/r/sp-code.result b/mysql-test/r/sp-code.result index 4ae38861d29..0b0ad802b54 100644 --- a/mysql-test/r/sp-code.result +++ b/mysql-test/r/sp-code.result @@ -199,6 +199,421 @@ Pos Instruction 44 jump 14 45 stmt 9 "drop temporary table sudoku_work, sud..." drop procedure sudoku_solve; +DROP PROCEDURE IF EXISTS proc_19194_simple; +DROP PROCEDURE IF EXISTS proc_19194_searched; +DROP PROCEDURE IF EXISTS proc_19194_nested_1; +DROP PROCEDURE IF EXISTS proc_19194_nested_2; +DROP PROCEDURE IF EXISTS proc_19194_nested_3; +DROP PROCEDURE IF EXISTS proc_19194_nested_4; +CREATE PROCEDURE proc_19194_simple(i int) +BEGIN +DECLARE str CHAR(10); +CASE i +WHEN 1 THEN SET str="1"; +WHEN 2 THEN SET str="2"; +WHEN 3 THEN SET str="3"; +ELSE SET str="unknown"; +END CASE; +SELECT str; +END| +CREATE PROCEDURE proc_19194_searched(i int) +BEGIN +DECLARE str CHAR(10); +CASE +WHEN i=1 THEN SET str="1"; +WHEN i=2 THEN SET str="2"; +WHEN i=3 THEN SET str="3"; +ELSE SET str="unknown"; +END CASE; +SELECT str; +END| +CREATE PROCEDURE proc_19194_nested_1(i int, j int) +BEGIN +DECLARE str_i CHAR(10); +DECLARE str_j CHAR(10); +CASE i +WHEN 10 THEN SET str_i="10"; +WHEN 20 THEN +BEGIN +set str_i="20"; +CASE +WHEN j=1 THEN SET str_j="1"; +WHEN j=2 THEN SET str_j="2"; +WHEN j=3 THEN SET str_j="3"; +ELSE SET str_j="unknown"; +END CASE; +select "i was 20"; +END; +WHEN 30 THEN SET str_i="30"; +WHEN 40 THEN SET str_i="40"; +ELSE SET str_i="unknown"; +END CASE; +SELECT str_i, str_j; +END| +CREATE PROCEDURE proc_19194_nested_2(i int, j int) +BEGIN +DECLARE str_i CHAR(10); +DECLARE str_j CHAR(10); +CASE +WHEN i=10 THEN SET str_i="10"; +WHEN i=20 THEN +BEGIN +set str_i="20"; +CASE j +WHEN 1 THEN SET str_j="1"; +WHEN 2 THEN SET str_j="2"; +WHEN 3 THEN SET str_j="3"; +ELSE SET str_j="unknown"; +END CASE; +select "i was 20"; +END; +WHEN i=30 THEN SET str_i="30"; +WHEN i=40 THEN SET str_i="40"; +ELSE SET str_i="unknown"; +END CASE; +SELECT str_i, str_j; +END| +CREATE PROCEDURE proc_19194_nested_3(i int, j int) +BEGIN +DECLARE str_i CHAR(10); +DECLARE str_j CHAR(10); +CASE i +WHEN 10 THEN SET str_i="10"; +WHEN 20 THEN +BEGIN +set str_i="20"; +CASE j +WHEN 1 THEN SET str_j="1"; +WHEN 2 THEN SET str_j="2"; +WHEN 3 THEN SET str_j="3"; +ELSE SET str_j="unknown"; +END CASE; +select "i was 20"; +END; +WHEN 30 THEN SET str_i="30"; +WHEN 40 THEN SET str_i="40"; +ELSE SET str_i="unknown"; +END CASE; +SELECT str_i, str_j; +END| +CREATE PROCEDURE proc_19194_nested_4(i int, j int) +BEGIN +DECLARE str_i CHAR(10); +DECLARE str_j CHAR(10); +CASE +WHEN i=10 THEN SET str_i="10"; +WHEN i=20 THEN +BEGIN +set str_i="20"; +CASE +WHEN j=1 THEN SET str_j="1"; +WHEN j=2 THEN SET str_j="2"; +WHEN j=3 THEN SET str_j="3"; +ELSE SET str_j="unknown"; +END CASE; +select "i was 20"; +END; +WHEN i=30 THEN SET str_i="30"; +WHEN i=40 THEN SET str_i="40"; +ELSE SET str_i="unknown"; +END CASE; +SELECT str_i, str_j; +END| +SHOW PROCEDURE CODE proc_19194_simple; +Pos Instruction +0 set str@1 NULL +1 set_case_expr (12) 0 i@0 +2 jump_if_not 5(12) (case_expr@0 = 1) +3 set str@1 _latin1'1' +4 jump 12 +5 jump_if_not 8(12) (case_expr@0 = 2) +6 set str@1 _latin1'2' +7 jump 12 +8 jump_if_not 11(12) (case_expr@0 = 3) +9 set str@1 _latin1'3' +10 jump 12 +11 set str@1 _latin1'unknown' +12 stmt 0 "SELECT str" +SHOW PROCEDURE CODE proc_19194_searched; +Pos Instruction +0 set str@1 NULL +1 jump_if_not 4(11) (i@0 = 1) +2 set str@1 _latin1'1' +3 jump 11 +4 jump_if_not 7(11) (i@0 = 2) +5 set str@1 _latin1'2' +6 jump 11 +7 jump_if_not 10(11) (i@0 = 3) +8 set str@1 _latin1'3' +9 jump 11 +10 set str@1 _latin1'unknown' +11 stmt 0 "SELECT str" +SHOW PROCEDURE CODE proc_19194_nested_1; +Pos Instruction +0 set str_i@2 NULL +1 set str_j@3 NULL +2 set_case_expr (27) 0 i@0 +3 jump_if_not 6(27) (case_expr@0 = 10) +4 set str_i@2 _latin1'10' +5 jump 27 +6 jump_if_not 20(27) (case_expr@0 = 20) +7 set str_i@2 _latin1'20' +8 jump_if_not 11(18) (j@1 = 1) +9 set str_j@3 _latin1'1' +10 jump 18 +11 jump_if_not 14(18) (j@1 = 2) +12 set str_j@3 _latin1'2' +13 jump 18 +14 jump_if_not 17(18) (j@1 = 3) +15 set str_j@3 _latin1'3' +16 jump 18 +17 set str_j@3 _latin1'unknown' +18 stmt 0 "select "i was 20"" +19 jump 27 +20 jump_if_not 23(27) (case_expr@0 = 30) +21 set str_i@2 _latin1'30' +22 jump 27 +23 jump_if_not 26(27) (case_expr@0 = 40) +24 set str_i@2 _latin1'40' +25 jump 27 +26 set str_i@2 _latin1'unknown' +27 stmt 0 "SELECT str_i, str_j" +SHOW PROCEDURE CODE proc_19194_nested_2; +Pos Instruction +0 set str_i@2 NULL +1 set str_j@3 NULL +2 jump_if_not 5(27) (i@0 = 10) +3 set str_i@2 _latin1'10' +4 jump 27 +5 jump_if_not 20(27) (i@0 = 20) +6 set str_i@2 _latin1'20' +7 set_case_expr (18) 0 j@1 +8 jump_if_not 11(18) (case_expr@0 = 1) +9 set str_j@3 _latin1'1' +10 jump 18 +11 jump_if_not 14(18) (case_expr@0 = 2) +12 set str_j@3 _latin1'2' +13 jump 18 +14 jump_if_not 17(18) (case_expr@0 = 3) +15 set str_j@3 _latin1'3' +16 jump 18 +17 set str_j@3 _latin1'unknown' +18 stmt 0 "select "i was 20"" +19 jump 27 +20 jump_if_not 23(27) (i@0 = 30) +21 set str_i@2 _latin1'30' +22 jump 27 +23 jump_if_not 26(27) (i@0 = 40) +24 set str_i@2 _latin1'40' +25 jump 27 +26 set str_i@2 _latin1'unknown' +27 stmt 0 "SELECT str_i, str_j" +SHOW PROCEDURE CODE proc_19194_nested_3; +Pos Instruction +0 set str_i@2 NULL +1 set str_j@3 NULL +2 set_case_expr (28) 0 i@0 +3 jump_if_not 6(28) (case_expr@0 = 10) +4 set str_i@2 _latin1'10' +5 jump 28 +6 jump_if_not 21(28) (case_expr@0 = 20) +7 set str_i@2 _latin1'20' +8 set_case_expr (19) 1 j@1 +9 jump_if_not 12(19) (case_expr@1 = 1) +10 set str_j@3 _latin1'1' +11 jump 19 +12 jump_if_not 15(19) (case_expr@1 = 2) +13 set str_j@3 _latin1'2' +14 jump 19 +15 jump_if_not 18(19) (case_expr@1 = 3) +16 set str_j@3 _latin1'3' +17 jump 19 +18 set str_j@3 _latin1'unknown' +19 stmt 0 "select "i was 20"" +20 jump 28 +21 jump_if_not 24(28) (case_expr@0 = 30) +22 set str_i@2 _latin1'30' +23 jump 28 +24 jump_if_not 27(28) (case_expr@0 = 40) +25 set str_i@2 _latin1'40' +26 jump 28 +27 set str_i@2 _latin1'unknown' +28 stmt 0 "SELECT str_i, str_j" +SHOW PROCEDURE CODE proc_19194_nested_4; +Pos Instruction +0 set str_i@2 NULL +1 set str_j@3 NULL +2 jump_if_not 5(26) (i@0 = 10) +3 set str_i@2 _latin1'10' +4 jump 26 +5 jump_if_not 19(26) (i@0 = 20) +6 set str_i@2 _latin1'20' +7 jump_if_not 10(17) (j@1 = 1) +8 set str_j@3 _latin1'1' +9 jump 17 +10 jump_if_not 13(17) (j@1 = 2) +11 set str_j@3 _latin1'2' +12 jump 17 +13 jump_if_not 16(17) (j@1 = 3) +14 set str_j@3 _latin1'3' +15 jump 17 +16 set str_j@3 _latin1'unknown' +17 stmt 0 "select "i was 20"" +18 jump 26 +19 jump_if_not 22(26) (i@0 = 30) +20 set str_i@2 _latin1'30' +21 jump 26 +22 jump_if_not 25(26) (i@0 = 40) +23 set str_i@2 _latin1'40' +24 jump 26 +25 set str_i@2 _latin1'unknown' +26 stmt 0 "SELECT str_i, str_j" +CALL proc_19194_nested_1(10, 1); +str_i str_j +10 NULL +CALL proc_19194_nested_1(25, 1); +str_i str_j +unknown NULL +CALL proc_19194_nested_1(20, 1); +i was 20 +i was 20 +str_i str_j +20 1 +CALL proc_19194_nested_1(20, 2); +i was 20 +i was 20 +str_i str_j +20 2 +CALL proc_19194_nested_1(20, 3); +i was 20 +i was 20 +str_i str_j +20 3 +CALL proc_19194_nested_1(20, 4); +i was 20 +i was 20 +str_i str_j +20 unknown +CALL proc_19194_nested_1(30, 1); +str_i str_j +30 NULL +CALL proc_19194_nested_1(40, 1); +str_i str_j +40 NULL +CALL proc_19194_nested_1(0, 0); +str_i str_j +unknown NULL +CALL proc_19194_nested_2(10, 1); +str_i str_j +10 NULL +CALL proc_19194_nested_2(25, 1); +str_i str_j +unknown NULL +CALL proc_19194_nested_2(20, 1); +i was 20 +i was 20 +str_i str_j +20 1 +CALL proc_19194_nested_2(20, 2); +i was 20 +i was 20 +str_i str_j +20 2 +CALL proc_19194_nested_2(20, 3); +i was 20 +i was 20 +str_i str_j +20 3 +CALL proc_19194_nested_2(20, 4); +i was 20 +i was 20 +str_i str_j +20 unknown +CALL proc_19194_nested_2(30, 1); +str_i str_j +30 NULL +CALL proc_19194_nested_2(40, 1); +str_i str_j +40 NULL +CALL proc_19194_nested_2(0, 0); +str_i str_j +unknown NULL +CALL proc_19194_nested_3(10, 1); +str_i str_j +10 NULL +CALL proc_19194_nested_3(25, 1); +str_i str_j +unknown NULL +CALL proc_19194_nested_3(20, 1); +i was 20 +i was 20 +str_i str_j +20 1 +CALL proc_19194_nested_3(20, 2); +i was 20 +i was 20 +str_i str_j +20 2 +CALL proc_19194_nested_3(20, 3); +i was 20 +i was 20 +str_i str_j +20 3 +CALL proc_19194_nested_3(20, 4); +i was 20 +i was 20 +str_i str_j +20 unknown +CALL proc_19194_nested_3(30, 1); +str_i str_j +30 NULL +CALL proc_19194_nested_3(40, 1); +str_i str_j +40 NULL +CALL proc_19194_nested_3(0, 0); +str_i str_j +unknown NULL +CALL proc_19194_nested_4(10, 1); +str_i str_j +10 NULL +CALL proc_19194_nested_4(25, 1); +str_i str_j +unknown NULL +CALL proc_19194_nested_4(20, 1); +i was 20 +i was 20 +str_i str_j +20 1 +CALL proc_19194_nested_4(20, 2); +i was 20 +i was 20 +str_i str_j +20 2 +CALL proc_19194_nested_4(20, 3); +i was 20 +i was 20 +str_i str_j +20 3 +CALL proc_19194_nested_4(20, 4); +i was 20 +i was 20 +str_i str_j +20 unknown +CALL proc_19194_nested_4(30, 1); +str_i str_j +30 NULL +CALL proc_19194_nested_4(40, 1); +str_i str_j +40 NULL +CALL proc_19194_nested_4(0, 0); +str_i str_j +unknown NULL +DROP PROCEDURE proc_19194_simple; +DROP PROCEDURE proc_19194_searched; +DROP PROCEDURE proc_19194_nested_1; +DROP PROCEDURE proc_19194_nested_2; +DROP PROCEDURE proc_19194_nested_3; +DROP PROCEDURE proc_19194_nested_4; DROP PROCEDURE IF EXISTS p1; CREATE PROCEDURE p1() CREATE INDEX idx ON t1 (c1); SHOW PROCEDURE CODE p1; diff --git a/mysql-test/r/sp_stress_case.result b/mysql-test/r/sp_stress_case.result new file mode 100644 index 00000000000..b85fb41d0bf --- /dev/null +++ b/mysql-test/r/sp_stress_case.result @@ -0,0 +1,42 @@ +DROP PROCEDURE IF EXISTS bug_19194_a; +DROP PROCEDURE IF EXISTS bug_19194_b; +'Silently creating PROCEDURE bug_19194_a' +'Silently creating PROCEDURE bug_19194_b' +CALL bug_19194_a(1); +str +1 +CALL bug_19194_a(2); +str +2 +CALL bug_19194_a(1000); +str +1000 +CALL bug_19194_a(4998); +str +4998 +CALL bug_19194_a(4999); +str +4999 +CALL bug_19194_a(9999); +str +unknown +CALL bug_19194_b(1); +str +1 +CALL bug_19194_b(2); +str +2 +CALL bug_19194_b(1000); +str +1000 +CALL bug_19194_b(4998); +str +4998 +CALL bug_19194_b(4999); +str +4999 +CALL bug_19194_b(9999); +str +unknown +DROP PROCEDURE bug_19194_a; +DROP PROCEDURE bug_19194_b; diff --git a/mysql-test/t/sp-code.test b/mysql-test/t/sp-code.test index 72efa831059..97bc29fcad2 100644 --- a/mysql-test/t/sp-code.test +++ b/mysql-test/t/sp-code.test @@ -191,6 +191,241 @@ show procedure code sudoku_solve; drop procedure sudoku_solve; +# +# Bug#19194 (Right recursion in parser for CASE causes excessive stack +# usage, limitation) +# This bug also exposed a flaw in the generated code with nested case +# statements +# + +--disable_warnings +DROP PROCEDURE IF EXISTS proc_19194_simple; +DROP PROCEDURE IF EXISTS proc_19194_searched; +DROP PROCEDURE IF EXISTS proc_19194_nested_1; +DROP PROCEDURE IF EXISTS proc_19194_nested_2; +DROP PROCEDURE IF EXISTS proc_19194_nested_3; +DROP PROCEDURE IF EXISTS proc_19194_nested_4; +--enable_warnings + +delimiter |; + +CREATE PROCEDURE proc_19194_simple(i int) +BEGIN + DECLARE str CHAR(10); + + CASE i + WHEN 1 THEN SET str="1"; + WHEN 2 THEN SET str="2"; + WHEN 3 THEN SET str="3"; + ELSE SET str="unknown"; + END CASE; + + SELECT str; +END| + +CREATE PROCEDURE proc_19194_searched(i int) +BEGIN + DECLARE str CHAR(10); + + CASE + WHEN i=1 THEN SET str="1"; + WHEN i=2 THEN SET str="2"; + WHEN i=3 THEN SET str="3"; + ELSE SET str="unknown"; + END CASE; + + SELECT str; +END| + +# Outer SIMPLE case, inner SEARCHED case +CREATE PROCEDURE proc_19194_nested_1(i int, j int) +BEGIN + DECLARE str_i CHAR(10); + DECLARE str_j CHAR(10); + + CASE i + WHEN 10 THEN SET str_i="10"; + WHEN 20 THEN + BEGIN + set str_i="20"; + CASE + WHEN j=1 THEN SET str_j="1"; + WHEN j=2 THEN SET str_j="2"; + WHEN j=3 THEN SET str_j="3"; + ELSE SET str_j="unknown"; + END CASE; + select "i was 20"; + END; + WHEN 30 THEN SET str_i="30"; + WHEN 40 THEN SET str_i="40"; + ELSE SET str_i="unknown"; + END CASE; + + SELECT str_i, str_j; +END| + +# Outer SEARCHED case, inner SIMPLE case +CREATE PROCEDURE proc_19194_nested_2(i int, j int) +BEGIN + DECLARE str_i CHAR(10); + DECLARE str_j CHAR(10); + + CASE + WHEN i=10 THEN SET str_i="10"; + WHEN i=20 THEN + BEGIN + set str_i="20"; + CASE j + WHEN 1 THEN SET str_j="1"; + WHEN 2 THEN SET str_j="2"; + WHEN 3 THEN SET str_j="3"; + ELSE SET str_j="unknown"; + END CASE; + select "i was 20"; + END; + WHEN i=30 THEN SET str_i="30"; + WHEN i=40 THEN SET str_i="40"; + ELSE SET str_i="unknown"; + END CASE; + + SELECT str_i, str_j; +END| + +# Outer SIMPLE case, inner SIMPLE case +CREATE PROCEDURE proc_19194_nested_3(i int, j int) +BEGIN + DECLARE str_i CHAR(10); + DECLARE str_j CHAR(10); + + CASE i + WHEN 10 THEN SET str_i="10"; + WHEN 20 THEN + BEGIN + set str_i="20"; + CASE j + WHEN 1 THEN SET str_j="1"; + WHEN 2 THEN SET str_j="2"; + WHEN 3 THEN SET str_j="3"; + ELSE SET str_j="unknown"; + END CASE; + select "i was 20"; + END; + WHEN 30 THEN SET str_i="30"; + WHEN 40 THEN SET str_i="40"; + ELSE SET str_i="unknown"; + END CASE; + + SELECT str_i, str_j; +END| + +# Outer SEARCHED case, inner SEARCHED case +CREATE PROCEDURE proc_19194_nested_4(i int, j int) +BEGIN + DECLARE str_i CHAR(10); + DECLARE str_j CHAR(10); + + CASE + WHEN i=10 THEN SET str_i="10"; + WHEN i=20 THEN + BEGIN + set str_i="20"; + CASE + WHEN j=1 THEN SET str_j="1"; + WHEN j=2 THEN SET str_j="2"; + WHEN j=3 THEN SET str_j="3"; + ELSE SET str_j="unknown"; + END CASE; + select "i was 20"; + END; + WHEN i=30 THEN SET str_i="30"; + WHEN i=40 THEN SET str_i="40"; + ELSE SET str_i="unknown"; + END CASE; + + SELECT str_i, str_j; +END| + +delimiter ;| + +SHOW PROCEDURE CODE proc_19194_simple; +SHOW PROCEDURE CODE proc_19194_searched; +SHOW PROCEDURE CODE proc_19194_nested_1; +SHOW PROCEDURE CODE proc_19194_nested_2; +SHOW PROCEDURE CODE proc_19194_nested_3; +SHOW PROCEDURE CODE proc_19194_nested_4; + +CALL proc_19194_nested_1(10, 1); + +# +# Before 19194, the generated code was: +# 20 jump_if_not 23(27) 30 +# 21 set str_i@2 _latin1'30' +# As opposed to the expected: +# 20 jump_if_not 23(27) (case_expr@0 = 30) +# 21 set str_i@2 _latin1'30' +# +# and as a result, this call returned "30", +# because the expression 30 is always true, +# masking the case 40, case 0 and the else. +# +CALL proc_19194_nested_1(25, 1); + +CALL proc_19194_nested_1(20, 1); +CALL proc_19194_nested_1(20, 2); +CALL proc_19194_nested_1(20, 3); +CALL proc_19194_nested_1(20, 4); +CALL proc_19194_nested_1(30, 1); +CALL proc_19194_nested_1(40, 1); +CALL proc_19194_nested_1(0, 0); + +CALL proc_19194_nested_2(10, 1); + +# +# Before 19194, the generated code was: +# 20 jump_if_not 23(27) (case_expr@0 = (i@0 = 30)) +# 21 set str_i@2 _latin1'30' +# As opposed to the expected: +# 20 jump_if_not 23(27) (i@0 = 30) +# 21 set str_i@2 _latin1'30' +# and as a result, this call crashed the server, because there is no +# such variable as "case_expr@0". +# +CALL proc_19194_nested_2(25, 1); + +CALL proc_19194_nested_2(20, 1); +CALL proc_19194_nested_2(20, 2); +CALL proc_19194_nested_2(20, 3); +CALL proc_19194_nested_2(20, 4); +CALL proc_19194_nested_2(30, 1); +CALL proc_19194_nested_2(40, 1); +CALL proc_19194_nested_2(0, 0); + +CALL proc_19194_nested_3(10, 1); +CALL proc_19194_nested_3(25, 1); +CALL proc_19194_nested_3(20, 1); +CALL proc_19194_nested_3(20, 2); +CALL proc_19194_nested_3(20, 3); +CALL proc_19194_nested_3(20, 4); +CALL proc_19194_nested_3(30, 1); +CALL proc_19194_nested_3(40, 1); +CALL proc_19194_nested_3(0, 0); + +CALL proc_19194_nested_4(10, 1); +CALL proc_19194_nested_4(25, 1); +CALL proc_19194_nested_4(20, 1); +CALL proc_19194_nested_4(20, 2); +CALL proc_19194_nested_4(20, 3); +CALL proc_19194_nested_4(20, 4); +CALL proc_19194_nested_4(30, 1); +CALL proc_19194_nested_4(40, 1); +CALL proc_19194_nested_4(0, 0); + +DROP PROCEDURE proc_19194_simple; +DROP PROCEDURE proc_19194_searched; +DROP PROCEDURE proc_19194_nested_1; +DROP PROCEDURE proc_19194_nested_2; +DROP PROCEDURE proc_19194_nested_3; +DROP PROCEDURE proc_19194_nested_4; # # Bug#19207: Final parenthesis omitted for CREATE INDEX in Stored diff --git a/mysql-test/t/sp_stress_case.sh b/mysql-test/t/sp_stress_case.sh new file mode 100755 index 00000000000..a2ac948e110 --- /dev/null +++ b/mysql-test/t/sp_stress_case.sh @@ -0,0 +1,68 @@ +#!/bin/sh + +# +# Bug#19194 (Right recursion in parser for CASE causes excessive stack +# usage, limitation) +# +# Because the code for the CASE statement is so massive, +# checking in an already generated .test is not practical, +# due to it's size (10 000 lines or 356 000 bytes). +# +# Patches are sent by email, which introduce size limitations. +# +# As a result, code is generated dynamically here. +# This script takes no argument, and generates code in stdout. +# + +cat <&1 + +CALL bug_19194_a(1); +CALL bug_19194_a(2); +CALL bug_19194_a(1000); +CALL bug_19194_a(4998); +CALL bug_19194_a(4999); +CALL bug_19194_a(9999); + +CALL bug_19194_b(1); +CALL bug_19194_b(2); +CALL bug_19194_b(1000); +CALL bug_19194_b(4998); +CALL bug_19194_b(4999); +CALL bug_19194_b(9999); + +DROP PROCEDURE bug_19194_a; +DROP PROCEDURE bug_19194_b; + diff --git a/sql/sp_head.cc b/sql/sp_head.cc index 9761de625be..689922cfa37 100644 --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -603,27 +603,6 @@ sp_head::create(THD *thd) DBUG_PRINT("info", ("type: %d name: %s params: %s body: %s", m_type, m_name.str, m_params.str, m_body.str)); -#ifndef DBUG_OFF - optimize(); - { - String s; - sp_instr *i; - uint ip= 0; - while ((i = get_instr(ip))) - { - char buf[8]; - - sprintf(buf, "%4u: ", ip); - s.append(buf); - i->print(&s); - s.append('\n'); - ip+= 1; - } - s.append('\0'); - DBUG_PRINT("info", ("Code %s\n%s", m_qname.str, s.ptr())); - } -#endif - if (m_type == TYPE_ENUM_FUNCTION) ret= sp_create_function(thd, this); else @@ -2172,7 +2151,7 @@ sp_head::show_create_function(THD *thd) This is the main mark and move loop; it relies on the following methods in sp_instr and its subclasses: - opt_mark() Mark instruction as reachable (will recurse for jumps) + opt_mark() Mark instruction as reachable opt_shortcut_jump() Shortcut jumps to the final destination; used by opt_mark(). opt_move() Update moved instruction @@ -2185,7 +2164,7 @@ void sp_head::optimize() sp_instr *i; uint src, dst; - opt_mark(0); + opt_mark(); bp.empty(); src= dst= 0; @@ -2219,13 +2198,50 @@ void sp_head::optimize() bp.empty(); } -void -sp_head::opt_mark(uint ip) +void sp_head::add_mark_lead(uint ip, List *leads) { - sp_instr *i; + sp_instr *i= get_instr(ip); - while ((i= get_instr(ip)) && !i->marked) - ip= i->opt_mark(this); + if (i && ! i->marked) + leads->push_front(i); +} + +void +sp_head::opt_mark() +{ + uint ip; + sp_instr *i; + List leads; + + /* + Forward flow analysis algorithm in the instruction graph: + - first, add the entry point in the graph (the first instruction) to the + 'leads' list of paths to explore. + - while there are still leads to explore: + - pick one lead, and follow the path forward. Mark instruction reached. + Stop only if the end of the routine is reached, or the path converge + to code already explored (marked). + - while following a path, collect in the 'leads' list any fork to + another path (caused by conditional jumps instructions), so that these + paths can be explored as well. + */ + + /* Add the entry point */ + i= get_instr(0); + leads.push_front(i); + + /* For each path of code ... */ + while (leads.elements != 0) + { + i= leads.pop(); + + /* Mark the entire path, collecting new leads. */ + while (i && ! i->marked) + { + ip= i->opt_mark(this, & leads); + i= get_instr(ip); + } + } } @@ -2618,7 +2634,7 @@ sp_instr_jump::print(String *str) } uint -sp_instr_jump::opt_mark(sp_head *sp) +sp_instr_jump::opt_mark(sp_head *sp, List *leads) { m_dest= opt_shortcut_jump(sp, this); if (m_dest != m_ip+1) /* Jumping to following instruction? */ @@ -2712,7 +2728,7 @@ sp_instr_jump_if_not::print(String *str) uint -sp_instr_jump_if_not::opt_mark(sp_head *sp) +sp_instr_jump_if_not::opt_mark(sp_head *sp, List *leads) { sp_instr *i; @@ -2722,13 +2738,13 @@ sp_instr_jump_if_not::opt_mark(sp_head *sp) m_dest= i->opt_shortcut_jump(sp, this); m_optdest= sp->get_instr(m_dest); } - sp->opt_mark(m_dest); + sp->add_mark_lead(m_dest, leads); if ((i= sp->get_instr(m_cont_dest))) { m_cont_dest= i->opt_shortcut_jump(sp, this); m_cont_optdest= sp->get_instr(m_cont_dest); } - sp->opt_mark(m_cont_dest); + sp->add_mark_lead(m_cont_dest, leads); return m_ip+1; } @@ -2849,7 +2865,7 @@ sp_instr_hpush_jump::print(String *str) uint -sp_instr_hpush_jump::opt_mark(sp_head *sp) +sp_instr_hpush_jump::opt_mark(sp_head *sp, List *leads) { sp_instr *i; @@ -2859,7 +2875,7 @@ sp_instr_hpush_jump::opt_mark(sp_head *sp) m_dest= i->opt_shortcut_jump(sp, this); m_optdest= sp->get_instr(m_dest); } - sp->opt_mark(m_dest); + sp->add_mark_lead(m_dest, leads); return m_ip+1; } @@ -2924,15 +2940,13 @@ sp_instr_hreturn::print(String *str) uint -sp_instr_hreturn::opt_mark(sp_head *sp) +sp_instr_hreturn::opt_mark(sp_head *sp, List *leads) { if (m_dest) - return sp_instr_jump::opt_mark(sp); - else - { - marked= 1; - return UINT_MAX; - } + return sp_instr_jump::opt_mark(sp, leads); + + marked= 1; + return UINT_MAX; } @@ -3275,7 +3289,7 @@ sp_instr_set_case_expr::print(String *str) } uint -sp_instr_set_case_expr::opt_mark(sp_head *sp) +sp_instr_set_case_expr::opt_mark(sp_head *sp, List *leads) { sp_instr *i; @@ -3285,7 +3299,7 @@ sp_instr_set_case_expr::opt_mark(sp_head *sp) m_cont_dest= i->opt_shortcut_jump(sp, this); m_cont_optdest= sp->get_instr(m_cont_dest); } - sp->opt_mark(m_cont_dest); + sp->add_mark_lead(m_cont_dest, leads); return m_ip+1; } diff --git a/sql/sp_head.h b/sql/sp_head.h index 7f2da69aa0c..6a377e6f188 100644 --- a/sql/sp_head.h +++ b/sql/sp_head.h @@ -302,8 +302,19 @@ public: void restore_thd_mem_root(THD *thd); + /** + Optimize the code. + */ void optimize(); - void opt_mark(uint ip); + + /** + Helper used during flow analysis during code optimization. + See the implementation of opt_mark(). + @param ip the instruction to add to the leads list + @param leads the list of remaining paths to explore in the graph that + represents the code, during flow analysis. + */ + void add_mark_lead(uint ip, List *leads); void recursion_level_error(THD *thd); @@ -393,6 +404,12 @@ private: bool execute(THD *thd); + /** + Perform a forward flow analysis in the generated code. + Mark reachable instructions, for the optimizer. + */ + void opt_mark(); + /* Merge the list of tables used by query into the multi-set of tables used by routine. @@ -460,10 +477,10 @@ public: /* Mark this instruction as reachable during optimization and return the - index to the next instruction. Jump instruction will mark their - destination too recursively. + index to the next instruction. Jump instruction will add their + destination to the leads list. */ - virtual uint opt_mark(sp_head *sp) + virtual uint opt_mark(sp_head *sp, List *leads) { marked= 1; return m_ip+1; @@ -735,7 +752,7 @@ public: virtual void print(String *str); - virtual uint opt_mark(sp_head *sp); + virtual uint opt_mark(sp_head *sp, List *leads); virtual uint opt_shortcut_jump(sp_head *sp, sp_instr *start); @@ -785,7 +802,7 @@ public: virtual void print(String *str); - virtual uint opt_mark(sp_head *sp); + virtual uint opt_mark(sp_head *sp, List *leads); /* Override sp_instr_jump's shortcut; we stop here */ virtual uint opt_shortcut_jump(sp_head *sp, sp_instr *start) @@ -831,7 +848,7 @@ public: virtual void print(String *str); - virtual uint opt_mark(sp_head *sp) + virtual uint opt_mark(sp_head *sp, List *leads) { marked= 1; return UINT_MAX; @@ -868,7 +885,7 @@ public: virtual void print(String *str); - virtual uint opt_mark(sp_head *sp); + virtual uint opt_mark(sp_head *sp, List *leads); /* Override sp_instr_jump's shortcut; we stop here. */ virtual uint opt_shortcut_jump(sp_head *sp, sp_instr *start) @@ -933,7 +950,7 @@ public: virtual void print(String *str); - virtual uint opt_mark(sp_head *sp); + virtual uint opt_mark(sp_head *sp, List *leads); private: @@ -1103,7 +1120,7 @@ public: virtual void print(String *str); - virtual uint opt_mark(sp_head *sp) + virtual uint opt_mark(sp_head *sp, List *leads) { marked= 1; return UINT_MAX; @@ -1136,7 +1153,7 @@ public: virtual void print(String *str); - virtual uint opt_mark(sp_head *sp); + virtual uint opt_mark(sp_head *sp, List *leads); virtual void opt_move(uint dst, List *ibp); diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 405f576ac04..71e7d1cf86f 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -1163,7 +1163,6 @@ void st_select_lex::init_select() options= 0; sql_cache= SQL_CACHE_UNSPECIFIED; braces= 0; - when_list.empty(); expr_list.empty(); interval_list.empty(); use_index.empty(); diff --git a/sql/sql_lex.h b/sql/sql_lex.h index 378f968118e..8f4b021ca00 100644 --- a/sql/sql_lex.h +++ b/sql/sql_lex.h @@ -518,7 +518,6 @@ public: SQL_LIST order_list; /* ORDER clause */ List expr_list; - List when_list; /* WHEN clause (expression) */ SQL_LIST *gorder_list; Item *select_limit, *offset_limit; /* LIMIT clause parameters */ // Arrays of pointers to top elements of all_fields list diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 6f24a42c07c..57b6201d4a9 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -96,6 +96,146 @@ void turn_parser_debug_on() } #endif + +/** + Helper action for a case statement (entering the CASE). + This helper is used for both 'simple' and 'searched' cases. + @param lex the parser lex context +*/ + +void case_stmt_action_case(LEX *lex) +{ + lex->sphead->new_cont_backpatch(NULL); + + /* + BACKPATCH: Creating target label for the jump to + "case_stmt_action_end_case" + */ + + lex->spcont->push_label((char *)"", lex->sphead->instructions()); +} + +/** + Helper action for a case expression statement (the expr in 'CASE expr'). + This helper is used for 'searched' cases only. + @param lex the parser lex context + @param expr the parsed expression + @return 0 on success +*/ + +int case_stmt_action_expr(LEX *lex, Item* expr) +{ + sp_head *sp= lex->sphead; + sp_pcontext *parsing_ctx= lex->spcont; + int case_expr_id= parsing_ctx->register_case_expr(); + sp_instr_set_case_expr *i; + + if (parsing_ctx->push_case_expr_id(case_expr_id)) + return 1; + + i= new sp_instr_set_case_expr(sp->instructions(), + parsing_ctx, case_expr_id, expr, lex); + + sp->add_cont_backpatch(i); + sp->add_instr(i); + + return 0; +} + +/** + Helper action for a case when condition. + This helper is used for both 'simple' and 'searched' cases. + @param lex the parser lex context + @param when the parsed expression for the WHEN clause + @param simple true for simple cases, false for searched cases +*/ + +void case_stmt_action_when(LEX *lex, Item *when, bool simple) +{ + sp_head *sp= lex->sphead; + sp_pcontext *ctx= lex->spcont; + uint ip= sp->instructions(); + sp_instr_jump_if_not *i; + Item_case_expr *var; + Item *expr; + + if (simple) + { + var= new Item_case_expr(ctx->get_current_case_expr_id()); + +#ifndef DBUG_OFF + if (var) + { + var->m_sp= sp; + } +#endif + + expr= new Item_func_eq(var, when); + i= new sp_instr_jump_if_not(ip, ctx, expr, lex); + } + else + i= new sp_instr_jump_if_not(ip, ctx, when, lex); + + /* + BACKPATCH: Registering forward jump from + "case_stmt_action_when" to "case_stmt_action_then" + */ + + sp->push_backpatch(i, ctx->push_label((char *)"", 0)); + sp->add_cont_backpatch(i); + sp->add_instr(i); +} + +/** + Helper action for a case then statements. + This helper is used for both 'simple' and 'searched' cases. + @param lex the parser lex context +*/ + +void case_stmt_action_then(LEX *lex) +{ + sp_head *sp= lex->sphead; + sp_pcontext *ctx= lex->spcont; + uint ip= sp->instructions(); + sp_instr_jump *i = new sp_instr_jump(ip, ctx); + sp->add_instr(i); + + /* + BACKPATCH: Resolving forward jump from + "case_stmt_action_when" to "case_stmt_action_then" + */ + + sp->backpatch(ctx->pop_label()); + + /* + BACKPATCH: Registering forward jump from + "case_stmt_action_when" to "case_stmt_action_end_case" + */ + + sp->push_backpatch(i, ctx->last_label()); +} + +/** + Helper action for an end case. + This helper is used for both 'simple' and 'searched' cases. + @param lex the parser lex context + @param simple true for simple cases, false for searched cases +*/ + +void case_stmt_action_end_case(LEX *lex, bool simple) +{ + /* + BACKPATCH: Resolving forward jump from + "case_stmt_action_then" to "case_stmt_action_end_case" + */ + lex->sphead->backpatch(lex->spcont->pop_label()); + + if (simple) + lex->spcont->pop_case_expr_id(); + + lex->sphead->do_cont_backpatch(); +} + %} %union { int num; @@ -832,7 +972,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); select_item_list select_item values_list no_braces opt_limit_clause delete_limit_clause fields opt_values values procedure_list procedure_list2 procedure_item - when_list2 expr_list2 udf_expr_list3 handler + expr_list2 udf_expr_list3 handler opt_precision opt_ignore opt_column opt_restrict grant revoke set lock unlock string_list field_options field_option field_opt_list opt_binary table_lock_list table_lock @@ -860,6 +1000,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); view_algorithm view_or_trigger_or_sp view_or_trigger_or_sp_tail view_suid view_tail view_list_opt view_list view_select view_check_option trigger_tail sp_tail + case_stmt_specification simple_case_stmt searched_case_stmt END_OF_INPUT %type call sp_proc_stmts sp_proc_stmts1 sp_proc_stmt @@ -1995,43 +2136,7 @@ sp_proc_stmt: { Lex->sphead->new_cont_backpatch(NULL); } sp_if END IF { Lex->sphead->do_cont_backpatch(); } - | CASE_SYM WHEN_SYM - { - Lex->sphead->m_flags&= ~sp_head::IN_SIMPLE_CASE; - Lex->sphead->new_cont_backpatch(NULL); - } - sp_case END CASE_SYM { Lex->sphead->do_cont_backpatch(); } - | CASE_SYM - { - Lex->sphead->reset_lex(YYTHD); - Lex->sphead->new_cont_backpatch(NULL); - } - expr WHEN_SYM - { - LEX *lex= Lex; - sp_head *sp= lex->sphead; - sp_pcontext *parsing_ctx= lex->spcont; - int case_expr_id= parsing_ctx->register_case_expr(); - sp_instr_set_case_expr *i; - - if (parsing_ctx->push_case_expr_id(case_expr_id)) - YYABORT; - - i= new sp_instr_set_case_expr(sp->instructions(), - parsing_ctx, - case_expr_id, - $3, - lex); - sp->add_cont_backpatch(i); - sp->add_instr(i); - sp->m_flags|= sp_head::IN_SIMPLE_CASE; - sp->restore_lex(YYTHD); - } - sp_case END CASE_SYM - { - Lex->spcont->pop_case_expr_id(); - Lex->sphead->do_cont_backpatch(); - } + | case_stmt_specification | sp_labeled_control {} | { /* Unlabeled controls get a secret label. */ @@ -2242,72 +2347,114 @@ sp_elseifs: | ELSE sp_proc_stmts1 ; -sp_case: - { Lex->sphead->reset_lex(YYTHD); } - expr THEN_SYM - { +case_stmt_specification: + simple_case_stmt + | searched_case_stmt + ; + +simple_case_stmt: + CASE_SYM + { LEX *lex= Lex; - sp_head *sp= lex->sphead; - sp_pcontext *ctx= Lex->spcont; - uint ip= sp->instructions(); - sp_instr_jump_if_not *i; + case_stmt_action_case(lex); + lex->sphead->reset_lex(YYTHD); /* For expr $3 */ + } + expr + { + LEX *lex= Lex; + if (case_stmt_action_expr(lex, $3)) + YYABORT; - if (! (sp->m_flags & sp_head::IN_SIMPLE_CASE)) - i= new sp_instr_jump_if_not(ip, ctx, $2, lex); - else - { /* Simple case: = */ + lex->sphead->restore_lex(YYTHD); /* For expr $3 */ + } + simple_when_clause_list + else_clause_opt + END + CASE_SYM + { + LEX *lex= Lex; + case_stmt_action_end_case(lex, true); + } + ; - Item_case_expr *var; - Item *expr; +searched_case_stmt: + CASE_SYM + { + LEX *lex= Lex; + case_stmt_action_case(lex); + } + searched_when_clause_list + else_clause_opt + END + CASE_SYM + { + LEX *lex= Lex; + case_stmt_action_end_case(lex, false); + } + ; - var= new Item_case_expr(ctx->get_current_case_expr_id()); +simple_when_clause_list: + simple_when_clause + | simple_when_clause_list simple_when_clause + ; -#ifndef DBUG_OFF - if (var) - var->m_sp= sp; -#endif +searched_when_clause_list: + searched_when_clause + | searched_when_clause_list searched_when_clause + ; - expr= new Item_func_eq(var, $2); +simple_when_clause: + WHEN_SYM + { + Lex->sphead->reset_lex(YYTHD); /* For expr $3 */ + } + expr + { + /* Simple case: = */ - i= new sp_instr_jump_if_not(ip, ctx, expr, lex); - } - sp->push_backpatch(i, ctx->push_label((char *)"", 0)); - sp->add_cont_backpatch(i); + LEX *lex= Lex; + case_stmt_action_when(lex, $3, true); + lex->sphead->restore_lex(YYTHD); /* For expr $3 */ + } + THEN_SYM + sp_proc_stmts1 + { + LEX *lex= Lex; + case_stmt_action_then(lex); + } + ; + +searched_when_clause: + WHEN_SYM + { + Lex->sphead->reset_lex(YYTHD); /* For expr $3 */ + } + expr + { + LEX *lex= Lex; + case_stmt_action_when(lex, $3, false); + lex->sphead->restore_lex(YYTHD); /* For expr $3 */ + } + THEN_SYM + sp_proc_stmts1 + { + LEX *lex= Lex; + case_stmt_action_then(lex); + } + ; + +else_clause_opt: + /* empty */ + { + LEX *lex= Lex; + sp_head *sp= lex->sphead; + uint ip= sp->instructions(); + sp_instr_error *i= new sp_instr_error(ip, lex->spcont, + ER_SP_CASE_NOT_FOUND); sp->add_instr(i); - sp->restore_lex(YYTHD); - } - sp_proc_stmts1 - { - sp_head *sp= Lex->sphead; - sp_pcontext *ctx= Lex->spcont; - uint ip= sp->instructions(); - sp_instr_jump *i = new sp_instr_jump(ip, ctx); - - sp->add_instr(i); - sp->backpatch(ctx->pop_label()); - sp->push_backpatch(i, ctx->push_label((char *)"", 0)); - } - sp_whens - { - LEX *lex= Lex; - - lex->sphead->backpatch(lex->spcont->pop_label()); - } - ; - -sp_whens: - /* Empty */ - { - sp_head *sp= Lex->sphead; - uint ip= sp->instructions(); - sp_instr_error *i= new sp_instr_error(ip, Lex->spcont, - ER_SP_CASE_NOT_FOUND); - - sp->add_instr(i); - } - | ELSE sp_proc_stmts1 {} - | WHEN_SYM sp_case {} - ; + } + | ELSE sp_proc_stmts1 + ; sp_labeled_control: label_ident ':' @@ -4374,8 +4521,8 @@ simple_expr: if (!$$) YYABORT; } - | CASE_SYM opt_expr WHEN_SYM when_list opt_else END - { $$= new Item_func_case(* $4, $2, $5 ); } + | CASE_SYM opt_expr when_list opt_else END + { $$= new Item_func_case(* $3, $2, $4 ); } | CONVERT_SYM '(' expr ',' cast_type ')' { $$= create_func_cast($3, $5, @@ -5162,23 +5309,19 @@ opt_else: | ELSE expr { $$= $2; }; when_list: - { Select->when_list.push_front(new List); } - when_list2 - { $$= Select->when_list.pop(); }; - -when_list2: - expr THEN_SYM expr - { - SELECT_LEX *sel=Select; - sel->when_list.head()->push_back($1); - sel->when_list.head()->push_back($3); - } - | when_list2 WHEN_SYM expr THEN_SYM expr - { - SELECT_LEX *sel=Select; - sel->when_list.head()->push_back($3); - sel->when_list.head()->push_back($5); - }; + WHEN_SYM expr THEN_SYM expr + { + $$= new List; + $$->push_back($2); + $$->push_back($4); + } + | when_list WHEN_SYM expr THEN_SYM expr + { + $1->push_back($3); + $1->push_back($5); + $$= $1; + } + ; /* Warning - may return NULL in case of incomplete SELECT */ table_ref: From 070f5ad497109dc7505c50b86f51ff684ebe993f Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Mon, 20 Nov 2006 20:40:35 -0700 Subject: [PATCH 04/23] WL#3602 (SET GLOBAL READONLY) Bug#11733 (COMMITs should not happen if read-only is set) Bug#22009 (Can write to a read-only server under some circumstances) See the work log for details The change consist of a) acquiring the global read lock in SET GLOBAL READONLY b) honoring opt_readonly in ha_commit_trans(), c) honoring opt_readonly in mysql_lock_tables(). a) takes care of the server stability, b) makes the transactional tables safe (Bug 11733) c) makes the non transactional tables safe (Bug 22009) --- mysql-test/r/read_only.result | 52 ++++++++++++- mysql-test/r/read_only_innodb.result | 18 +++++ mysql-test/t/read_only.test | 110 ++++++++++++++++++++++++++- mysql-test/t/read_only_innodb.test | 43 +++++++++++ sql/handler.cc | 9 +++ sql/lock.cc | 13 ++++ sql/set_var.cc | 66 +++++++++++++++- sql/set_var.h | 14 ++++ 8 files changed, 321 insertions(+), 4 deletions(-) create mode 100644 mysql-test/r/read_only_innodb.result create mode 100644 mysql-test/t/read_only_innodb.test diff --git a/mysql-test/r/read_only.result b/mysql-test/r/read_only.result index 1a1991a6255..337143ce414 100644 --- a/mysql-test/r/read_only.result +++ b/mysql-test/r/read_only.result @@ -39,6 +39,56 @@ delete t1 from t1,t3 where t1.a=t3.a; drop table t1; insert into t1 values(1); ERROR HY000: The MySQL server is running with the --read-only option so it cannot execute this statement +set global read_only=0; +lock table t1 write; +lock table t2 write; +set global read_only=1; +ERROR HY000: Can't execute the given command because you have active locked tables or an active transaction +unlock tables ; +set global read_only=1; +select @@global.read_only; +@@global.read_only +0 +unlock tables ; +select @@global.read_only; +@@global.read_only +1 +set global read_only=0; +lock table t1 read; +lock table t2 read; +set global read_only=1; +ERROR HY000: Can't execute the given command because you have active locked tables or an active transaction +unlock tables ; +set global read_only=1; +select @@global.read_only; +@@global.read_only +0 +unlock tables ; +select @@global.read_only; +@@global.read_only +1 +set global read_only=0; +BEGIN; +BEGIN; +set global read_only=1; +ERROR HY000: Can't execute the given command because you have active locked tables or an active transaction +ROLLBACK; +set global read_only=1; +select @@global.read_only; +@@global.read_only +1 +ROLLBACK; +set global read_only=0; +flush tables with read lock; +set global read_only=1; +unlock tables; +set global read_only=0; +flush tables with read lock; +set global read_only=1; +select @@global.read_only; +@@global.read_only +1 +unlock tables; +set global read_only=0; drop table t1,t2; drop user test@localhost; -set global read_only=0; diff --git a/mysql-test/r/read_only_innodb.result b/mysql-test/r/read_only_innodb.result new file mode 100644 index 00000000000..d028e3cc207 --- /dev/null +++ b/mysql-test/r/read_only_innodb.result @@ -0,0 +1,18 @@ +DROP TABLE IF EXISTS table_11733 ; +grant CREATE, SELECT, DROP on *.* to test@localhost; +set global read_only=0; +create table table_11733 (a int) engine=InnoDb; +BEGIN; +insert into table_11733 values(11733); +set global read_only=1; +select @@global.read_only; +@@global.read_only +1 +select * from table_11733 ; +a +11733 +COMMIT; +ERROR HY000: The MySQL server is running with the --read-only option so it cannot execute this statement +set global read_only=0; +drop table table_11733 ; +drop user test@localhost; diff --git a/mysql-test/t/read_only.test b/mysql-test/t/read_only.test index 175a5bba6fa..655e207e235 100644 --- a/mysql-test/t/read_only.test +++ b/mysql-test/t/read_only.test @@ -101,8 +101,114 @@ drop table t1; --error 1290 insert into t1 values(1); +# +# BUG#11733: COMMITs should not happen if read-only is set +# + +# LOCK TABLE ... WRITE / READ_ONLY +# - is an error in the same connection +# - is ok in a different connection + connection default; +set global read_only=0; +lock table t1 write; + +connection con1; +lock table t2 write; + +connection default; +--error ER_LOCK_OR_ACTIVE_TRANSACTION +set global read_only=1; +unlock tables ; +# The following call blocks until con1 releases the write lock. +# Blocking is expected. +send set global read_only=1; + +connection con1; +--sleep 1 +select @@global.read_only; +unlock tables ; +--sleep 1 +select @@global.read_only; + +connection default; +reap; + +# LOCK TABLE ... READ / READ_ONLY +# - is an error in the same connection +# - is ok in a different connection + +connection default; +set global read_only=0; +lock table t1 read; + +connection con1; +lock table t2 read; + +connection default; +--error ER_LOCK_OR_ACTIVE_TRANSACTION +set global read_only=1; +unlock tables ; +# The following call blocks until con1 releases the read lock. +# Blocking is a limitation, and could be improved. +send set global read_only=1; + +connection con1; +--sleep 1 +select @@global.read_only; +unlock tables ; +--sleep 1 +select @@global.read_only; + +connection default; +reap; + +# pending transaction / READ_ONLY +# - is an error in the same connection +# - is ok in a different connection + +connection default; +set global read_only=0; +BEGIN; + +connection con1; +BEGIN; + +connection default; +--error ER_LOCK_OR_ACTIVE_TRANSACTION +set global read_only=1; +ROLLBACK; +set global read_only=1; + +connection con1; +select @@global.read_only; +ROLLBACK; + +# Verify that FLUSH TABLES WITH READ LOCK do not block READ_ONLY +# - in the same SUPER connection +# - in another SUPER connection + +connection default; +set global read_only=0; +flush tables with read lock; +set global read_only=1; +unlock tables; + +connect (root2,localhost,root,,test); + +connection default; +set global read_only=0; +flush tables with read lock; + +connection root2; +set global read_only=1; + +connection default; +select @@global.read_only; +unlock tables; + +# Cleanup +connection default; +set global read_only=0; drop table t1,t2; drop user test@localhost; - -set global read_only=0; diff --git a/mysql-test/t/read_only_innodb.test b/mysql-test/t/read_only_innodb.test new file mode 100644 index 00000000000..76d9748aa60 --- /dev/null +++ b/mysql-test/t/read_only_innodb.test @@ -0,0 +1,43 @@ +# should work with embedded server after mysqltest is fixed +-- source include/not_embedded.inc +-- source include/have_innodb.inc + +# +# BUG#11733: COMMITs should not happen if read-only is set +# + +--disable_warnings +DROP TABLE IF EXISTS table_11733 ; +--enable_warnings + +# READ_ONLY does nothing to SUPER users +# so we use a non-SUPER one: + +grant CREATE, SELECT, DROP on *.* to test@localhost; + +connect (con1,localhost,test,,test); + +connection default; +set global read_only=0; + +# Any transactional engine will do +create table table_11733 (a int) engine=InnoDb; + +connection con1; +BEGIN; +insert into table_11733 values(11733); + +connection default; +set global read_only=1; + +connection con1; +select @@global.read_only; +select * from table_11733 ; +-- error ER_OPTION_PREVENTS_STATEMENT +COMMIT; + +connection default; +set global read_only=0; +drop table table_11733 ; +drop user test@localhost; + diff --git a/sql/handler.cc b/sql/handler.cc index 451f974a066..e97fb7ddee4 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -692,6 +692,15 @@ int ha_commit_trans(THD *thd, bool all) ha_rollback_trans(thd, all); DBUG_RETURN(1); } + + if (is_real_trans && opt_readonly) + { + my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only"); + ha_rollback_trans(thd, all); + error= 1; + goto end; + } + DBUG_EXECUTE_IF("crash_commit_before", abort();); /* Close all cursors that can not survive COMMIT */ diff --git a/sql/lock.cc b/sql/lock.cc index f36ecf58620..9886149ba77 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -151,6 +151,19 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count, } } + if (write_lock_used && opt_readonly) + { + /* + Someone has issued SET GLOBAL READ_ONLY=1 and we want a write lock. + We do not wait for READ_ONLY=0, and fail. + */ + reset_lock_data(sql_lock); + my_free((gptr) sql_lock, MYF(0)); + sql_lock=0; + my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only"); + break; + } + thd->proc_info="System lock"; DBUG_PRINT("info", ("thd->proc_info %s", thd->proc_info)); if (lock_external(thd, tables, count)) diff --git a/sql/set_var.cc b/sql/set_var.cc index 5590e71c810..4a01ab41763 100644 --- a/sql/set_var.cc +++ b/sql/set_var.cc @@ -370,7 +370,7 @@ sys_var_thd_ulong sys_preload_buff_size("preload_buffer_size", &SV::preload_buff_size); sys_var_thd_ulong sys_read_buff_size("read_buffer_size", &SV::read_buff_size); -sys_var_bool_ptr sys_readonly("read_only", &opt_readonly); +sys_var_opt_readonly sys_readonly("read_only", &opt_readonly); sys_var_thd_ulong sys_read_rnd_buff_size("read_rnd_buffer_size", &SV::read_rnd_buff_size); sys_var_thd_ulong sys_div_precincrement("div_precision_increment", @@ -3880,6 +3880,70 @@ bool sys_var_trust_routine_creators::update(THD *thd, set_var *var) return sys_var_bool_ptr::update(thd, var); } +bool sys_var_opt_readonly::update(THD *thd, set_var *var) +{ + bool result; + + DBUG_ENTER("sys_var_opt_readonly::update"); + + /* Prevent self dead-lock */ + if (thd->locked_tables || thd->active_transaction()) + { + my_error(ER_LOCK_OR_ACTIVE_TRANSACTION, MYF(0)); + DBUG_RETURN(true); + } + + if (thd->global_read_lock) + { + /* + This connection already holds the global read lock. + This can be the case with: + - FLUSH TABLES WITH READ LOCK + - SET GLOBAL READ_ONLY = 1 + */ + result= sys_var_bool_ptr::update(thd, var); + DBUG_RETURN(result); + } + + /* + Perform a 'FLUSH TABLES WITH READ LOCK'. + This is a 3 step process: + - [1] lock_global_read_lock() + - [2] close_cached_tables() + - [3] make_global_read_lock_block_commit() + [1] prevents new connections from obtaining tables locked for write. + [2] waits until all existing connections close their tables. + [3] prevents transactions from being committed. + */ + + if (lock_global_read_lock(thd)) + DBUG_RETURN(true); + + /* + This call will be blocked by any connection holding a READ or WRITE lock. + Ideally, we want to wait only for pending WRITE locks, but since: + con 1> LOCK TABLE T FOR READ; + con 2> LOCK TABLE T FOR WRITE; (blocked by con 1) + con 3> SET GLOBAL READ ONLY=1; (blocked by con 2) + can cause to wait on a read lock, it's required for the client application + to unlock everything, and acceptable for the server to wait on all locks. + */ + if (close_cached_tables(thd, true, NULL, false)) + goto end_with_read_lock; + + if (result= make_global_read_lock_block_commit(thd)) + goto end_with_read_lock; + + /* Change the opt_readonly system variable, safe because the lock is held */ + result= sys_var_bool_ptr::update(thd, var); + +end_with_read_lock: + /* Release the lock */ + unlock_global_read_lock(thd); + DBUG_RETURN(result); +} + + /* even session variable here requires SUPER, because of -#o,file */ bool sys_var_thd_dbug::check(THD *thd, set_var *var) { diff --git a/sql/set_var.h b/sql/set_var.h index 01669b378e1..14792f4a7c8 100644 --- a/sql/set_var.h +++ b/sql/set_var.h @@ -905,6 +905,20 @@ public: }; +/** + Handler for setting the system variable --read-only. +*/ + +class sys_var_opt_readonly :public sys_var_bool_ptr +{ +public: + sys_var_opt_readonly(const char *name_arg, my_bool *value_arg) : + sys_var_bool_ptr(name_arg, value_arg) {}; + ~sys_var_opt_readonly() {}; + bool update(THD *thd, set_var *var); +}; + + class sys_var_thd_lc_time_names :public sys_var_thd { public: From b534ca4bc089ff0901327a8db8daff8f27ed8dea Mon Sep 17 00:00:00 2001 From: "anozdrin/alik@booka." <> Date: Thu, 30 Nov 2006 12:23:55 +0300 Subject: [PATCH 05/23] Fix for the following bugs: - BUG#22306: STOP INSTANCE can not be applied for instances in Crashed, Failed and Abandoned; - BUG#23476: DROP INSTANCE does not work - BUG#23215: STOP INSTANCE takes too much time BUG#22306: The problem was that STOP INSTANCE checked that mysqld is up and running. If it was not so, STOP INSTANCE reported an error. Now, STOP INSTANCE reports an error if the instance has been started (mysqld can be down). BUG#23476: The problem was that DROP INSTANCE tried to stop inactive instance. The fix is trivial. BUG#23215: The problem was that locks were not acquired properly, so the instance-monitoring thread could not acquire the mutex, holded by the query-processing thread. The fix is to simplify locking scheme by moving instance-related information to Instance-class out of Guardian-class. This allows to get rid of storing a separate list of Instance-information in Guardian and keeping it synchronized with the original list in Instance_map. --- server-tools/instance-manager/commands.cc | 292 ++++---- server-tools/instance-manager/commands.h | 114 +-- server-tools/instance-manager/guardian.cc | 551 +++++++-------- server-tools/instance-manager/guardian.h | 131 ++-- server-tools/instance-manager/instance.cc | 660 ++++++++++++------ server-tools/instance-manager/instance.h | 175 +++-- server-tools/instance-manager/instance_map.cc | 217 +++--- server-tools/instance-manager/instance_map.h | 61 +- .../instance-manager/instance_options.h | 1 - server-tools/instance-manager/manager.cc | 154 +++- server-tools/instance-manager/manager.h | 8 +- server-tools/instance-manager/user_map.cc | 20 +- 12 files changed, 1398 insertions(+), 986 deletions(-) diff --git a/server-tools/instance-manager/commands.cc b/server-tools/instance-manager/commands.cc index 15738f8ebb3..6cc1c8d3047 100644 --- a/server-tools/instance-manager/commands.cc +++ b/server-tools/instance-manager/commands.cc @@ -29,6 +29,7 @@ #include "guardian.h" #include "instance_map.h" #include "log.h" +#include "manager.h" #include "messages.h" #include "mysqld_error.h" #include "mysql_manager_error.h" @@ -36,8 +37,11 @@ #include "priv.h" #include "protocol.h" +/************************************************************************** + {{{ Static functions. +**************************************************************************/ -/* +/** modify_defaults_to_im_error -- a map of error codes of mysys::modify_defaults_file() into Instance Manager error codes. */ @@ -46,38 +50,25 @@ static const int modify_defaults_to_im_error[]= { 0, ER_OUT_OF_RESOURCES, ER_ACCESS_OPTION_FILE }; -/* - Add a string to a buffer. +/** + Parse version number from the version string. SYNOPSIS - put_to_buff() - buff buffer to add the string - str string to add - position offset in the buff to add a string + parse_version_number() + version_str + version + version_size DESCRIPTION + TODO - Function to add a string to the buffer. It is different from - store_to_protocol_packet, which is used in the protocol.cc. - The last one also stores the length of the string in a special way. - This is required for MySQL client/server protocol support only. + TODO: Move this function to Instance_options and parse version number + only once. - RETURN - 0 - ok - 1 - error occured + NOTE: This function is used only in SHOW INSTANCE STATUS statement at the + moment. */ -static inline int put_to_buff(Buffer *buff, const char *str, uint *position) -{ - uint len= strlen(str); - if (buff->append(*position, str, len)) - return 1; - - *position+= len; - return 0; -} - - static int parse_version_number(const char *version_str, char *version, uint version_size) { @@ -102,6 +93,9 @@ static int parse_version_number(const char *version_str, char *version, return 0; } +/************************************************************************** + }}} +**************************************************************************/ /************************************************************************** Implementation of Instance_name. @@ -122,7 +116,7 @@ Instance_name::Instance_name(const LEX_STRING *name) Implementation of Show_instances. **************************************************************************/ -/* +/** Implementation of SHOW INSTANCES statement. Possible error codes: @@ -172,7 +166,6 @@ int Show_instances::write_data(st_net *net) Instance *instance; Instance_map::Iterator iterator(instance_map); - instance_map->guardian->lock(); instance_map->lock(); while ((instance= iterator.next())) @@ -180,20 +173,25 @@ int Show_instances::write_data(st_net *net) Buffer send_buf; /* buffer for packets */ uint pos= 0; + instance->lock(); + const char *instance_name= instance->options.instance_name.str; - const char *state_name= instance_map->get_instance_state_name(instance); + const char *state_name= instance->get_state_name(); if (store_to_protocol_packet(&send_buf, instance_name, &pos) || store_to_protocol_packet(&send_buf, state_name, &pos) || my_net_write(net, send_buf.buffer, pos)) { err_status= TRUE; - break; } + + instance->unlock(); + + if (err_status) + break; } instance_map->unlock(); - instance_map->guardian->unlock(); return err_status ? ER_OUT_OF_RESOURCES : 0; } @@ -203,7 +201,7 @@ int Show_instances::write_data(st_net *net) Implementation of Flush_instances. **************************************************************************/ -/* +/** Implementation of FLUSH INSTANCES statement. Possible error codes: @@ -213,36 +211,19 @@ int Show_instances::write_data(st_net *net) int Flush_instances::execute(st_net *net, ulong connection_id) { - instance_map->guardian->lock(); - instance_map->lock(); - - if (instance_map->is_there_active_instance()) - { - instance_map->unlock(); - instance_map->guardian->unlock(); - return ER_THERE_IS_ACTIVE_INSTACE; - } - - if (instance_map->flush_instances()) - { - instance_map->unlock(); - instance_map->guardian->unlock(); + if (Manager::flush_instances()) return ER_OUT_OF_RESOURCES; - } - - instance_map->unlock(); - instance_map->guardian->unlock(); return net_send_ok(net, connection_id, NULL) ? ER_OUT_OF_RESOURCES : 0; } /************************************************************************** - Implementation of Abstract_instance_cmd. + Implementation of Instance_cmd. **************************************************************************/ -Abstract_instance_cmd::Abstract_instance_cmd(const LEX_STRING *instance_name_arg) - :instance_name(instance_name_arg) +Instance_cmd::Instance_cmd(const LEX_STRING *instance_name_arg): + instance_name(instance_name_arg) { /* MT-NOTE: we can not make a search for Instance object here, @@ -251,26 +232,39 @@ Abstract_instance_cmd::Abstract_instance_cmd(const LEX_STRING *instance_name_arg } +/************************************************************************** + Implementation of Abstract_instance_cmd. +**************************************************************************/ + +Abstract_instance_cmd::Abstract_instance_cmd( + const LEX_STRING *instance_name_arg) + :Instance_cmd(instance_name_arg) +{ +} + + int Abstract_instance_cmd::execute(st_net *net, ulong connection_id) { int err_code; + Instance *instance; instance_map->lock(); + instance= instance_map->find(get_instance_name()); + + if (!instance) { - Instance *instance= instance_map->find(get_instance_name()); - - if (!instance) - { - instance_map->unlock(); - return ER_BAD_INSTANCE_NAME; - } - - err_code= execute_impl(net, instance); + instance_map->unlock(); + return ER_BAD_INSTANCE_NAME; } + instance->lock(); instance_map->unlock(); + err_code= execute_impl(net, instance); + + instance->unlock(); + if (!err_code) err_code= send_ok_response(net, connection_id); @@ -288,7 +282,7 @@ Show_instance_status::Show_instance_status(const LEX_STRING *instance_name_arg) } -/* +/** Implementation of SHOW INSTANCE STATUS statement. Possible error codes: @@ -363,19 +357,14 @@ int Show_instance_status::write_data(st_net *net, Instance *instance) char version_num_buf[MAX_VERSION_LENGTH]; uint pos= 0; - const char *state_name; + const char *state_name= instance->get_state_name(); const char *version_tag= "unknown"; const char *version_num= "unknown"; - const char *mysqld_compatible_status; - - instance_map->guardian->lock(); - state_name= instance_map->get_instance_state_name(instance); - mysqld_compatible_status= instance->is_mysqld_compatible() ? "yes" : "no"; - instance_map->guardian->unlock(); + const char *mysqld_compatible_status= + instance->is_mysqld_compatible() ? "yes" : "no"; if (instance->options.mysqld_version) { - if (parse_version_number(instance->options.mysqld_version, version_num_buf, sizeof(version_num_buf))) return ER_OUT_OF_RESOURCES; @@ -409,7 +398,7 @@ Show_instance_options::Show_instance_options( } -/* +/** Implementation of SHOW INSTANCE OPTIONS statement. Possible error codes: @@ -505,23 +494,33 @@ Start_instance::Start_instance(const LEX_STRING *instance_name_arg) } -/* +/** Implementation of START INSTANCE statement. Possible error codes: ER_BAD_INSTANCE_NAME The instance with the given name does not exist - ER_OUT_OF_RESOURCES Not enough resources to complete the operation + ER_INSTANCE_MISCONFIGURED The instance configuration is invalid + ER_INSTANCE_ALREADY_STARTED The instance is already started + ER_CANNOT_START_INSTANCE The instance could not have been started + + TODO: as soon as this method operates only with Instance, we probably + should introduce a new method (execute_stop_instance()) in Instance and + just call it from here. */ int Start_instance::execute_impl(st_net * /* net */, Instance *instance) { - int err_code; + if (!instance->is_configured()) + return ER_INSTANCE_MISCONFIGURED; - if ((err_code= instance->start())) - return err_code; + if (instance->is_active()) + return ER_INSTANCE_ALREADY_STARTED; - if (!(instance->options.nonguarded)) - instance_map->guardian->guard(instance); + if (instance->start_mysqld()) + return ER_CANNOT_START_INSTANCE; + + instance->reset_stat(); + instance->set_state(Instance::NOT_STARTED); return 0; } @@ -546,25 +545,26 @@ Stop_instance::Stop_instance(const LEX_STRING *instance_name_arg) } -/* +/** Implementation of STOP INSTANCE statement. Possible error codes: ER_BAD_INSTANCE_NAME The instance with the given name does not exist ER_OUT_OF_RESOURCES Not enough resources to complete the operation + + TODO: as soon as this method operates only with Instance, we probably + should introduce a new method (execute_stop_instance()) in Instance and + just call it from here. */ int Stop_instance::execute_impl(st_net * /* net */, Instance *instance) { - int err_code; + if (!instance->is_active()) + return ER_INSTANCE_IS_NOT_STARTED; - if (!(instance->options.nonguarded)) - instance_map->guardian->stop_guard(instance); + instance->set_state(Instance::STOPPED); - if ((err_code= instance->stop())) - return err_code; - - return 0; + return instance->stop_mysqld() ? ER_STOP_INSTANCE : 0; } @@ -582,12 +582,12 @@ int Stop_instance::send_ok_response(st_net *net, ulong connection_id) **************************************************************************/ Create_instance::Create_instance(const LEX_STRING *instance_name_arg) - :instance_name(instance_name_arg) + :Instance_cmd(instance_name_arg) { } -/* +/** This operation initializes Create_instance object. SYNOPSIS @@ -604,7 +604,7 @@ bool Create_instance::init(const char **text) } -/* +/** This operation parses CREATE INSTANCE options. SYNOPSIS @@ -724,7 +724,7 @@ bool Create_instance::parse_args(const char **text) } -/* +/** Implementation of CREATE INSTANCE statement. Possible error codes: @@ -736,6 +736,7 @@ bool Create_instance::parse_args(const char **text) int Create_instance::execute(st_net *net, ulong connection_id) { int err_code; + Instance *instance; /* Check that the name is valid and there is no instance with such name. */ @@ -761,17 +762,26 @@ int Create_instance::execute(st_net *net, ulong connection_id) return err_code; } + instance= instance_map->find(get_instance_name()); + DBUG_ASSERT(instance); + if ((err_code= create_instance_in_file(get_instance_name(), &options))) { - Instance *instance= instance_map->find(get_instance_name()); - - if (instance) - instance_map->remove_instance(instance); /* instance is deleted here. */ + instance_map->remove_instance(instance); /* instance is deleted here. */ instance_map->unlock(); return err_code; } + /* + CREATE INSTANCE must not lead to start instance, even if it guarded. + + TODO: The problem however is that if Instance Manager restarts after + creating instance, the instance will be restarted (see also BUG#19718). + */ + + instance->set_state(Instance::STOPPED); + /* That's all. */ instance_map->unlock(); @@ -790,12 +800,12 @@ int Create_instance::execute(st_net *net, ulong connection_id) **************************************************************************/ Drop_instance::Drop_instance(const LEX_STRING *instance_name_arg) - :Abstract_instance_cmd(instance_name_arg) + :Instance_cmd(instance_name_arg) { } -/* +/** Implementation of DROP INSTANCE statement. Possible error codes: @@ -804,14 +814,38 @@ Drop_instance::Drop_instance(const LEX_STRING *instance_name_arg) ER_OUT_OF_RESOURCES Not enough resources to complete the operation */ -int Drop_instance::execute_impl(st_net * /* net */, Instance *instance) +int Drop_instance::execute(st_net *net, ulong connection_id) { int err_code; + Instance *instance; + + /* Lock Guardian, then Instance_map. */ + + instance_map->lock(); + + /* Find an instance. */ + + instance= instance_map->find(get_instance_name()); + + if (!instance) + { + instance_map->unlock(); + return ER_BAD_INSTANCE_NAME; + } + + instance->lock(); /* Check that the instance is offline. */ - if (instance_map->guardian->is_active(instance)) + if (instance->is_active()) + { + instance->unlock(); + instance_map->unlock(); + return ER_DROP_ACTIVE_INSTANCE; + } + + /* Try to remove instance from the file. */ err_code= modify_defaults_file(Options::Main::config_file, NULL, NULL, get_instance_name()->str, MY_REMOVE_SECTION); @@ -824,27 +858,30 @@ int Drop_instance::execute_impl(st_net * /* net */, Instance *instance) (const char *) get_instance_name()->str, (const char *) Options::Main::config_file, (int) err_code); + + instance->unlock(); + instance_map->unlock(); + + return modify_defaults_to_im_error[err_code]; } - if (err_code) - return modify_defaults_to_im_error[err_code]; + /* Unlock the instance before destroy. */ - /* Remove instance from the instance map hash and Guardian's list. */ + instance->unlock(); - if (!instance->options.nonguarded) - instance_map->guardian->stop_guard(instance); - - if ((err_code= instance->stop())) - return err_code; + /* + Remove instance from the instance map + (the instance will be also destroyed here). + */ instance_map->remove_instance(instance); - return 0; -} + /* Unlock the instance map. */ + instance_map->unlock(); + + /* That's all: send ok. */ -int Drop_instance::send_ok_response(st_net *net, ulong connection_id) -{ if (net_send_ok(net, connection_id, "Instance dropped")) return ER_OUT_OF_RESOURCES; @@ -867,7 +904,7 @@ Show_instance_log::Show_instance_log(const LEX_STRING *instance_name_arg, } -/* +/** Implementation of SHOW INSTANCE LOG statement. Possible error codes: @@ -1012,7 +1049,7 @@ Show_instance_log_files::Show_instance_log_files } -/* +/** Implementation of SHOW INSTANCE LOG FILES statement. Possible error codes: @@ -1133,7 +1170,7 @@ int Show_instance_log_files::write_data(st_net *net, Instance *instance) Implementation of Abstract_option_cmd. **************************************************************************/ -/* +/** Instance_options_list -- a data class representing a list of options for some instance. */ @@ -1251,7 +1288,7 @@ bool Abstract_option_cmd::init(const char **text) } -/* +/** Correct the option file. The "skip" option is used to remove the found option. @@ -1290,8 +1327,8 @@ int Abstract_option_cmd::correct_file(Instance *instance, Named_value *option, } -/* - Implementation of SET statement. +/** + Lock Instance Map and call execute_impl(). Possible error codes: ER_BAD_INSTANCE_NAME The instance with the given name does not exist @@ -1341,6 +1378,11 @@ Abstract_option_cmd::get_instance_options_list(const LEX_STRING *instance_name) } +/** + Skeleton implementation of option-management command. + + MT-NOTE: Instance Map is locked before calling this operation. +*/ int Abstract_option_cmd::execute_impl(st_net *net, ulong connection_id) { int err_code= 0; @@ -1352,12 +1394,18 @@ int Abstract_option_cmd::execute_impl(st_net *net, ulong connection_id) Instance_options_list *lst= (Instance_options_list *) hash_element(&instance_options_map, i); + bool instance_is_active; + lst->instance= instance_map->find(lst->get_instance_name()); if (!lst->instance) return ER_BAD_INSTANCE_NAME; - if (instance_map->guardian->is_active(lst->instance)) + lst->instance->lock(); + instance_is_active= lst->instance->is_active(); + lst->instance->unlock(); + + if (instance_is_active) return ER_INSTANCE_IS_ACTIVE; } @@ -1368,6 +1416,8 @@ int Abstract_option_cmd::execute_impl(st_net *net, ulong connection_id) Instance_options_list *lst= (Instance_options_list *) hash_element(&instance_options_map, i); + lst->instance->lock(); + for (int j= 0; j < lst->options.get_size(); ++j) { Named_value option= lst->options.get_element(j); @@ -1377,6 +1427,8 @@ int Abstract_option_cmd::execute_impl(st_net *net, ulong connection_id) break; } + lst->instance->unlock(); + if (err_code) break; } @@ -1392,7 +1444,7 @@ int Abstract_option_cmd::execute_impl(st_net *net, ulong connection_id) Implementation of Set_option. **************************************************************************/ -/* +/** This operation parses SET options. SYNOPSIS @@ -1566,7 +1618,7 @@ int Set_option::process_option(Instance *instance, Named_value *option) Implementation of Unset_option. **************************************************************************/ -/* +/** This operation parses UNSET options. SYNOPSIS @@ -1662,7 +1714,7 @@ bool Unset_option::parse_args(const char **text) } -/* +/** Implementation of UNSET statement. Possible error codes: diff --git a/server-tools/instance-manager/commands.h b/server-tools/instance-manager/commands.h index 8768aaab121..9b5d27b0982 100644 --- a/server-tools/instance-manager/commands.h +++ b/server-tools/instance-manager/commands.h @@ -30,7 +30,7 @@ #endif -/* +/** Print all instances of this instance manager. Grammar: SHOW INSTANCES */ @@ -50,7 +50,7 @@ private: }; -/* +/** Reread configuration file and refresh internal cache. Grammar: FLUSH INSTANCES */ @@ -66,29 +66,20 @@ public: }; -/* - Abstract class for Instance-specific commands. +/** + Base class for Instance-specific commands + (commands that operate on one instance). + + Instance_cmd extends Command class by: + - an attribute for storing instance name; + - code to initialize instance name in constructor; + - an accessor to get instance name. */ -class Abstract_instance_cmd: public Command +class Instance_cmd : public Command { public: - Abstract_instance_cmd(const LEX_STRING *instance_name_arg); - -public: - virtual int execute(st_net *net, ulong connection_id); - -protected: - /* MT-NOTE: this operation is called under acquired Instance_map's lock. */ - virtual int execute_impl(st_net *net, Instance *instance) = 0; - - /* - This operation is invoked on successful return of execute_impl() and is - intended to send closing data. - - MT-NOTE: this operation is called under released Instance_map's lock. - */ - virtual int send_ok_response(st_net *net, ulong connection_id) = 0; + Instance_cmd(const LEX_STRING *instance_name_arg); protected: inline const LEX_STRING *get_instance_name() const @@ -101,7 +92,50 @@ private: }; -/* +/** + Abstract class for Instance-specific commands. + + Abstract_instance_cmd extends Instance_cmd by providing a common + framework for writing command-implementations. Basically, the class + implements Command::execute() pure virtual function in the following + way: + - Lock Instance_map; + - Get an instance by name. Return an error, if there is no such + instance; + - Lock the instance; + - Unlock Instance_map; + - Call execute_impl(), which should be implemented in derived class; + - Unlock the instance; + - Send response to the client and return error status. +*/ + +class Abstract_instance_cmd: public Instance_cmd +{ +public: + Abstract_instance_cmd(const LEX_STRING *instance_name_arg); + +public: + virtual int execute(st_net *net, ulong connection_id); + +protected: + /** + This operation is intended to contain command-specific implementation. + + MT-NOTE: this operation is called under acquired Instance's lock. + */ + virtual int execute_impl(st_net *net, Instance *instance) = 0; + + /** + This operation is invoked on successful return of execute_impl() and is + intended to send closing data. + + MT-NOTE: this operation is called under released Instance's lock. + */ + virtual int send_ok_response(st_net *net, ulong connection_id) = 0; +}; + + +/** Print status of an instance. Grammar: SHOW INSTANCE STATUS */ @@ -121,7 +155,7 @@ private: }; -/* +/** Print options of chosen instance. Grammar: SHOW INSTANCE OPTIONS */ @@ -141,7 +175,7 @@ private: }; -/* +/** Start an instance. Grammar: START INSTANCE */ @@ -157,7 +191,7 @@ protected: }; -/* +/** Stop an instance. Grammar: STOP INSTANCE */ @@ -173,12 +207,12 @@ protected: }; -/* +/** Create an instance. Grammar: CREATE INSTANCE [] */ -class Create_instance: public Command +class Create_instance: public Instance_cmd { public: Create_instance(const LEX_STRING *instance_name_arg); @@ -189,22 +223,15 @@ public: protected: virtual int execute(st_net *net, ulong connection_id); - inline const LEX_STRING *get_instance_name() const - { - return instance_name.get_str(); - } - private: bool parse_args(const char **text); private: - Instance_name instance_name; - Named_value_arr options; }; -/* +/** Drop an instance. Grammar: DROP INSTANCE @@ -213,18 +240,17 @@ private: is removed from the instance map. */ -class Drop_instance: public Abstract_instance_cmd +class Drop_instance: public Instance_cmd { public: Drop_instance(const LEX_STRING *instance_name_arg); protected: - virtual int execute_impl(st_net *net, Instance *instance); - virtual int send_ok_response(st_net *net, ulong connection_id); + virtual int execute(st_net *net, ulong connection_id); }; -/* +/** Print requested part of the log. Grammar: SHOW LOG {ERROR | SLOW | GENERAL} size[, offset_from_end] @@ -252,7 +278,7 @@ private: }; -/* +/** Shows the list of the log files, used by an instance. Grammar: SHOW LOG FILES */ @@ -272,7 +298,7 @@ private: }; -/* +/** Abstract class for option-management commands. */ @@ -312,7 +338,7 @@ private: }; -/* +/** Set an option for the instance. Grammar: SET instance_name.option[=option_value][, ...] */ @@ -329,7 +355,7 @@ protected: }; -/* +/** Remove option of the instance. Grammar: UNSET instance_name.option[, ...] */ @@ -346,7 +372,7 @@ protected: }; -/* +/** Syntax error command. This command is issued if parser reported a syntax error. We need it to diff --git a/server-tools/instance-manager/guardian.cc b/server-tools/instance-manager/guardian.cc index e601ce0111c..1b451cd9933 100644 --- a/server-tools/instance-manager/guardian.cc +++ b/server-tools/instance-manager/guardian.cc @@ -28,101 +28,126 @@ #include "instance_map.h" #include "log.h" #include "mysql_manager_error.h" +#include "options.h" -const char * -Guardian::get_instance_state_name(enum_instance_state state) -{ - switch (state) { - case NOT_STARTED: - return "offline"; - case STARTING: - return "starting"; +/************************************************************************* + {{{ Constructor & destructor. +*************************************************************************/ - case STARTED: - return "online"; +/** + Guardian constructor. - case JUST_CRASHED: - return "failed"; + SYNOPSIS + Guardian() + thread_registry_arg + instance_map_arg - case CRASHED: - return "crashed"; - - case CRASHED_AND_ABANDONED: - return "abandoned"; - - case STOPPING: - return "stopping"; - } - - return NULL; /* just to ignore compiler warning. */ -} - -/* {{{ Constructor & destructor. */ + DESCRIPTION + Nominal contructor intended for assigning references and initialize + trivial objects. Real initialization is made by init() method. +*/ Guardian::Guardian(Thread_registry *thread_registry_arg, - Instance_map *instance_map_arg, - uint monitoring_interval_arg) - :stopped(FALSE), - monitoring_interval(monitoring_interval_arg), + Instance_map *instance_map_arg) + :shutdown_requested(FALSE), + stopped(FALSE), thread_registry(thread_registry_arg), - instance_map(instance_map_arg), - shutdown_requested(FALSE) + instance_map(instance_map_arg) { pthread_mutex_init(&LOCK_guardian, 0); pthread_cond_init(&COND_guardian, 0); - init_alloc_root(&alloc, MEM_ROOT_BLOCK_SIZE, 0); } Guardian::~Guardian() { - /* delay guardian destruction to the moment when no one needs it */ - pthread_mutex_lock(&LOCK_guardian); - free_root(&alloc, MYF(0)); - pthread_mutex_unlock(&LOCK_guardian); + /* + NOTE: it's necessary to synchronize here, because Guiardian thread can be + still alive an hold the mutex (because it is detached and we have no + control over it). + */ + + lock(); + unlock(); + pthread_mutex_destroy(&LOCK_guardian); pthread_cond_destroy(&COND_guardian); } -/* }}} */ +/************************************************************************* + }}} +*************************************************************************/ +/** + Send request to stop Guardian. + + SYNOPSIS + request_shutdown() +*/ + void Guardian::request_shutdown() { - pthread_mutex_lock(&LOCK_guardian); - /* STOP Instances or just clean up Guardian repository */ stop_instances(); + + lock(); shutdown_requested= TRUE; - pthread_mutex_unlock(&LOCK_guardian); + unlock(); + + ping(); } -void Guardian::process_instance(Instance *instance, - GUARD_NODE *current_node, - LIST **guarded_instances, - LIST *node) +/** + Process an instance. + + SYNOPSIS + process_instance() + instance a pointer to the instance for processing + + MT-NOTE: + - the given instance must be locked before calling this operation; + - Guardian must be locked before calling this operation. +*/ + +void Guardian::process_instance(Instance *instance) { - uint waitchild= (uint) Instance::DEFAULT_SHUTDOWN_DELAY; - /* The amount of times, Guardian attempts to restart an instance */ int restart_retry= 100; time_t current_time= time(NULL); - if (current_node->state == STOPPING) + if (instance->get_state() == Instance::STOPPING) { - waitchild= instance->options.get_shutdown_delay(); + /* This brach is executed during shutdown. */ - /* this returns TRUE if and only if an instance was stopped for sure */ + /* This returns TRUE if and only if an instance was stopped for sure. */ if (instance->is_crashed()) - *guarded_instances= list_delete(*guarded_instances, node); - else if ( (uint) (current_time - current_node->last_checked) > waitchild) { + log_info("Guardian: '%s' stopped.", + (const char *) instance->get_name()->str); + + instance->set_state(Instance::STOPPED); + } + else if ((uint) (current_time - instance->last_checked) >= + instance->options.get_shutdown_delay()) + { + log_info("Guardian: '%s' hasn't stopped within %d secs.", + (const char *) instance->get_name()->str, + (int) instance->options.get_shutdown_delay()); + instance->kill_mysqld(SIGKILL); - /* - Later we do node= node->next. This is ok, as we are only removing - the node from the list. The pointer to the next one is still valid. - */ - *guarded_instances= list_delete(*guarded_instances, node); + + log_info("Guardian: pretend that '%s' is killed.", + (const char *) instance->get_name()->str); + + instance->set_state(Instance::STOPPED); + } + else + { + log_info("Guardian: waiting for '%s' to stop (%d secs left).", + (const char *) instance->get_name()->str, + (int) (instance->options.get_shutdown_delay() - + current_time + instance->last_checked)); } return; @@ -133,83 +158,90 @@ void Guardian::process_instance(Instance *instance, /* The instance can be contacted on it's port */ /* If STARTING also check that pidfile has been created */ - if (current_node->state == STARTING && - current_node->instance->options.load_pid() == 0) + if (instance->get_state() == Instance::STARTING && + instance->options.load_pid() == 0) { /* Pid file not created yet, don't go to STARTED state yet */ } - else if (current_node->state != STARTED) + else if (instance->get_state() != Instance::STARTED) { /* clear status fields */ log_info("Guardian: '%s' is running, set state to STARTED.", (const char *) instance->options.instance_name.str); - current_node->restart_counter= 0; - current_node->crash_moment= 0; - current_node->state= STARTED; + instance->reset_stat(); + instance->set_state(Instance::STARTED); } } else { - switch (current_node->state) { - case NOT_STARTED: + switch (instance->get_state()) { + case Instance::NOT_STARTED: log_info("Guardian: starting '%s'...", (const char *) instance->options.instance_name.str); - /* NOTE, set state to STARTING _before_ start() is called */ - current_node->state= STARTING; - instance->start(); - current_node->last_checked= current_time; - break; - case STARTED: /* fallthrough */ - case STARTING: /* let the instance start or crash */ - if (instance->is_crashed()) - { - current_node->crash_moment= current_time; - current_node->last_checked= current_time; - current_node->state= JUST_CRASHED; - /* fallthrough -- restart an instance immediately */ - } - else - break; - case JUST_CRASHED: - if (current_time - current_node->crash_moment <= 2) + /* NOTE: set state to STARTING _before_ start() is called. */ + instance->set_state(Instance::STARTING); + instance->last_checked= current_time; + + instance->start_mysqld(); + + return; + + case Instance::STARTED: /* fallthrough */ + case Instance::STARTING: /* let the instance start or crash */ + if (!instance->is_crashed()) + return; + + instance->crash_moment= current_time; + instance->last_checked= current_time; + instance->set_state(Instance::JUST_CRASHED); + /* fallthrough -- restart an instance immediately */ + + case Instance::JUST_CRASHED: + if (current_time - instance->crash_moment <= 2) { if (instance->is_crashed()) { - instance->start(); + instance->start_mysqld(); log_info("Guardian: starting '%s'...", (const char *) instance->options.instance_name.str); } } else - current_node->state= CRASHED; - break; - case CRASHED: /* just regular restarts */ - if (current_time - current_node->last_checked > - monitoring_interval) + instance->set_state(Instance::CRASHED); + + return; + + case Instance::CRASHED: /* just regular restarts */ + if (current_time - instance->last_checked <= + Options::Main::monitoring_interval) + return; + + if (instance->restart_counter < restart_retry) { - if ((current_node->restart_counter < restart_retry)) + if (instance->is_crashed()) { - if (instance->is_crashed()) - { - instance->start(); - current_node->last_checked= current_time; - current_node->restart_counter++; - log_info("Guardian: restarting '%s'...", - (const char *) instance->options.instance_name.str); - } - } - else - { - log_info("Guardian: can not start '%s'. " - "Abandoning attempts to (re)start it", + instance->start_mysqld(); + instance->last_checked= current_time; + + log_info("Guardian: restarting '%s'...", (const char *) instance->options.instance_name.str); - current_node->state= CRASHED_AND_ABANDONED; } } - break; - case CRASHED_AND_ABANDONED: - break; /* do nothing */ + else + { + log_info("Guardian: can not start '%s'. " + "Abandoning attempts to (re)start it", + (const char *) instance->options.instance_name.str); + + instance->set_state(Instance::CRASHED_AND_ABANDONED); + } + + return; + + case Instance::CRASHED_AND_ABANDONED: + return; /* do nothing */ + default: DBUG_ASSERT(0); } @@ -217,56 +249,78 @@ void Guardian::process_instance(Instance *instance, } -/* +/** Main function of Guardian thread. SYNOPSIS run() DESCRIPTION - Check for all guarded instances and restart them if needed. If everything - is fine go and sleep for some time. + Check for all guarded instances and restart them if needed. */ void Guardian::run() { - Instance *instance; - LIST *node; struct timespec timeout; log_info("Guardian: started."); thread_registry->register_thread(&thread_info); - pthread_mutex_lock(&LOCK_guardian); + /* Loop, until all instances were shut down at the end. */ - /* loop, until all instances were shut down at the end */ - while (!(shutdown_requested && (guarded_instances == NULL))) + while (true) { - node= guarded_instances; + Instance_map::Iterator instances_it(instance_map); + Instance *instance; + bool all_instances_stopped= TRUE; - while (node != NULL) + instance_map->lock(); + + while ((instance= instances_it.next())) { - GUARD_NODE *current_node= (GUARD_NODE *) node->data; - instance= ((GUARD_NODE *) node->data)->instance; - process_instance(instance, current_node, &guarded_instances, node); + instance->lock(); - node= node->next; + if (!instance->is_guarded() || + instance->get_state() == Instance::STOPPED) + { + instance->unlock(); + continue; + } + + process_instance(instance); + + if (instance->get_state() != Instance::STOPPED) + all_instances_stopped= FALSE; + + instance->unlock(); } - timeout.tv_sec= time(NULL) + monitoring_interval; + + instance_map->unlock(); + + lock(); + + if (shutdown_requested && all_instances_stopped) + { + log_info("Guardian: all guarded mysqlds stopped."); + + stopped= TRUE; + unlock(); + break; + } + + timeout.tv_sec= time(NULL) + Options::Main::monitoring_interval; timeout.tv_nsec= 0; - /* check the loop predicate before sleeping */ - if (!(shutdown_requested && (!(guarded_instances)))) - thread_registry->cond_timedwait(&thread_info, &COND_guardian, - &LOCK_guardian, &timeout); + thread_registry->cond_timedwait(&thread_info, &COND_guardian, + &LOCK_guardian, &timeout); + unlock(); } log_info("Guardian: stopped."); - stopped= TRUE; - pthread_mutex_unlock(&LOCK_guardian); - /* now, when the Guardian is stopped we can stop the IM */ + /* Now, when the Guardian is stopped we can stop the IM. */ + thread_registry->unregister_thread(&thread_info); thread_registry->request_shutdown(); @@ -274,129 +328,65 @@ void Guardian::run() } -int Guardian::is_stopped() +/** + Return the value of stopped flag. +*/ + +bool Guardian::is_stopped() { int var; - pthread_mutex_lock(&LOCK_guardian); + + lock(); var= stopped; - pthread_mutex_unlock(&LOCK_guardian); + unlock(); + return var; } -/* - Initialize the list of guarded instances: loop through the Instance_map and - add all of the instances, which don't have 'nonguarded' option specified. +/** + Wake up Guardian thread. - SYNOPSIS - Guardian::init() - - NOTE: The operation should be invoked with the following locks acquired: - - Guardian; - - Instance_map; - - RETURN - 0 - ok - 1 - error occurred + MT-NOTE: though usually the mutex associated with condition variable should + be acquired before signalling the variable, here this is not needed. + Signalling under locked mutex is used to avoid lost signals. In the current + logic however locking mutex does not guarantee that the signal will not be + lost. */ -int Guardian::init() +void Guardian::ping() +{ + pthread_cond_signal(&COND_guardian); +} + + +/** + Prepare list of instances. + + SYNOPSIS + init() + + MT-NOTE: Instance Map must be locked before calling the operation. +*/ + +void Guardian::init() { Instance *instance; Instance_map::Iterator iterator(instance_map); - /* clear the list of guarded instances */ - free_root(&alloc, MYF(0)); - init_alloc_root(&alloc, MEM_ROOT_BLOCK_SIZE, 0); - guarded_instances= NULL; - while ((instance= iterator.next())) { - if (instance->options.nonguarded) - continue; + instance->lock(); - if (guard(instance, TRUE)) /* do not lock guardian */ - return 1; + instance->reset_stat(); + instance->set_state(Instance::NOT_STARTED); + + instance->unlock(); } - - return 0; } -/* - Add instance to the Guardian list - - SYNOPSIS - guard() - instance the instance to be guarded - nolock whether we prefer do not lock Guardian here, - but use external locking instead - - DESCRIPTION - - The instance is added to the guarded instances list. Usually guard() is - called after we start an instance. - - RETURN - 0 - ok - 1 - error occurred -*/ - -int Guardian::guard(Instance *instance, bool nolock) -{ - LIST *node; - GUARD_NODE *content; - - node= (LIST *) alloc_root(&alloc, sizeof(LIST)); - content= (GUARD_NODE *) alloc_root(&alloc, sizeof(GUARD_NODE)); - - if ((!(node)) || (!(content))) - return 1; - /* we store the pointers to instances from the instance_map's MEM_ROOT */ - content->instance= instance; - content->restart_counter= 0; - content->crash_moment= 0; - content->state= NOT_STARTED; - node->data= (void*) content; - - if (nolock) - guarded_instances= list_add(guarded_instances, node); - else - { - pthread_mutex_lock(&LOCK_guardian); - guarded_instances= list_add(guarded_instances, node); - pthread_mutex_unlock(&LOCK_guardian); - } - - return 0; -} - - -/* - TODO: perhaps it would make sense to create a pool of the LIST nodeents - and give them upon request. Now we are loosing a bit of memory when - guarded instance was stopped and then restarted (since we cannot free just - a piece of the MEM_ROOT). -*/ - -int Guardian::stop_guard(Instance *instance) -{ - LIST *node; - - pthread_mutex_lock(&LOCK_guardian); - - node= find_instance_node(instance); - - if (node != NULL) - guarded_instances= list_delete(guarded_instances, node); - - pthread_mutex_unlock(&LOCK_guardian); - - /* if there is nothing to delete it is also fine */ - return 0; -} - -/* +/** An internal method which is called at shutdown to unregister instances and attempt to stop them if requested. @@ -409,86 +399,71 @@ int Guardian::stop_guard(Instance *instance) accordingly. NOTE - Guardian object should be locked by the calling function. + Guardian object should be locked by the caller. - RETURN - 0 - ok - 1 - error occurred */ -int Guardian::stop_instances() +void Guardian::stop_instances() { - LIST *node; - node= guarded_instances; - while (node != NULL) + Instance_map::Iterator instances_it(instance_map); + Instance *instance; + + instance_map->lock(); + + while ((instance= instances_it.next())) { - GUARD_NODE *current_node= (GUARD_NODE *) node->data; + instance->lock(); + + if (!instance->is_guarded() || + instance->get_state() == Instance::STOPPED) + { + instance->unlock(); + continue; + } + /* If instance is running or was running (and now probably hanging), request stop. */ - if (current_node->instance->is_mysqld_running() || - (current_node->state == STARTED)) + + if (instance->is_mysqld_running() || + instance->get_state() == Instance::STARTED) { - current_node->state= STOPPING; - current_node->last_checked= time(NULL); + instance->set_state(Instance::STOPPING); + instance->last_checked= time(NULL); } else - /* otherwise remove it from the list */ - guarded_instances= list_delete(guarded_instances, node); - /* But try to kill it anyway. Just in case */ - current_node->instance->kill_mysqld(SIGTERM); - node= node->next; + { + /* Otherwise mark it as STOPPED. */ + instance->set_state(Instance::STOPPED); + } + + /* Request mysqld to stop. */ + + instance->kill_mysqld(SIGTERM); + + instance->unlock(); } - return 0; + + instance_map->unlock(); } +/** + Lock Guardian. +*/ + void Guardian::lock() { pthread_mutex_lock(&LOCK_guardian); } +/** + Unlock Guardian. +*/ + void Guardian::unlock() { pthread_mutex_unlock(&LOCK_guardian); } - - -LIST *Guardian::find_instance_node(Instance *instance) -{ - LIST *node= guarded_instances; - - while (node != NULL) - { - /* - We compare only pointers, as we always use pointers from the - instance_map's MEM_ROOT. - */ - if (((GUARD_NODE *) node->data)->instance == instance) - return node; - - node= node->next; - } - - return NULL; -} - - -bool Guardian::is_active(Instance *instance) -{ - bool guarded; - - lock(); - - guarded= find_instance_node(instance) != NULL; - - /* is_running() can take a long time, so let's unlock mutex first. */ - unlock(); - - if (guarded) - return true; - - return instance->is_mysqld_running(); -} diff --git a/server-tools/instance-manager/guardian.h b/server-tools/instance-manager/guardian.h index 0eee1dc631d..2c7987f4565 100644 --- a/server-tools/instance-manager/guardian.h +++ b/server-tools/instance-manager/guardian.h @@ -17,10 +17,12 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ -#include "thread_registry.h" +#include #include #include +#include "thread_registry.h" + #if defined(__GNUC__) && defined(USE_PRAGMA_INTERFACE) #pragma interface #endif @@ -28,7 +30,6 @@ class Instance; class Instance_map; class Thread_registry; -struct GUARD_NODE; /** The guardian thread is responsible for monitoring and restarting of guarded @@ -38,97 +39,73 @@ struct GUARD_NODE; class Guardian: public Thread { public: - /* states of an instance */ - enum enum_instance_state { NOT_STARTED= 1, STARTING, STARTED, JUST_CRASHED, - CRASHED, CRASHED_AND_ABANDONED, STOPPING }; - - /* - The Guardian list node structure. Guardian utilizes it to store - guarded instances plus some additional info. - */ - - struct GUARD_NODE - { - Instance *instance; - /* state of an instance (i.e. STARTED, CRASHED, etc.) */ - enum_instance_state state; - /* the amount of attemts to restart instance (cleaned up at success) */ - int restart_counter; - /* triggered at a crash */ - time_t crash_moment; - /* General time field. Used to provide timeouts (at shutdown and restart) */ - time_t last_checked; - }; - - /* Return client state name. */ - static const char *get_instance_state_name(enum_instance_state state); - Guardian(Thread_registry *thread_registry_arg, - Instance_map *instance_map_arg, - uint monitoring_interval_arg); - virtual ~Guardian(); - /* Initialize or refresh the list of guarded instances */ - int init(); - /* Request guardian shutdown. Stop instances if needed */ + Instance_map *instance_map_arg); + ~Guardian(); + + void init(); + +public: void request_shutdown(); - /* Start instance protection */ - int guard(Instance *instance, bool nolock= FALSE); - /* Stop instance protection */ - int stop_guard(Instance *instance); - /* Returns TRUE if guardian thread is stopped */ - int is_stopped(); + + bool is_stopped(); + void lock(); void unlock(); - /* - Return an internal list node for the given instance if the instance is - managed by Guardian. Otherwise, return NULL. + void ping(); - MT-NOTE: must be called under acquired lock. - */ - LIST *find_instance_node(Instance *instance); - - /* The operation is used to check if the instance is active or not. */ - bool is_active(Instance *instance); - - /* - Return state of the given instance list node. The pointer must specify - a valid list node. - */ - inline enum_instance_state get_instance_state(LIST *instance_node); protected: - /* Main funtion of the thread */ virtual void run(); -public: +private: + void stop_instances(); + + void process_instance(Instance *instance); + +private: + /* + LOCK_guardian protectes the members in this section: + - shutdown_requested; + - stopped; + + Also, it is used for COND_guardian. + */ + pthread_mutex_t LOCK_guardian; + + /* + Guardian's main loop waits on this condition. So, it should be signalled + each time, when instance state has been changed and we want Guardian to + wake up. + + TODO: Change this to having data-scoped conditions, i.e. conditions, + which indicate that some data has been changed. + */ pthread_cond_t COND_guardian; -private: - /* Prepares Guardian shutdown. Stops instances is needed */ - int stop_instances(); - /* check instance state and act accordingly */ - void process_instance(Instance *instance, GUARD_NODE *current_node, - LIST **guarded_instances, LIST *elem); + /* + This variable is set to TRUE, when Manager thread is shutting down. + The flag is used by Guardian thread to understand that it's time to + finish. + */ + bool shutdown_requested; - int stopped; + /* + This flag is set to TRUE on shutdown by Guardian thread, when all guarded + mysqlds are stopped. + + The flag is used in the Manager thread to wait for Guardian to stop all + mysqlds. + */ + bool stopped; -private: - pthread_mutex_t LOCK_guardian; Thread_info thread_info; - int monitoring_interval; Thread_registry *thread_registry; Instance_map *instance_map; - LIST *guarded_instances; - MEM_ROOT alloc; - /* this variable is set to TRUE when we want to stop Guardian thread */ - bool shutdown_requested; + +private: + Guardian(const Guardian &); + Guardian&operator =(const Guardian &); }; - -inline Guardian::enum_instance_state -Guardian::get_instance_state(LIST *instance_node) -{ - return ((GUARD_NODE *) instance_node->data)->state; -} - #endif /* INCLUDES_MYSQL_INSTANCE_MANAGER_GUARDIAN_H */ diff --git a/server-tools/instance-manager/instance.cc b/server-tools/instance-manager/instance.cc index 6b4289c5b29..b852a8323e5 100644 --- a/server-tools/instance-manager/instance.cc +++ b/server-tools/instance-manager/instance.cc @@ -36,7 +36,9 @@ #include "thread_registry.h" #include "instance_map.h" -/* {{{ Platform-specific functions. */ +/************************************************************************* + {{{ Platform-specific functions. +*************************************************************************/ #ifndef __WIN__ typedef pid_t My_process_info; @@ -44,34 +46,6 @@ typedef pid_t My_process_info; typedef PROCESS_INFORMATION My_process_info; #endif -/* - Proxy thread is a simple way to avoid all pitfalls of the threads - implementation in the OS (e.g. LinuxThreads). With such a thread we - don't have to process SIGCHLD, which is a tricky business if we want - to do it in a portable way. -*/ - -class Instance_monitor: public Thread -{ -public: - Instance_monitor(Instance *instance_arg) :instance(instance_arg) {} -protected: - virtual void run(); - void start_and_monitor_instance(Instance_options *old_instance_options, - Instance_map *instance_map, - Thread_registry *thread_registry); -private: - Instance *instance; -}; - -void Instance_monitor::run() -{ - start_and_monitor_instance(&instance->options, - Manager::get_instance_map(), - Manager::get_thread_registry()); - delete this; -} - /* Wait for an instance @@ -285,113 +259,149 @@ int kill(pid_t pid, int signum) } #endif -/* }}} */ +/************************************************************************* + }}} +*************************************************************************/ -/* {{{ Static constants. */ + +/************************************************************************* + {{{ Static constants. +*************************************************************************/ const LEX_STRING Instance::DFLT_INSTANCE_NAME= { C_STRING_WITH_LEN("mysqld") }; -/* }}} */ +/************************************************************************* + }}} +*************************************************************************/ -/* - Fork child, exec an instance and monitor it. +/************************************************************************* + {{{ Instance Monitor thread. +*************************************************************************/ - SYNOPSIS - start_and_monitor_instance() - old_instance_options Pointer to the options of the instance to be - launched. This info is likely to become obsolete - when function returns from wait_process() - instance_map Pointer to the instance_map. We use it to protect - the instance from deletion, while we are working - with it. +/** + Proxy thread is a simple way to avoid all pitfalls of the threads + implementation in the OS (e.g. LinuxThreads). With such a thread we + don't have to process SIGCHLD, which is a tricky business if we want + to do it in a portable way. - DESCRIPTION - Fork a child, then exec and monitor it. When the child is dead, - find appropriate instance (for this purpose we save its name), - set appropriate flags and wake all threads waiting for instance - to stop. + Instance Monitor Thread forks a child process, execs mysqld and waits for + the child to die. - NOTE - A separate thread for starting/monitoring instance is a simple way - to avoid all pitfalls of the threads implementation in the OS (e.g. - LinuxThreads). For one, with such a thread we don't have to process - SIGCHLD, which is a tricky business if we want to do it in a - portable way. - - RETURN - Function returns no value + Instance Monitor assumes that the monitoring instance will not be dropped. + This is guaranteed by having flag monitoring_thread_active and + Instance::is_active() operation. */ -void -Instance_monitor:: -start_and_monitor_instance(Instance_options *old_instance_options, - Instance_map *instance_map, - Thread_registry *thread_registry) +class Instance_monitor: public Thread { - Instance_name instance_name(&old_instance_options->instance_name); - Instance *current_instance; - My_process_info process_info; - Thread_info thread_info; +public: + Instance_monitor(Instance *instance_arg) :instance(instance_arg) {} +protected: + virtual void run(); + void start_and_monitor_instance(); +private: + Instance *instance; +}; + + +void Instance_monitor::run() +{ + start_and_monitor_instance(); + delete this; +} + + +void Instance_monitor::start_and_monitor_instance() +{ + Thread_registry *thread_registry= Manager::get_thread_registry(); + Guardian *guardian= Manager::get_guardian(); + + My_process_info mysqld_process_info; + Thread_info monitor_thread_info; log_info("Instance '%s': Monitor: started.", (const char *) instance->get_name()->str); - if (!old_instance_options->nonguarded) + /* + For guarded instance register the thread in Thread_registry to wait for + the thread to stop on shutdown (nonguarded instances are not stopped on + shutdown, so the thread will no finish). + */ + + if (instance->is_guarded()) { - /* - Register thread in Thread_registry to wait for it to stop on shutdown - only if instance is guarded. If instance is guarded, the thread will not - finish, because nonguarded instances are not stopped on shutdown. - */ - thread_registry->register_thread(&thread_info, FALSE); + thread_registry->register_thread(&monitor_thread_info, FALSE); } - /* - Lock instance map to guarantee that no instances are deleted during - strmake() and execv() calls. - */ - instance_map->lock(); - - /* - Save the instance name in the case if Instance object we - are using is destroyed. (E.g. by "FLUSH INSTANCES") - */ + /* Starting mysqld. */ log_info("Instance '%s': Monitor: starting mysqld...", (const char *) instance->get_name()->str); - if (start_process(old_instance_options, &process_info)) + if (start_process(&instance->options, &mysqld_process_info)) { - instance_map->unlock(); - return; /* error is logged */ + instance->lock(); + instance->monitoring_thread_active= FALSE; + instance->unlock(); + + return; } - /* allow users to delete instances */ - instance_map->unlock(); + /* Waiting for mysqld to die. */ log_info("Instance '%s': Monitor: waiting for mysqld to stop...", (const char *) instance->get_name()->str); - wait_process(&process_info); /* Don't check for return value. */ + wait_process(&mysqld_process_info); /* Don't check for return value. */ - instance_map->lock(); + log_info("Instance '%s': Monitor: mysqld stopped.", + (const char *) instance->get_name()->str); - current_instance= instance_map->find(instance_name.get_str()); + /* Update instance status. */ - if (current_instance) - current_instance->set_crash_flag_n_wake_all(); + instance->lock(); - instance_map->unlock(); + if (instance->is_guarded()) + thread_registry->unregister_thread(&monitor_thread_info); - if (!old_instance_options->nonguarded) - thread_registry->unregister_thread(&thread_info); + instance->crashed= TRUE; + instance->monitoring_thread_active= FALSE; log_info("Instance '%s': Monitor: finished.", (const char *) instance->get_name()->str); + + instance->unlock(); + + /* Wake up guardian. */ + + guardian->ping(); } +/************************************************************************** + }}} +**************************************************************************/ + + +/************************************************************************** + {{{ Static operations. +**************************************************************************/ + +/** + The operation is intended to check whether string is a well-formed + instance name or not. + + SYNOPSIS + is_name_valid() + name string to check + + RETURN + TRUE string is a valid instance name + FALSE string is not a valid instance name + + TODO: Move to Instance_name class: Instance_name::is_valid(). +*/ bool Instance::is_name_valid(const LEX_STRING *name) { @@ -405,21 +415,83 @@ bool Instance::is_name_valid(const LEX_STRING *name) } +/** + The operation is intended to check if the given instance name is + mysqld-compatible or not. + + SYNOPSIS + is_mysqld_compatible_name() + name name to check + + RETURN + TRUE name is mysqld-compatible + FALSE otherwise + + TODO: Move to Instance_name class: Instance_name::is_mysqld_compatible(). +*/ + bool Instance::is_mysqld_compatible_name(const LEX_STRING *name) { return strcmp(name->str, DFLT_INSTANCE_NAME.str) == 0; } +/** + Return client state name. Must not be used outside the class. + Use Instance::get_state_name() instead. +*/ -/* {{{ Constructor & destructor */ +const char * Instance::get_instance_state_name(enum_instance_state state) +{ + switch (state) { + case STOPPED: + return "offline"; + + case NOT_STARTED: + return "not started"; + + case STARTING: + return "starting"; + + case STARTED: + return "online"; + + case JUST_CRASHED: + return "failed"; + + case CRASHED: + return "crashed"; + + case CRASHED_AND_ABANDONED: + return "abandoned"; + + case STOPPING: + return "stopping"; + } + + return NULL; /* just to ignore compiler warning. */ +} + +/************************************************************************** + }}} +**************************************************************************/ + + +/************************************************************************** + {{{ Initialization & deinitialization. +**************************************************************************/ Instance::Instance() - :crashed(FALSE), - configured(FALSE) + :monitoring_thread_active(FALSE), + crashed(FALSE), + configured(FALSE), + /* mysqld_compatible is initialized in init() */ + state(NOT_STARTED), + restart_counter(0), + crash_moment(0), + last_checked(0) { pthread_mutex_init(&LOCK_instance, 0); - pthread_cond_init(&COND_instance_stopped, 0); } @@ -427,13 +499,11 @@ Instance::~Instance() { log_info("Instance '%s': destroying...", (const char *) get_name()->str); - pthread_cond_destroy(&COND_instance_stopped); pthread_mutex_destroy(&LOCK_instance); } -/* }}} */ -/* +/** Initialize instance options. SYNOPSIS @@ -453,7 +523,7 @@ bool Instance::init(const LEX_STRING *name_arg) } -/* +/** Complete instance options initialization. SYNOPSIS @@ -474,7 +544,47 @@ bool Instance::complete_initialization() */ } -/* +/************************************************************************** + }}} +**************************************************************************/ + + +/************************************************************************** + {{{ Instance: public interface implementation. +**************************************************************************/ + +/** + Determine if there is some activity with the instance. + + SYNOPSIS + is_active() + + DESCRIPTION + An instance is active if one of the following conditions is true: + - Instance-monitoring thread is running; + - Instance is guarded and its state is other than STOPPED; + - Corresponding mysqld-server accepts connections. + + MT-NOTE: instance must be locked before calling the operation. + + RETURN + TRUE - instance is active + FALSE - otherwise. +*/ + +bool Instance::is_active() +{ + if (monitoring_thread_active) + return TRUE; + + if (is_guarded() && get_state() != STOPPED) + return TRUE; + + return is_mysqld_running(); +} + + +/** Determine if mysqld is accepting connections. SYNOPSIS @@ -484,7 +594,7 @@ bool Instance::complete_initialization() Try to connect to mysqld with fake login/password to check whether it is accepting connections or not. - MT-NOTE: this operation must be called under acquired LOCK_instance. + MT-NOTE: instance must be locked before calling the operation. RETURN TRUE - mysqld is alive and accept connections @@ -508,8 +618,6 @@ bool Instance::is_mysqld_running() if (!port && !options.mysqld_socket) port= SERVER_DEFAULT_PORT; - pthread_mutex_lock(&LOCK_instance); - mysql_init(&mysql); /* try to connect to a server with a fake username/password pair */ if (mysql_real_connect(&mysql, LOCAL_HOST, username, @@ -523,7 +631,6 @@ bool Instance::is_mysqld_running() */ log_error("Instance '%s': was able to log into mysqld.", (const char *) get_name()->str); - pthread_mutex_unlock(&LOCK_instance); return_val= TRUE; /* server is alive */ } else @@ -531,145 +638,145 @@ bool Instance::is_mysqld_running() sizeof(access_denied_message) - 1)); mysql_close(&mysql); - pthread_mutex_unlock(&LOCK_instance); return return_val; } -/* - The method starts an instance. + +/** + Start mysqld. SYNOPSIS - start() - - RETURN - 0 ok - ER_CANNOT_START_INSTANCE Cannot start instance - ER_INSTANCE_ALREADY_STARTED The instance on the specified port/socket - is already started -*/ - -int Instance::start() -{ - /* clear crash flag */ - pthread_mutex_lock(&LOCK_instance); - crashed= FALSE; - pthread_mutex_unlock(&LOCK_instance); - - - if (configured && !is_mysqld_running()) - { - Instance_monitor *instance_monitor; - remove_pid(); - - instance_monitor= new Instance_monitor(this); - - if (instance_monitor == NULL || instance_monitor->start(Thread::DETACHED)) - { - delete instance_monitor; - log_error("Instance::start(): failed to create the monitoring thread" - " to start an instance"); - return ER_CANNOT_START_INSTANCE; - } - /* The monitoring thread will delete itself when it's finished. */ - - return 0; - } - - /* The instance is started already or misconfigured. */ - return configured ? ER_INSTANCE_ALREADY_STARTED : ER_INSTANCE_MISCONFIGURED; -} - -/* - The method sets the crash flag and wakes all waiters on - COND_instance_stopped and COND_guardian - - SYNOPSIS - set_crash_flag_n_wake_all() + start_mysqld() DESCRIPTION - The method is called when an instance is crashed or terminated. - In the former case it might indicate that guardian probably should - restart it. + Reset flags and start Instance Monitor thread, which will start mysqld. + + MT-NOTE: instance must be locked before calling the operation. RETURN - Function returns no value + FALSE - ok + TRUE - could not start instance */ -void Instance::set_crash_flag_n_wake_all() +bool Instance::start_mysqld() { - /* set instance state to crashed */ - pthread_mutex_lock(&LOCK_instance); - crashed= TRUE; - pthread_mutex_unlock(&LOCK_instance); + Instance_monitor *instance_monitor; /* - Wake connection threads waiting for an instance to stop. This - is needed if a user issued command to stop an instance via - mysql connection. This is not the case if Guardian stop the thread. + Prepare instance to start Instance Monitor thread. + + NOTE: It's important to set these actions here in order to avoid + race conditions -- these actions must be done under acquired lock on + Instance. */ - pthread_cond_signal(&COND_instance_stopped); - /* wake guardian */ - pthread_cond_signal(&Manager::get_guardian()->COND_guardian); -} + crashed= FALSE; + monitoring_thread_active= TRUE; -/* - Stop an instance. + remove_pid(); - SYNOPSIS - stop() + /* Create and start the Instance Monitor thread. */ - RETURN: - 0 ok - ER_INSTANCE_IS_NOT_STARTED Looks like the instance it is not started - ER_STOP_INSTANCE mysql_shutdown reported an error -*/ + instance_monitor= new Instance_monitor(this); -int Instance::stop() -{ - struct timespec timeout; - uint waitchild= (uint) DEFAULT_SHUTDOWN_DELAY; - - if (is_mysqld_running()) + if (instance_monitor == NULL || instance_monitor->start(Thread::DETACHED)) { - waitchild= options.get_shutdown_delay(); + delete instance_monitor; + monitoring_thread_active= FALSE; - kill_mysqld(SIGTERM); - /* sleep on condition to wait for SIGCHLD */ + log_error("Instance '%s': can not create instance monitor thread.", + (const char *) get_name()->str); - timeout.tv_sec= time(NULL) + waitchild; - timeout.tv_nsec= 0; - if (pthread_mutex_lock(&LOCK_instance)) - return ER_STOP_INSTANCE; - - while (options.load_pid() != 0) /* while server isn't stopped */ - { - int status; - - status= pthread_cond_timedwait(&COND_instance_stopped, - &LOCK_instance, - &timeout); - if (status == ETIMEDOUT || status == ETIME) - break; - } - - pthread_mutex_unlock(&LOCK_instance); - - kill_mysqld(SIGKILL); - - return 0; + return TRUE; } - return ER_INSTANCE_IS_NOT_STARTED; + ++restart_counter; + + /* The Instance Monitor thread will delete itself when it's finished. */ + + return FALSE; } -/* +/** + Stop mysqld. + + SYNOPSIS + stop_mysqld() + + DESCRIPTION + Try to stop mysqld gracefully. Otherwise kill it with SIGKILL. + + MT-NOTE: instance must be locked before calling the operation. + + RETURN + FALSE - ok + TRUE - could not stop the instance +*/ + +bool Instance::stop_mysqld() +{ + log_info("Instance '%s': stopping mysqld...", + (const char *) get_name()->str); + + kill_mysqld(SIGTERM); + + if (!wait_for_stop()) + { + log_info("Instance '%s': mysqld stopped gracefully.", + (const char *) get_name()->str); + return FALSE; + } + + log_info("Instance '%s': mysqld failed to stop gracefully within %d seconds.", + (const char *) get_name()->str, + (int) options.get_shutdown_delay()); + + log_info("Instance'%s': killing mysqld...", + (const char *) get_name()->str); + + kill_mysqld(SIGKILL); + + if (!wait_for_stop()) + { + log_info("Instance '%s': mysqld has been killed.", + (const char *) get_name()->str); + return FALSE; + } + + log_info("Instance '%s': can not kill mysqld within %d seconds.", + (const char *) get_name()->str, + (int) options.get_shutdown_delay()); + + return TRUE; +} + + +/** Send signal to mysqld. SYNOPSIS kill_mysqld() + + DESCRIPTION + Load pid from the pid file and send the given signal to that process. + If the signal is SIGKILL, remove the pid file after sending the signal. + + MT-NOTE: instance must be locked before calling the operation. + + TODO + This too low-level and OS-specific operation for public interface. + Also, it has some implicit behaviour for SIGKILL signal. Probably, we + should have the following public operations instead: + - start_mysqld() -- as is; + - stop_mysqld -- request mysqld to shutdown gracefully (send SIGTERM); + don't wait for complete shutdown; + - wait_for_stop() (or join_mysqld()) -- wait for mysqld to stop within + time interval; + - kill_mysqld() -- request to terminate mysqld; don't wait for + completion. + These operations should also be used in Guardian to manage instances. */ void Instance::kill_mysqld(int signum) @@ -707,27 +814,91 @@ void Instance::kill_mysqld(int signum) } } -/* - Return crashed flag. - SYNOPSIS - is_crashed() - - RETURN - TRUE - mysqld crashed - FALSE - mysqld hasn't crashed yet +/** + Lock instance. */ -bool Instance::is_crashed() +void Instance::lock() { - bool val; pthread_mutex_lock(&LOCK_instance); - val= crashed; - pthread_mutex_unlock(&LOCK_instance); - return val; } -/* + +/** + Unlock instance. +*/ + +void Instance::unlock() +{ + pthread_mutex_unlock(&LOCK_instance); +} + + +/** + Return instance state name. + + SYNOPSIS + get_state_name() + + DESCRIPTION + The operation returns user-friendly state name. The operation can be + used both for guarded and non-guarded instances. + + MT-NOTE: instance must be locked before calling the operation. + + TODO: Replace with the static get_state_name(state_code) function. +*/ + +const char *Instance::get_state_name() +{ + if (!is_configured()) + return "misconfigured"; + + if (is_guarded()) + { + /* The instance is managed by Guardian: we can report precise state. */ + + return get_instance_state_name(get_state()); + } + + /* The instance is not managed by Guardian: we can report status only. */ + + return is_active() ? "online" : "offline"; +} + + +/** + Reset statistics. + + SYNOPSIS + reset_stat() + + DESCRIPTION + The operation resets statistics used for guarding the instance. + + MT-NOTE: instance must be locked before calling the operation. + + TODO: Make private. +*/ + +void Instance::reset_stat() +{ + restart_counter= 0; + crash_moment= 0; + last_checked= 0; +} + +/************************************************************************** + }}} +**************************************************************************/ + + +/************************************************************************** + {{{ Instance: implementation of private operations. +**************************************************************************/ + +/** Remove pid file. */ @@ -744,3 +915,36 @@ void Instance::remove_pid() (const char *) options.instance_name.str); } } + + +/** + Wait for mysqld to stop within shutdown interval. +*/ + +bool Instance::wait_for_stop() +{ + int start_time= time(NULL); + int finish_time= start_time + options.get_shutdown_delay(); + + log_info("Instance '%s': waiting for mysqld to stop " + "(timeout: %d seconds)...", + (const char *) get_name()->str, + (int) options.get_shutdown_delay()); + + while (true) + { + if (options.load_pid() == 0 && !is_mysqld_running()) + return FALSE; + + if (time(NULL) >= finish_time) + return TRUE; + + /* Sleep for 0.3 sec and check again. */ + + my_sleep(300000); + } +} + +/************************************************************************** + }}} +**************************************************************************/ diff --git a/server-tools/instance-manager/instance.h b/server-tools/instance-manager/instance.h index 412d01acc46..5bdd8d61d2f 100644 --- a/server-tools/instance-manager/instance.h +++ b/server-tools/instance-manager/instance.h @@ -30,7 +30,7 @@ class Instance_map; class Thread_registry; -/* +/** Instance_name -- the class represents instance name -- a string of length less than MAX_INSTANCE_NAME_SIZE. @@ -68,72 +68,127 @@ private: class Instance { public: - /* - The following two constants defines name of the default mysqld-instance - ("mysqld"). + /* States of an instance. */ + enum enum_instance_state + { + STOPPED, + NOT_STARTED, + STARTING, + STARTED, + JUST_CRASHED, + CRASHED, + CRASHED_AND_ABANDONED, + STOPPING + }; + +public: + /** + The constant defines name of the default mysqld-instance ("mysqld"). */ static const LEX_STRING DFLT_INSTANCE_NAME; public: - /* - The operation is intended to check whether string is a well-formed - instance name or not. - */ static bool is_name_valid(const LEX_STRING *name); - - /* - The operation is intended to check if the given instance name is - mysqld-compatible or not. - */ static bool is_mysqld_compatible_name(const LEX_STRING *name); public: Instance(); - ~Instance(); + bool init(const LEX_STRING *name_arg); bool complete_initialization(); - bool is_mysqld_running(); - int start(); - int stop(); - /* send a signal to the instance */ - void kill_mysqld(int signo); - bool is_crashed(); - void set_crash_flag_n_wake_all(); +public: + bool is_active(); - /* + bool is_mysqld_running(); + + bool start_mysqld(); + bool stop_mysqld(); + void kill_mysqld(int signo); + + void lock(); + void unlock(); + + const char *get_state_name(); + + void reset_stat(); + +public: + /** The operation is intended to check if the instance is mysqld-compatible or not. */ inline bool is_mysqld_compatible() const; - /* + /** The operation is intended to check if the instance is configured properly or not. Misconfigured instances are not managed. */ inline bool is_configured() const; + /** + The operation returns TRUE if the instance is guarded and FALSE otherwise. + */ + inline bool is_guarded() const; + + /** + The operation returns name of the instance. + */ inline const LEX_STRING *get_name() const; + /** + The operation returns the current state of the instance. + + NOTE: At the moment should be used only for guarded instances. + */ + inline enum_instance_state get_state() const; + + /** + The operation changes the state of the instance. + + NOTE: At the moment should be used only for guarded instances. + TODO: Make private. + */ + inline void set_state(enum_instance_state new_state); + + /** + The operation returns crashed flag. + */ + inline bool is_crashed(); + public: - enum { DEFAULT_SHUTDOWN_DELAY= 35 }; + /** + This attributes contains instance options. + + TODO: Make private. + */ Instance_options options; private: - /* This attributes is a flag, specifies if the instance has been crashed. */ + /** + monitoring_thread_active is TRUE if there is a thread that monitors the + corresponding mysqld-process. + */ + bool monitoring_thread_active; + + /** + crashed is TRUE when corresponding mysqld-process has been died after + start. + */ bool crashed; - /* - This attribute specifies if the instance is configured properly or not. + /** + configured is TRUE when the instance is configured and FALSE otherwise. Misconfigured instances are not managed. */ bool configured; /* - This attribute specifies whether the instance is mysqld-compatible or not. - Mysqld-compatible instances can contain only mysqld-specific options. - At the moment an instance is mysqld-compatible if its name is "mysqld". + mysqld_compatible specifies whether the instance is mysqld-compatible + or not. Mysqld-compatible instances can contain only mysqld-specific + options. At the moment an instance is mysqld-compatible if its name is + "mysqld". The idea is that [mysqld] section should contain only mysqld-specific options (no Instance Manager-specific options) to be readable by mysqld @@ -142,18 +197,36 @@ private: bool mysqld_compatible; /* - Mutex protecting the instance. Currently we use it to avoid the - double start of the instance. This happens when the instance is starting - and we issue the start command once more. + Mutex protecting the instance. */ pthread_mutex_t LOCK_instance; - /* - This condition variable is used to wake threads waiting for instance to - stop in Instance::stop() - */ - pthread_cond_t COND_instance_stopped; - void remove_pid(); +private: + /* Guarded-instance attributes. */ + + /* state of an instance (i.e. STARTED, CRASHED, etc.) */ + enum_instance_state state; + +public: + /* the amount of attemts to restart instance (cleaned up at success) */ + int restart_counter; + + /* triggered at a crash */ + time_t crash_moment; + + /* General time field. Used to provide timeouts (at shutdown and restart) */ + time_t last_checked; + +private: + static const char *get_instance_state_name(enum_instance_state state); + +private: + void remove_pid(); + + bool wait_for_stop(); + +private: + friend class Instance_monitor; }; @@ -169,9 +242,33 @@ inline bool Instance::is_configured() const } +inline bool Instance::is_guarded() const +{ + return !options.nonguarded; +} + + inline const LEX_STRING *Instance::get_name() const { return &options.instance_name; } + +inline Instance::enum_instance_state Instance::get_state() const +{ + return state; +} + + +inline void Instance::set_state(enum_instance_state new_state) +{ + state= new_state; +} + + +inline bool Instance::is_crashed() +{ + return crashed; +} + #endif /* INCLUDES_MYSQL_INSTANCE_MANAGER_INSTANCE_H */ diff --git a/server-tools/instance-manager/instance_map.cc b/server-tools/instance-manager/instance_map.cc index a356e308e44..a9108eae763 100644 --- a/server-tools/instance-manager/instance_map.cc +++ b/server-tools/instance-manager/instance_map.cc @@ -25,26 +25,18 @@ #include #include "buffer.h" -#include "guardian.h" #include "instance.h" #include "log.h" -#include "manager.h" #include "mysqld_error.h" #include "mysql_manager_error.h" #include "options.h" #include "priv.h" -/* - Note: As we are going to suppost different types of connections, - we shouldn't have connection-specific functions. To avoid it we could - put such functions to the Command-derived class instead. - The command could be easily constructed for a specific connection if - we would provide a special factory for each connection. -*/ - C_MODE_START -/* Procedure needed for HASH initialization */ +/** + HASH-routines: get key of instance for storing in hash. +*/ static byte* get_instance_key(const byte* u, uint* len, my_bool __attribute__((unused)) t) @@ -54,14 +46,18 @@ static byte* get_instance_key(const byte* u, uint* len, return (byte *) instance->options.instance_name.str; } +/** + HASH-routines: cleanup handler. +*/ + static void delete_instance(void *u) { Instance *instance= (Instance *) u; delete instance; } -/* - The option handler to pass to the process_default_option_files finction. +/** + The option handler to pass to the process_default_option_files function. SYNOPSIS process_option() @@ -96,8 +92,8 @@ static int process_option(void *ctx, const char *group, const char *option) C_MODE_END -/* - Parse option string. +/** + Parse option string. SYNOPSIS parse_option() @@ -137,7 +133,7 @@ static void parse_option(const char *option_str, } -/* +/** Process one option from the configuration file. SYNOPSIS @@ -151,6 +147,10 @@ static void parse_option(const char *option_str, process_option(). The caller ensures proper locking of the instance map object. */ + /* + Process a given option and assign it to appropricate instance. This is + required for the option handler, passed to my_search_option_files(). + */ int Instance_map::process_one_option(const LEX_STRING *group, const char *option) @@ -213,92 +213,97 @@ int Instance_map::process_one_option(const LEX_STRING *group, } +/** + Instance_map constructor. +*/ + Instance_map::Instance_map() { pthread_mutex_init(&LOCK_instance_map, 0); } +/** + Initialize Instance_map internals. +*/ + bool Instance_map::init() { return hash_init(&hash, default_charset_info, START_HASH_SIZE, 0, 0, get_instance_key, delete_instance, 0); } + +/** + Reset Instance_map data. +*/ + +bool Instance_map::reset() +{ + hash_free(&hash); + return init(); +} + + +/** + Instance_map destructor. +*/ + Instance_map::~Instance_map() { - pthread_mutex_lock(&LOCK_instance_map); + lock(); + + /* + NOTE: it's necessary to synchronize on each instance before removal, + because Instance-monitoring thread can be still alive an hold the mutex + (because it is detached and we have no control over it). + */ + + while (true) + { + Iterator it(this); + Instance *instance= it.next(); + + if (!instance) + break; + + instance->lock(); + instance->unlock(); + + remove_instance(instance); + } + hash_free(&hash); - pthread_mutex_unlock(&LOCK_instance_map); + unlock(); + pthread_mutex_destroy(&LOCK_instance_map); } +/** + Lock Instance_map. +*/ + void Instance_map::lock() { pthread_mutex_lock(&LOCK_instance_map); } +/** + Unlock Instance_map. +*/ + void Instance_map::unlock() { pthread_mutex_unlock(&LOCK_instance_map); } -/* - Re-read instance configuration file. - SYNOPSIS - Instance_map::flush_instances() - - DESCRIPTION - This function will: - - clear the current list of instances. This removes both - running and stopped instances. - - load a new instance configuration from the file. - - pass on the new map to the guardian thread: it will start - all instances that are marked `guarded' and not yet started. - Note, as the check whether an instance is started is currently - very simple (returns TRUE if there is a MySQL server running - at the given port), this function has some peculiar - side-effects: - * if the port number of a running instance was changed, the - old instance is forgotten, even if it was running. The new - instance will be started at the new port. - * if the configuration was changed in a way that two - instances swapped their port numbers, the guardian thread - will not notice that and simply report that both instances - are configured successfully and running. - In order to avoid such side effects one should never call - FLUSH INSTANCES without prior stop of all running instances. - - NOTE: The operation should be invoked with the following locks acquired: - - Guardian; - - Instance_map; +/** + Check if there is an active instance or not. */ -int Instance_map::flush_instances() -{ - int rc; - - /* - Guardian thread relies on the instance map repository for guarding - instances. This is why refreshing instance map, we need (1) to stop - guardian (2) reload the instance map (3) reinitialize the guardian - with new instances. - */ - hash_free(&hash); - hash_init(&hash, default_charset_info, START_HASH_SIZE, 0, 0, - get_instance_key, delete_instance, 0); - - rc= load(); - /* don't init guardian if we failed to load instances */ - if (!rc) - guardian->init(); // TODO: check error status. - return rc; -} - - bool Instance_map::is_there_active_instance() { Instance *instance; @@ -306,29 +311,50 @@ bool Instance_map::is_there_active_instance() while ((instance= iterator.next())) { - if (guardian->find_instance_node(instance) != NULL || - instance->is_mysqld_running()) - { + bool active_instance_found; + + instance->lock(); + active_instance_found= instance->is_active(); + instance->unlock(); + + if (active_instance_found) return TRUE; - } } return FALSE; } +/** + Add an instance into the internal hash. + + MT-NOTE: Instance Map must be locked before calling the operation. +*/ + int Instance_map::add_instance(Instance *instance) { return my_hash_insert(&hash, (byte *) instance); } +/** + Remove instance from the internal hash. + + MT-NOTE: Instance Map must be locked before calling the operation. +*/ + int Instance_map::remove_instance(Instance *instance) { return hash_delete(&hash, (byte *) instance); } +/** + Create a new instance and register it in the internal hash. + + MT-NOTE: Instance Map must be locked before calling the operation. +*/ + int Instance_map::create_instance(const LEX_STRING *instance_name, const Named_value_arr *options) { @@ -392,12 +418,22 @@ int Instance_map::create_instance(const LEX_STRING *instance_name, } +/** + Return a pointer to the instance or NULL, if there is no such instance. + + MT-NOTE: Instance Map must be locked before calling the operation. +*/ + Instance * Instance_map::find(const LEX_STRING *name) { return (Instance *) hash_search(&hash, (byte *) name->str, name->length); } +/** + Init instances command line arguments after all options have been loaded. +*/ + bool Instance_map::complete_initialization() { bool mysqld_found; @@ -455,7 +491,10 @@ bool Instance_map::complete_initialization() } -/* load options from config files and create appropriate instance structures */ +/** + Load options from config files and create appropriate instance + structures. +*/ int Instance_map::load() { @@ -505,8 +544,9 @@ int Instance_map::load() } -/*--- Implementaton of the Instance map iterator class ---*/ - +/************************************************************************* + {{{ Instance_map::Iterator implementation. +*************************************************************************/ void Instance_map::Iterator::go_to_first() { @@ -522,29 +562,12 @@ Instance *Instance_map::Iterator::next() return NULL; } - -const char *Instance_map::get_instance_state_name(Instance *instance) -{ - LIST *instance_node; - - if (!instance->is_configured()) - return "misconfigured"; - - if ((instance_node= guardian->find_instance_node(instance)) != NULL) - { - /* The instance is managed by Guardian: we can report precise state. */ - - return Guardian::get_instance_state_name( - guardian->get_instance_state(instance_node)); - } - - /* The instance is not managed by Guardian: we can report status only. */ - - return instance->is_mysqld_running() ? "online" : "offline"; -} +/************************************************************************* + }}} +*************************************************************************/ -/* +/** Create a new configuration section for mysqld-instance in the config file. SYNOPSIS diff --git a/server-tools/instance-manager/instance_map.h b/server-tools/instance-manager/instance_map.h index 69d225c89f7..cdbac7635c2 100644 --- a/server-tools/instance-manager/instance_map.h +++ b/server-tools/instance-manager/instance_map.h @@ -37,14 +37,17 @@ extern int create_instance_in_file(const LEX_STRING *instance_name, const Named_value_arr *options); -/* +/** Instance_map - stores all existing instances */ class Instance_map { public: - /* Instance_map iterator */ + /** + Instance_map iterator + */ + class Iterator { private: @@ -58,79 +61,43 @@ public: void go_to_first(); Instance *next(); }; - friend class Iterator; + public: - /* - Return a pointer to the instance or NULL, if there is no such instance. - MT-NOTE: must be called under acquired lock. - */ Instance *find(const LEX_STRING *name); - /* Clear the configuration cache and reload the configuration file. */ - int flush_instances(); - - /* The operation is used to check if there is an active instance or not. */ bool is_there_active_instance(); void lock(); void unlock(); bool init(); + bool reset(); + + int load(); - /* - Process a given option and assign it to appropricate instance. This is - required for the option handler, passed to my_search_option_files(). - */ int process_one_option(const LEX_STRING *group, const char *option); - /* - Add an instance into the internal hash. - - MT-NOTE: the operation must be called under acquired lock. - */ int add_instance(Instance *instance); - /* - Remove instance from the internal hash. - - MT-NOTE: the operation must be called under acquired lock. - */ int remove_instance(Instance *instance); - /* - Create a new instance and register it in the internal hash. - - MT-NOTE: the operation must be called under acquired lock. - */ int create_instance(const LEX_STRING *instance_name, const Named_value_arr *options); +public: Instance_map(); ~Instance_map(); - /* - Retrieve client state name of the given instance. - - MT-NOTE: the options must be called under acquired locks of the following - objects: - - Instance_map; - - Guardian; - */ - const char *get_instance_state_name(Instance *instance); - -public: - const char *mysqld_path; - Guardian *guardian; - private: - /* loads options from config files */ - int load(); - /* inits instances argv's after all options have been loaded */ bool complete_initialization(); + private: enum { START_HASH_SIZE = 16 }; pthread_mutex_t LOCK_instance_map; HASH hash; + +private: + friend class Iterator; }; #endif /* INCLUDES_MYSQL_INSTANCE_MANAGER_INSTANCE_MAP_H */ diff --git a/server-tools/instance-manager/instance_options.h b/server-tools/instance-manager/instance_options.h index 7c1e1a8dcf3..38de839d762 100644 --- a/server-tools/instance-manager/instance_options.h +++ b/server-tools/instance-manager/instance_options.h @@ -46,7 +46,6 @@ public: Instance_options(); ~Instance_options(); - /* fills in argv */ bool complete_initialization(); bool set_option(Named_value *option); diff --git a/server-tools/instance-manager/manager.cc b/server-tools/instance-manager/manager.cc index a002902bd56..da36982f510 100644 --- a/server-tools/instance-manager/manager.cc +++ b/server-tools/instance-manager/manager.cc @@ -37,6 +37,9 @@ #include "user_map.h" +/********************************************************************** + {{{ Platform-specific implementation. +**********************************************************************/ #ifndef __WIN__ void set_signals(sigset_t *mask) @@ -92,9 +95,13 @@ int my_sigwait(const sigset_t *set, int *sig) #endif +/********************************************************************** + }}} +**********************************************************************/ + /********************************************************************** - Implementation of checking the actual thread model. + {{{ Implementation of checking the actual thread model. ***********************************************************************/ namespace { /* no-indent */ @@ -137,6 +144,10 @@ bool check_if_linux_threads(bool *linux_threads) } +/********************************************************************** + }}} +***********************************************************************/ + /********************************************************************** Manager implementation @@ -152,25 +163,37 @@ bool Manager::linux_threads; #endif // __WIN__ +/** + Request shutdown of guardian and threads registered in Thread_registry. + + SYNOPSIS + stop_all_threads() +*/ + void Manager::stop_all_threads() { /* - Let guardian thread know that it should break it's processing cycle, + Let Guardian thread know that it should break it's processing cycle, once it wakes up. */ p_guardian->request_shutdown(); - /* wake guardian */ - pthread_cond_signal(&p_guardian->COND_guardian); - /* stop all threads */ + + /* Stop all threads. */ p_thread_registry->deliver_shutdown(); } -/* - manager - entry point to the main instance manager process: start - listener thread, write pid file and enter into signal handling. - See also comments in mysqlmanager.cc to picture general Instance Manager - architecture. +/** + Main manager function. + + SYNOPSIS + main() + + DESCRIPTION + This is an entry point to the main instance manager process: + start listener thread, write pid file and enter into signal handling. + See also comments in mysqlmanager.cc to picture general Instance Manager + architecture. TODO: how about returning error status. */ @@ -194,22 +217,33 @@ int Manager::main() (const char *) (linux_threads ? "LINUX threads" : "POSIX threads")); #endif // __WIN__ - Thread_registry thread_registry; /* - All objects created in the manager() function live as long as - thread_registry lives, and thread_registry is alive until there are - working threads. + All objects created in the Manager object live as long as thread_registry + lives, and thread_registry is alive until there are working threads. + + There are two main purposes of the Thread Registry: + 1. Interrupt blocking I/O and signal condition variables in case of + shutdown; + 2. Wait for detached threads before shutting down the main thread. + + NOTE: + 1. Handling shutdown can be done in more elegant manner by introducing + Event (or Condition) object with support of logical operations. + 2. Using Thread Registry to wait for detached threads is definitely not + the best way, because when Thread Registry unregisters an thread, the + thread is still alive. Accurate way to wait for threads to stop is + not using detached threads and join all threads before shutdown. */ + Thread_registry thread_registry; User_map user_map; Instance_map instance_map; - Guardian guardian(&thread_registry, &instance_map, - Options::Main::monitoring_interval); + Guardian guardian(&thread_registry, &instance_map); Listener listener(&thread_registry, &user_map); p_instance_map= &instance_map; - p_guardian= instance_map.guardian= &guardian; + p_guardian= &guardian; p_thread_registry= &thread_registry; p_user_map= &user_map; @@ -249,7 +283,7 @@ int Manager::main() } } - /* write Instance Manager pid file */ + /* Write Instance Manager pid file. */ log_info("IM pid file: '%s'; PID: %d.", (const char *) Options::Main::pid_file_name, @@ -290,6 +324,7 @@ int Manager::main() permitted to process instances. And before flush_instances() has completed, there are no instances to guard. */ + if (guardian.start(Thread::DETACHED)) { log_error("Can not start Guardian thread."); @@ -298,21 +333,11 @@ int Manager::main() /* Load instances. */ + if (Manager::flush_instances()) { - instance_map.guardian->lock(); - instance_map.lock(); - - int flush_instances_status= instance_map.flush_instances(); - - instance_map.unlock(); - instance_map.guardian->unlock(); - - if (flush_instances_status) - { - log_error("Can not init instances repository."); - stop_all_threads(); - goto err; - } + log_error("Can not init instances repository."); + stop_all_threads(); + goto err; } /* Initialize the Listener. */ @@ -328,7 +353,8 @@ int Manager::main() After the list of guarded instances have been initialized, Guardian should start them. */ - pthread_cond_signal(&guardian.COND_guardian); + + guardian.ping(); /* Main loop. */ @@ -381,7 +407,6 @@ int Manager::main() if (!guardian.is_stopped()) { guardian.request_shutdown(); - pthread_cond_signal(&guardian.COND_guardian); } else { @@ -406,3 +431,64 @@ err: #endif return rc; } + + +/** + Re-read instance configuration file. + + SYNOPSIS + flush_instances() + + DESCRIPTION + This function will: + - clear the current list of instances. This removes both + running and stopped instances. + - load a new instance configuration from the file. + - pass on the new map to the guardian thread: it will start + all instances that are marked `guarded' and not yet started. + + Note, as the check whether an instance is started is currently + very simple (returns TRUE if there is a MySQL server running + at the given port), this function has some peculiar + side-effects: + * if the port number of a running instance was changed, the + old instance is forgotten, even if it was running. The new + instance will be started at the new port. + * if the configuration was changed in a way that two + instances swapped their port numbers, the guardian thread + will not notice that and simply report that both instances + are configured successfully and running. + + In order to avoid such side effects one should never call + FLUSH INSTANCES without prior stop of all running instances. +*/ + +bool Manager::flush_instances() +{ + p_instance_map->lock(); + + if (p_instance_map->is_there_active_instance()) + { + p_instance_map->unlock(); + return TRUE; + } + + if (p_instance_map->reset()) + { + p_instance_map->unlock(); + return TRUE; + } + + if (p_instance_map->load()) + { + p_instance_map->unlock(); + return TRUE; /* Don't init guardian if we failed to load instances. */ + } + + get_guardian()->init(); /* TODO: check error status. */ + get_guardian()->ping(); + + p_instance_map->unlock(); + + return FALSE; +} diff --git a/server-tools/instance-manager/manager.h b/server-tools/instance-manager/manager.h index a77809cca6d..9a00b9320ce 100644 --- a/server-tools/instance-manager/manager.h +++ b/server-tools/instance-manager/manager.h @@ -19,6 +19,7 @@ #if defined(__GNUC__) && defined(USE_PRAGMA_INTERFACE) #pragma interface #endif + #include class Guardian; @@ -30,8 +31,12 @@ class Manager { public: static int main(); + + static bool flush_instances(); + +public: /** - These methods return a non-zero value only for the duration + These methods return a non-NULL value only for the duration of main(). */ static Instance_map *get_instance_map() { return p_instance_map; } @@ -39,6 +44,7 @@ public: static Thread_registry *get_thread_registry() { return p_thread_registry; } static User_map *get_user_map() { return p_user_map; } +public: #ifndef __WIN__ static bool is_linux_threads() { return linux_threads; } #endif // __WIN__ diff --git a/server-tools/instance-manager/user_map.cc b/server-tools/instance-manager/user_map.cc index f3a6e3cd76c..7f34195c0b1 100644 --- a/server-tools/instance-manager/user_map.cc +++ b/server-tools/instance-manager/user_map.cc @@ -42,7 +42,7 @@ int User::init(const char *line) if (name_end == 0 || name_end[1] != ':') { log_error("Invalid format (unmatched quote) of user line (%s).", - (const char *) line); + (const char *) line); return 1; } password= name_end + 2; @@ -54,7 +54,7 @@ int User::init(const char *line) if (name_end == 0) { log_error("Invalid format (no delimiter) of user line (%s).", - (const char *) line); + (const char *) line); return 1; } password= name_end + 1; @@ -64,10 +64,10 @@ int User::init(const char *line) if (user_length > USERNAME_LENGTH) { log_error("User name is too long (%d). Max length: %d. " - "User line: '%s'.", - (int) user_length, - (int) USERNAME_LENGTH, - (const char *) line); + "User line: '%s'.", + (int) user_length, + (int) USERNAME_LENGTH, + (const char *) line); return 1; } @@ -75,10 +75,10 @@ int User::init(const char *line) if (password_length > SCRAMBLED_PASSWORD_CHAR_LENGTH) { log_error("Password is too long (%d). Max length: %d." - "User line: '%s'.", - (int) password_length, - (int) SCRAMBLED_PASSWORD_CHAR_LENGTH, - line); + "User line: '%s'.", + (int) password_length, + (int) SCRAMBLED_PASSWORD_CHAR_LENGTH, + (const char *) line); return 1; } From e1bdc140d163efd6c5e70f9b963a100ccfab65f5 Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Thu, 30 Nov 2006 18:43:33 -0700 Subject: [PATCH 06/23] WL#3602 Post review changes the --read-only option is not enforced for the slave thread in replication, or for the SUPER user. --- mysql-test/r/rpl_read_only.result | 113 +++++++++++++++++++++++++++ mysql-test/t/rpl_read_only-slave.opt | 1 + mysql-test/t/rpl_read_only.test | 105 +++++++++++++++++++++++++ sql/handler.cc | 6 +- sql/lock.cc | 6 +- 5 files changed, 229 insertions(+), 2 deletions(-) create mode 100644 mysql-test/r/rpl_read_only.result create mode 100644 mysql-test/t/rpl_read_only-slave.opt create mode 100644 mysql-test/t/rpl_read_only.test diff --git a/mysql-test/r/rpl_read_only.result b/mysql-test/r/rpl_read_only.result new file mode 100644 index 00000000000..0b06a3d414a --- /dev/null +++ b/mysql-test/r/rpl_read_only.result @@ -0,0 +1,113 @@ +stop slave; +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9; +reset master; +reset slave; +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9; +start slave; +create table t1(a int) engine=InnoDB; +create table t2(a int) engine=MyISAM; +insert into t1 values(1001); +insert into t2 values(2001); +set global read_only=1; +select @@read_only; +@@read_only +1 +select * from t1; +a +1001 +select * from t2; +a +2001 +select @@read_only; +@@read_only +0 +select * from t1; +a +1001 +select * from t2; +a +2001 +set global read_only=0; +BEGIN; +insert into t1 values(1002); +insert into t2 values(2002); +BEGIN; +insert into t1 values(1003); +insert into t2 values(2003); +set global read_only=1; +COMMIT; +COMMIT; +ERROR HY000: The MySQL server is running with the --read-only option so it cannot execute this statement +set global read_only=0; +insert into t1 values(1004); +insert into t2 values(2004); +select * from t1; +a +1001 +1002 +1004 +select * from t2; +a +2001 +2002 +2003 +2004 +select * from t1; +a +1001 +1002 +1004 +select * from t2; +a +2001 +2002 +2003 +2004 +set global read_only=1; +select @@read_only; +@@read_only +1 +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` int(11) DEFAULT NULL +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +show create table t2; +Table Create Table +t2 CREATE TABLE `t2` ( + `a` int(11) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +insert into t1 values(1005); +insert into t2 values(2005); +select * from t1; +a +1001 +1002 +1004 +1005 +select * from t2; +a +2001 +2002 +2003 +2004 +2005 +select * from t1; +a +1001 +1002 +1004 +1005 +select * from t2; +a +2001 +2002 +2003 +2004 +2005 +insert into t1 values(1006); +ERROR HY000: The MySQL server is running with the --read-only option so it cannot execute this statement +insert into t2 values(2006); +ERROR HY000: The MySQL server is running with the --read-only option so it cannot execute this statement +drop table t1; +drop table t2; diff --git a/mysql-test/t/rpl_read_only-slave.opt b/mysql-test/t/rpl_read_only-slave.opt new file mode 100644 index 00000000000..627becdbfb5 --- /dev/null +++ b/mysql-test/t/rpl_read_only-slave.opt @@ -0,0 +1 @@ +--innodb diff --git a/mysql-test/t/rpl_read_only.test b/mysql-test/t/rpl_read_only.test new file mode 100644 index 00000000000..659c3d10044 --- /dev/null +++ b/mysql-test/t/rpl_read_only.test @@ -0,0 +1,105 @@ +# Test case for BUG #11733 +-- source include/master-slave.inc +-- source include/have_innodb.inc + +# Setting the master readonly : +# - the variable @@readonly is not replicated on the slave + +connect (master2,127.0.0.1,test,,test,$MASTER_MYPORT,); +connect (slave2,127.0.0.1,test,,test,$SLAVE_MYPORT,); + +connection master1; + +create table t1(a int) engine=InnoDB; +create table t2(a int) engine=MyISAM; +insert into t1 values(1001); +insert into t2 values(2001); + +connection master; +set global read_only=1; + +connection master1; +select @@read_only; +select * from t1; +select * from t2; + +sync_slave_with_master; +select @@read_only; +select * from t1; +select * from t2; + +# - replication of transactions +connection master; +set global read_only=0; + +connection master1; +BEGIN; +insert into t1 values(1002); +insert into t2 values(2002); + +connection master2; +BEGIN; +insert into t1 values(1003); +insert into t2 values(2003); + +connection master; +set global read_only=1; + +connection master1; +## works even with read_only=1, because master1 is root +COMMIT; + +connection master2; +--error ER_OPTION_PREVENTS_STATEMENT +COMMIT; + +connection master; +set global read_only=0; + +connection master1; +insert into t1 values(1004); +insert into t2 values(2004); + +select * from t1; +select * from t2; + +sync_slave_with_master; +select * from t1; +select * from t2; + +# Setting the slave readonly : replication will pass +# +connection slave1; +set global read_only=1; + +connection slave; +select @@read_only; +# Make sure the replicated table is also transactional +show create table t1; +# Make sure the replicated table is not transactional +show create table t2; + +connection master; +insert into t1 values(1005); +insert into t2 values(2005); +select * from t1; +select * from t2; + +sync_slave_with_master; +connection slave; +select * from t1; +select * from t2; + +# Non root user can not write on the slave +connection slave2; +--error ER_OPTION_PREVENTS_STATEMENT +insert into t1 values(1006); +--error ER_OPTION_PREVENTS_STATEMENT +insert into t2 values(2006); + +## Cleanup +connection master; +drop table t1; +drop table t2; +sync_slave_with_master; + diff --git a/sql/handler.cc b/sql/handler.cc index e97fb7ddee4..2ba0be11ec4 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -693,7 +693,11 @@ int ha_commit_trans(THD *thd, bool all) DBUG_RETURN(1); } - if (is_real_trans && opt_readonly) + if ( is_real_trans + && opt_readonly + && ! (thd->security_ctx->master_access & SUPER_ACL) + && ! thd->slave_thread + ) { my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only"); ha_rollback_trans(thd, all); diff --git a/sql/lock.cc b/sql/lock.cc index 9886149ba77..be267f8d160 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -151,7 +151,11 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count, } } - if (write_lock_used && opt_readonly) + if ( write_lock_used + && opt_readonly + && ! (thd->security_ctx->master_access & SUPER_ACL) + && ! thd->slave_thread + ) { /* Someone has issued SET GLOBAL READ_ONLY=1 and we want a write lock. From 87a87bc04b0314908933e3f644e04dc2d2733ba4 Mon Sep 17 00:00:00 2001 From: "kostja@bodhi.local" <> Date: Fri, 1 Dec 2006 13:25:06 +0300 Subject: [PATCH 07/23] A fix and a test case for Bug#24179 "select b into $var" fails with --cursor_protocol": fix a misleading error message in case of SELECT .. INTO. --- sql/sql_class.cc | 14 ++++++++++++++ sql/sql_class.h | 12 ++++++++++-- sql/sql_prepare.cc | 3 +-- tests/mysql_client_test.c | 28 ++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 4 deletions(-) diff --git a/sql/sql_class.cc b/sql/sql_class.cc index d2f1e9ed0d9..f46cd2353eb 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -885,6 +885,13 @@ void select_result::cleanup() /* do nothing */ } +bool select_result::check_simple_select() const +{ + my_error(ER_SP_BAD_CURSOR_QUERY, MYF(0)); + return TRUE; +} + + static String default_line_term("\n",default_charset_info); static String default_escaped("\\",default_charset_info); static String default_field_term("\t",default_charset_info); @@ -1553,6 +1560,13 @@ int select_dumpvar::prepare(List &list, SELECT_LEX_UNIT *u) } +bool select_dumpvar::check_simple_select() const +{ + my_error(ER_SP_BAD_CURSOR_SELECT, MYF(0)); + return TRUE; +} + + void select_dumpvar::cleanup() { vars.empty(); diff --git a/sql/sql_class.h b/sql/sql_class.h index 41845dc5c76..ec03b9cb4a6 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -1724,7 +1724,14 @@ public: virtual bool initialize_tables (JOIN *join=0) { return 0; } virtual void send_error(uint errcode,const char *err); virtual bool send_eof()=0; - virtual bool simple_select() { return 0; } + /** + Check if this query returns a result set and therefore is allowed in + cursors and set an error message if it is not the case. + + @retval FALSE success + @retval TRUE error, an error message is set + */ + virtual bool check_simple_select() const; virtual void abort() {} /* Cleanup instance of this class for next execution of a prepared @@ -1762,7 +1769,7 @@ public: bool send_fields(List &list, uint flags); bool send_data(List &items); bool send_eof(); - bool simple_select() { return 1; } + virtual bool check_simple_select() const { return FALSE; } void abort(); }; @@ -2202,6 +2209,7 @@ public: int prepare(List &list, SELECT_LEX_UNIT *u); bool send_data(List &items); bool send_eof(); + virtual bool check_simple_select() const; void cleanup(); }; diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc index 1e7601c0951..8b2c2cd3674 100644 --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -2906,10 +2906,9 @@ bool Prepared_statement::execute(String *expanded_query, bool open_cursor) in INSERT ... SELECT and similar commands. */ - if (open_cursor && lex->result && !lex->result->simple_select()) + if (open_cursor && lex->result && lex->result->check_simple_select()) { DBUG_PRINT("info",("Cursor asked for not SELECT stmt")); - my_error(ER_SP_BAD_CURSOR_QUERY, MYF(0)); return TRUE; } diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c index c596a589d55..0b8d4d81558 100644 --- a/tests/mysql_client_test.c +++ b/tests/mysql_client_test.c @@ -15456,6 +15456,33 @@ static void test_bug21635() DBUG_VOID_RETURN; } +/* + Bug#24179 "select b into $var" fails with --cursor_protocol" + The failure is correct, check that the returned message is meaningful. +*/ + +static void test_bug24179() +{ + int rc; + MYSQL_STMT *stmt; + + DBUG_ENTER("test_bug24179"); + myheader("test_bug24179"); + + stmt= open_cursor("select 1 into @a"); + rc= mysql_stmt_execute(stmt); + DIE_UNLESS(rc); + if (!opt_silent) + { + printf("Got error (as expected): %d %s\n", + mysql_stmt_errno(stmt), + mysql_stmt_error(stmt)); + } + DIE_UNLESS(mysql_stmt_errno(stmt) == 1323); + + DBUG_VOID_RETURN; +} + /* Read and parse arguments and MySQL options from my.cnf @@ -15735,6 +15762,7 @@ static struct my_tests_st my_tests[]= { { "test_bug21726", test_bug21726 }, { "test_bug23383", test_bug23383 }, { "test_bug21635", test_bug21635 }, + { "test_bug24179", test_bug24179 }, { 0, 0 } }; From b44bb83a27c0e49e47a51586742c8eb219efd099 Mon Sep 17 00:00:00 2001 From: "petr/cps@outpost.site" <> Date: Mon, 4 Dec 2006 03:07:44 +0300 Subject: [PATCH 08/23] Fix Bug #21328 mysqld issues warnings on ALTER CSV table to MyISAM --- mysql-test/r/csv.result | 8 ++++++++ mysql-test/r/log_tables.result | 3 --- mysql-test/t/csv.test | 14 ++++++++++++++ storage/csv/ha_tina.cc | 6 ++++-- 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/mysql-test/r/csv.result b/mysql-test/r/csv.result index 32ca47e20d2..12cf900046f 100644 --- a/mysql-test/r/csv.result +++ b/mysql-test/r/csv.result @@ -5223,3 +5223,11 @@ check table bug22080_3; Table Op Msg_type Msg_text test.bug22080_3 check error Corrupt drop tables bug22080_1,bug22080_2,bug22080_3; +CREATE TABLE `bug21328` ( +`col1` int(11) DEFAULT NULL, +`col2` int(11) DEFAULT NULL, +`col3` int(11) DEFAULT NULL +) ENGINE=CSV; +insert into bug21328 values (1,NULL,NULL); +alter table bug21328 engine=myisam; +drop table bug21328; diff --git a/mysql-test/r/log_tables.result b/mysql-test/r/log_tables.result index d9d754c91e6..fc6891a3909 100644 --- a/mysql-test/r/log_tables.result +++ b/mysql-test/r/log_tables.result @@ -111,9 +111,6 @@ slow_log CREATE TABLE `slow_log` ( ) ENGINE=CSV DEFAULT CHARSET=utf8 COMMENT='Slow log' alter table mysql.general_log engine=myisam; alter table mysql.slow_log engine=myisam; -Warnings: -Warning 1366 Incorrect integer value: '' for column 'last_insert_id' at row 0 -Warning 1366 Incorrect integer value: '' for column 'insert_id' at row 0 show create table mysql.general_log; Table Create Table general_log CREATE TABLE `general_log` ( diff --git a/mysql-test/t/csv.test b/mysql-test/t/csv.test index 60d38394fc0..48c174355f4 100644 --- a/mysql-test/t/csv.test +++ b/mysql-test/t/csv.test @@ -1605,3 +1605,17 @@ check table bug22080_2; check table bug22080_3; drop tables bug22080_1,bug22080_2,bug22080_3; + +# +# Bug #21328 mysqld issues warnings on ALTER CSV table to MyISAM +# + +CREATE TABLE `bug21328` ( + `col1` int(11) DEFAULT NULL, + `col2` int(11) DEFAULT NULL, + `col3` int(11) DEFAULT NULL +) ENGINE=CSV; + +insert into bug21328 values (1,NULL,NULL); +alter table bug21328 engine=myisam; +drop table bug21328; diff --git a/storage/csv/ha_tina.cc b/storage/csv/ha_tina.cc index f7e5aa9d50c..6a6bc52d6ad 100644 --- a/storage/csv/ha_tina.cc +++ b/storage/csv/ha_tina.cc @@ -542,14 +542,16 @@ int ha_tina::encode_quote(byte *buf) const char *end_ptr; /* - Write an empty string to the buffer in case of a NULL value. + CSV does not support nulls. Write quoted 0 to the buffer. In fact, + (*field)->val_str(&attribute,&attribute) would usually return 0 + in this case but we write it explicitly here. Basically this is a safety check, as no one ensures that the field content is cleaned up every time we use Field::set_null() in the code. */ if ((*field)->is_null()) { - buffer.append(STRING_WITH_LEN("\"\",")); + buffer.append(STRING_WITH_LEN("\"0\",")); continue; } else From a9e1563fa44e631e1466f4c7044b2624f30f5240 Mon Sep 17 00:00:00 2001 From: "anozdrin/alik@booka." <> Date: Mon, 4 Dec 2006 14:05:27 +0300 Subject: [PATCH 09/23] Use standard shell instead of BASH. --- mysql-test/t/log.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mysql-test/t/log.sh b/mysql-test/t/log.sh index 20b265087cc..29cf8d3e1a3 100755 --- a/mysql-test/t/log.sh +++ b/mysql-test/t/log.sh @@ -1,4 +1,4 @@ -#!/bin/bash +#!/bin/sh ########################################################################### From 2bc1898bc6e0943bde5a2d4211a716b28ff364f4 Mon Sep 17 00:00:00 2001 From: "petr/cps@outpost.site" <> Date: Mon, 4 Dec 2006 19:48:49 +0300 Subject: [PATCH 10/23] Fix Bug #19044 IM aborts on exit On windows IM aborted on assert once one stoppped it. The reason is that we didn't close the sockets on windows and therefore, the listener thread wasn't able to finish. This happened because we used close() call for it. While on windows one should use closesocket(). On other platfroms we have appropriate defines for closesocket(), so this is the function which should be used. --- server-tools/instance-manager/listener.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server-tools/instance-manager/listener.cc b/server-tools/instance-manager/listener.cc index 3f60c2cc74a..70cd7360f0d 100644 --- a/server-tools/instance-manager/listener.cc +++ b/server-tools/instance-manager/listener.cc @@ -188,7 +188,7 @@ void Listener_thread::run() else { shutdown(client_fd, SHUT_RDWR); - close(client_fd); + closesocket(client_fd); } } } @@ -200,7 +200,7 @@ void Listener_thread::run() log_info("Listener_thread::run(): shutdown requested, exiting..."); for (i= 0; i < num_sockets; i++) - close(sockets[i]); + closesocket(sockets[i]); #ifndef __WIN__ unlink(unix_socket_address.sun_path); @@ -213,7 +213,7 @@ void Listener_thread::run() err: // we have to close the ip sockets in case of error for (i= 0; i < num_sockets; i++) - close(sockets[i]); + closesocket(sockets[i]); thread_registry.unregister_thread(&thread_info); thread_registry.request_shutdown(); @@ -260,7 +260,7 @@ int Listener_thread::create_tcp_socket() { log_error("Listener_thread::run(): bind(ip socket) failed, '%s'", strerror(errno)); - close(ip_socket); + closesocket(ip_socket); return -1; } @@ -268,7 +268,7 @@ int Listener_thread::create_tcp_socket() { log_error("Listener_thread::run(): listen(ip socket) failed, %s", strerror(errno)); - close(ip_socket); + closesocket(ip_socket); return -1; } From b593a7afdb159595da633c7dc72ee74b56a1f950 Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Mon, 4 Dec 2006 16:31:30 -0700 Subject: [PATCH 11/23] No bug number. Fixed typos in the comments. --- sql/sql_yacc.yy | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index fcac7eb8e05..dd41c19c9e7 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -6280,7 +6280,7 @@ function_call_nonkeyword: ; /* - Functions calls using a non reserved keywork, and using a regular syntax. + Functions calls using a non reserved keyword, and using a regular syntax. Because the non reserved keyword is used in another part of the grammar, a dedicated rule is needed here. */ @@ -6495,7 +6495,7 @@ function_call_generic: parser and the implementation in item_create.cc clean, since this will change with WL#2128 (SQL PATH): - INFORMATION_SCHEMA.version() is the SQL 99 syntax for the native - funtion version(), + function version(), - MySQL.version() is the SQL 2003 syntax for the native function version() (a vendor can specify any schema). */ @@ -6608,7 +6608,7 @@ sum_expr: { $$=new Item_sum_min($3); } /* According to ANSI SQL, DISTINCT is allowed and has - no sence inside MIN and MAX grouping functions; so MIN|MAX(DISTINCT ...) + no sense inside MIN and MAX grouping functions; so MIN|MAX(DISTINCT ...) is processed like an ordinary MIN | MAX() */ | MIN_SYM '(' DISTINCT in_sum_expr ')' From c25429f1a6fc8333e2c3edf7490c08b5c09c09d8 Mon Sep 17 00:00:00 2001 From: "petr/cps@outpost.site" <> Date: Tue, 5 Dec 2006 19:04:46 +0300 Subject: [PATCH 12/23] remove assert --- sql/event_data_objects.cc | 25 ++++++++++++++++--------- sql/event_queue.cc | 1 - 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/sql/event_data_objects.cc b/sql/event_data_objects.cc index ad7f0ab4e41..6c37b6f1d29 100644 --- a/sql/event_data_objects.cc +++ b/sql/event_data_objects.cc @@ -395,7 +395,14 @@ Event_parse_data::init_starts(THD *thd) if ((not_used= item_starts->get_date(<ime, TIME_NO_ZERO_DATE))) goto wrong_value; - /* Let's check whether time is in the past */ + /* + Let's check whether time is in the past. + Note: This call is not post year 2038 safe. That's because + thd->query_start() is of time_t, while gmt_sec_to_TIME() + wants my_time_t. In the case time_t is larger than my_time_t + an overflow might happen and events subsystem will not work as + expected. + */ thd->variables.time_zone->gmt_sec_to_TIME(&time_tmp, (my_time_t) thd->query_start()); @@ -407,12 +414,12 @@ Event_parse_data::init_starts(THD *thd) goto wrong_value; /* - This may result in a 1970-01-01 date if ltime is > 2037-xx-xx. - CONVERT_TZ has similar problem. - mysql_priv.h currently lists + Again, after 2038 this code won't work. As + mysql_priv.h currently lists #define TIMESTAMP_MAX_YEAR 2038 (see TIME_to_timestamp()) */ - my_tz_UTC->gmt_sec_to_TIME(<ime,t=TIME_to_timestamp(thd, <ime, ¬_used)); + my_tz_UTC->gmt_sec_to_TIME(<ime,t=TIME_to_timestamp(thd, <ime, + ¬_used)); if (!t) goto wrong_value; @@ -465,13 +472,13 @@ Event_parse_data::init_ends(THD *thd) goto error_bad_params; /* - This may result in a 1970-01-01 date if ltime is > 2037-xx-xx. - CONVERT_TZ has similar problem. - mysql_priv.h currently lists + Again, after 2038 this code won't work. As + mysql_priv.h currently lists #define TIMESTAMP_MAX_YEAR 2038 (see TIME_to_timestamp()) */ DBUG_PRINT("info", ("get the UTC time")); - my_tz_UTC->gmt_sec_to_TIME(<ime,t=TIME_to_timestamp(thd, <ime, ¬_used)); + my_tz_UTC->gmt_sec_to_TIME(<ime,t=TIME_to_timestamp(thd, <ime, + ¬_used)); if (!t) goto error_bad_params; diff --git a/sql/event_queue.cc b/sql/event_queue.cc index 7ec665fcd5f..9f7c1c9a9b2 100644 --- a/sql/event_queue.cc +++ b/sql/event_queue.cc @@ -160,7 +160,6 @@ Event_queue::init_queue(THD *thd, Event_db_repository *db_repo) { sql_print_error("SCHEDULER: sizeof(my_time_t) != sizeof(time_t) ." "The scheduler may not work correctly. Stopping"); - DBUG_ASSERT(0); goto err; } From 506c2a722fd2078c2005df71b52145633454559c Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Mon, 11 Dec 2006 16:59:02 -0700 Subject: [PATCH 13/23] Bug#19194 (Right recursion in parser for CASE causes excessive stack usage, limitation) Bug#24854 (Mixing Searched Case with Simple Case inside Stored Procedure crashes Mysqld) Implemented code review (19194) comments --- mysql-test/r/sp_stress_case.result | 114 ++++++++++++++++++++++++----- mysql-test/t/sp_stress_case.sh | 68 ----------------- mysql-test/t/sp_stress_case.test | 105 ++++++++++++++++++++------ sql/sql_yacc.yy | 43 ++++++++++- 4 files changed, 220 insertions(+), 110 deletions(-) delete mode 100755 mysql-test/t/sp_stress_case.sh diff --git a/mysql-test/r/sp_stress_case.result b/mysql-test/r/sp_stress_case.result index b85fb41d0bf..8ec68363c8d 100644 --- a/mysql-test/r/sp_stress_case.result +++ b/mysql-test/r/sp_stress_case.result @@ -1,42 +1,120 @@ -DROP PROCEDURE IF EXISTS bug_19194_a; -DROP PROCEDURE IF EXISTS bug_19194_b; -'Silently creating PROCEDURE bug_19194_a' -'Silently creating PROCEDURE bug_19194_b' -CALL bug_19194_a(1); +DROP PROCEDURE IF EXISTS proc_19194_codegen; +DROP PROCEDURE IF EXISTS bug_19194_simple; +DROP PROCEDURE IF EXISTS bug_19194_searched; +CREATE PROCEDURE proc_19194_codegen( +IN proc_name VARCHAR(50), +IN count INTEGER, +IN simple INTEGER, +OUT body MEDIUMTEXT) +BEGIN +DECLARE code MEDIUMTEXT; +DECLARE i INT DEFAULT 1; +SET code = concat("CREATE PROCEDURE ", proc_name, "(i INT)\n"); +SET code = concat(code, "BEGIN\n"); +SET code = concat(code, " DECLARE str CHAR(10);\n"); +IF (simple) +THEN +SET code = concat(code, " CASE i\n"); +ELSE +SET code = concat(code, " CASE\n"); +END IF; +WHILE (i <= count) +DO +IF (simple) +THEN +SET code = concat(code, " WHEN ", i, " THEN SET str=\"", i, "\";\n"); +ELSE +SET code = concat(code, " WHEN i=", i, " THEN SET str=\"", i, "\";\n"); +END IF; +SET i = i + 1; +END WHILE; +SET code = concat(code, " ELSE SET str=\"unknown\";\n"); +SET code = concat(code, " END CASE;\n"); +SET code = concat(code, " SELECT str;\n"); +SET code = concat(code, "END\n"); +SET body = code; +END| +set @body=""; +call proc_19194_codegen("test_simple", 10, 1, @body); +select @body; +@body +CREATE PROCEDURE test_simple(i INT) +BEGIN + DECLARE str CHAR(10); + CASE i + WHEN 1 THEN SET str="1"; + WHEN 2 THEN SET str="2"; + WHEN 3 THEN SET str="3"; + WHEN 4 THEN SET str="4"; + WHEN 5 THEN SET str="5"; + WHEN 6 THEN SET str="6"; + WHEN 7 THEN SET str="7"; + WHEN 8 THEN SET str="8"; + WHEN 9 THEN SET str="9"; + WHEN 10 THEN SET str="10"; + ELSE SET str="unknown"; + END CASE; + SELECT str; +END + +call proc_19194_codegen("test_searched", 10, 0, @body); +select @body; +@body +CREATE PROCEDURE test_searched(i INT) +BEGIN + DECLARE str CHAR(10); + CASE + WHEN i=1 THEN SET str="1"; + WHEN i=2 THEN SET str="2"; + WHEN i=3 THEN SET str="3"; + WHEN i=4 THEN SET str="4"; + WHEN i=5 THEN SET str="5"; + WHEN i=6 THEN SET str="6"; + WHEN i=7 THEN SET str="7"; + WHEN i=8 THEN SET str="8"; + WHEN i=9 THEN SET str="9"; + WHEN i=10 THEN SET str="10"; + ELSE SET str="unknown"; + END CASE; + SELECT str; +END + +CALL bug_19194_simple(1); str 1 -CALL bug_19194_a(2); +CALL bug_19194_simple(2); str 2 -CALL bug_19194_a(1000); +CALL bug_19194_simple(1000); str 1000 -CALL bug_19194_a(4998); +CALL bug_19194_simple(4998); str 4998 -CALL bug_19194_a(4999); +CALL bug_19194_simple(4999); str 4999 -CALL bug_19194_a(9999); +CALL bug_19194_simple(9999); str unknown -CALL bug_19194_b(1); +CALL bug_19194_searched(1); str 1 -CALL bug_19194_b(2); +CALL bug_19194_searched(2); str 2 -CALL bug_19194_b(1000); +CALL bug_19194_searched(1000); str 1000 -CALL bug_19194_b(4998); +CALL bug_19194_searched(4998); str 4998 -CALL bug_19194_b(4999); +CALL bug_19194_searched(4999); str 4999 -CALL bug_19194_b(9999); +CALL bug_19194_searched(9999); str unknown -DROP PROCEDURE bug_19194_a; -DROP PROCEDURE bug_19194_b; +DROP PROCEDURE proc_19194_codegen; +DROP PROCEDURE bug_19194_simple; +DROP PROCEDURE bug_19194_searched; diff --git a/mysql-test/t/sp_stress_case.sh b/mysql-test/t/sp_stress_case.sh deleted file mode 100755 index a2ac948e110..00000000000 --- a/mysql-test/t/sp_stress_case.sh +++ /dev/null @@ -1,68 +0,0 @@ -#!/bin/sh - -# -# Bug#19194 (Right recursion in parser for CASE causes excessive stack -# usage, limitation) -# -# Because the code for the CASE statement is so massive, -# checking in an already generated .test is not practical, -# due to it's size (10 000 lines or 356 000 bytes). -# -# Patches are sent by email, which introduce size limitations. -# -# As a result, code is generated dynamically here. -# This script takes no argument, and generates code in stdout. -# - -cat <&1 +delimiter |; -CALL bug_19194_a(1); -CALL bug_19194_a(2); -CALL bug_19194_a(1000); -CALL bug_19194_a(4998); -CALL bug_19194_a(4999); -CALL bug_19194_a(9999); +CREATE PROCEDURE proc_19194_codegen( + IN proc_name VARCHAR(50), + IN count INTEGER, + IN simple INTEGER, + OUT body MEDIUMTEXT) +BEGIN + DECLARE code MEDIUMTEXT; + DECLARE i INT DEFAULT 1; -CALL bug_19194_b(1); -CALL bug_19194_b(2); -CALL bug_19194_b(1000); -CALL bug_19194_b(4998); -CALL bug_19194_b(4999); -CALL bug_19194_b(9999); + SET code = concat("CREATE PROCEDURE ", proc_name, "(i INT)\n"); + SET code = concat(code, "BEGIN\n"); + SET code = concat(code, " DECLARE str CHAR(10);\n"); -DROP PROCEDURE bug_19194_a; -DROP PROCEDURE bug_19194_b; + IF (simple) + THEN + SET code = concat(code, " CASE i\n"); + ELSE + SET code = concat(code, " CASE\n"); + END IF; + + WHILE (i <= count) + DO + IF (simple) + THEN + SET code = concat(code, " WHEN ", i, " THEN SET str=\"", i, "\";\n"); + ELSE + SET code = concat(code, " WHEN i=", i, " THEN SET str=\"", i, "\";\n"); + END IF; + + SET i = i + 1; + END WHILE; + + SET code = concat(code, " ELSE SET str=\"unknown\";\n"); + SET code = concat(code, " END CASE;\n"); + SET code = concat(code, " SELECT str;\n"); + + SET code = concat(code, "END\n"); + + SET body = code; +END| + +delimiter ;| + +set @body=""; +call proc_19194_codegen("test_simple", 10, 1, @body); +select @body; +call proc_19194_codegen("test_searched", 10, 0, @body); +select @body; + +--disable_query_log +call proc_19194_codegen("bug_19194_simple", 5000, 1, @body); +let $proc_body = `select @body`; +eval $proc_body; +call proc_19194_codegen("bug_19194_searched", 5000, 1, @body); +let $proc_body = `select @body`; +eval $proc_body; +--enable_query_log + +CALL bug_19194_simple(1); +CALL bug_19194_simple(2); +CALL bug_19194_simple(1000); +CALL bug_19194_simple(4998); +CALL bug_19194_simple(4999); +CALL bug_19194_simple(9999); + +CALL bug_19194_searched(1); +CALL bug_19194_searched(2); +CALL bug_19194_searched(1000); +CALL bug_19194_searched(4998); +CALL bug_19194_searched(4999); +CALL bug_19194_searched(9999); + +DROP PROCEDURE proc_19194_codegen; +DROP PROCEDURE bug_19194_simple; +DROP PROCEDURE bug_19194_searched; diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 57b6201d4a9..014be54eef6 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -100,6 +100,42 @@ void turn_parser_debug_on() /** Helper action for a case statement (entering the CASE). This helper is used for both 'simple' and 'searched' cases. + This helper, with the other case_stmt_action_..., is executed when + the following SQL code is parsed: +
+CREATE PROCEDURE proc_19194_simple(i int)
+BEGIN
+  DECLARE str CHAR(10);
+
+  CASE i
+    WHEN 1 THEN SET str="1";
+    WHEN 2 THEN SET str="2";
+    WHEN 3 THEN SET str="3";
+    ELSE SET str="unknown";
+  END CASE;
+
+  SELECT str;
+END
+
+ The actions are used to generate the following code: +
+SHOW PROCEDURE CODE proc_19194_simple;
+Pos     Instruction
+0       set str@1 NULL
+1       set_case_expr (12) 0 i@0
+2       jump_if_not 5(12) (case_expr@0 = 1)
+3       set str@1 _latin1'1'
+4       jump 12
+5       jump_if_not 8(12) (case_expr@0 = 2)
+6       set str@1 _latin1'2'
+7       jump 12
+8       jump_if_not 11(12) (case_expr@0 = 3)
+9       set str@1 _latin1'3'
+10      jump 12
+11      set str@1 _latin1'unknown'
+12      stmt 0 "SELECT str"
+
+ @param lex the parser lex context */ @@ -110,6 +146,7 @@ void case_stmt_action_case(LEX *lex) /* BACKPATCH: Creating target label for the jump to "case_stmt_action_end_case" + (Instruction 12 in the example) */ lex->spcont->push_label((char *)"", lex->sphead->instructions()); @@ -179,6 +216,7 @@ void case_stmt_action_when(LEX *lex, Item *when, bool simple) /* BACKPATCH: Registering forward jump from "case_stmt_action_when" to "case_stmt_action_then" + (jump_if_not from instruction 2 to 5, 5 to 8 ... in the example) */ sp->push_backpatch(i, ctx->push_label((char *)"", 0)); @@ -203,13 +241,15 @@ void case_stmt_action_then(LEX *lex) /* BACKPATCH: Resolving forward jump from "case_stmt_action_when" to "case_stmt_action_then" + (jump_if_not from instruction 2 to 5, 5 to 8 ... in the example) */ sp->backpatch(ctx->pop_label()); /* BACKPATCH: Registering forward jump from - "case_stmt_action_when" to "case_stmt_action_end_case" + "case_stmt_action_then" to "case_stmt_action_end_case" + (jump from instruction 4 to 12, 7 to 12 ... in the example) */ sp->push_backpatch(i, ctx->last_label()); @@ -227,6 +267,7 @@ void case_stmt_action_end_case(LEX *lex, bool simple) /* BACKPATCH: Resolving forward jump from "case_stmt_action_then" to "case_stmt_action_end_case" + (jump from instruction 4 to 12, 7 to 12 ... in the example) */ lex->sphead->backpatch(lex->spcont->pop_label()); From 7696592b091740c2dac49704c5d9e1574f12bf30 Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Mon, 11 Dec 2006 18:52:24 -0700 Subject: [PATCH 14/23] minor cleanup --- mysql-test/t/sp_stress_case.test | 5 ----- 1 file changed, 5 deletions(-) diff --git a/mysql-test/t/sp_stress_case.test b/mysql-test/t/sp_stress_case.test index 5b278802e26..1b5bd8991a9 100644 --- a/mysql-test/t/sp_stress_case.test +++ b/mysql-test/t/sp_stress_case.test @@ -3,11 +3,6 @@ # usage, limitation) # -# This test takes some time (8 min) in debug builds -# It's provided as a separate file so that the next line can be uncommented -# later if needed: -# -- source include/big_test.inc - --disable_warnings DROP PROCEDURE IF EXISTS proc_19194_codegen; DROP PROCEDURE IF EXISTS bug_19194_simple; From b893d347a19c55202196b8a35a94248224dcaa82 Mon Sep 17 00:00:00 2001 From: "kostja@bodhi.local" <> Date: Tue, 12 Dec 2006 23:22:46 +0300 Subject: [PATCH 15/23] Post-merge fixes. --- mysql-test/r/read_only.result | 2 +- server-tools/instance-manager/guardian.cc | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/mysql-test/r/read_only.result b/mysql-test/r/read_only.result index d479fdb4c3c..f270f1ed5ad 100644 --- a/mysql-test/r/read_only.result +++ b/mysql-test/r/read_only.result @@ -89,11 +89,11 @@ select @@global.read_only; @@global.read_only 1 unlock tables; -set global read_only=0; drop temporary table ttt; ERROR 42S02: Unknown table 'ttt' drop temporary table if exists ttt; Warnings: Note 1051 Unknown table 'ttt' +set global read_only=0; drop table t1,t2; drop user test@localhost; diff --git a/server-tools/instance-manager/guardian.cc b/server-tools/instance-manager/guardian.cc index 4c601537075..1b451cd9933 100644 --- a/server-tools/instance-manager/guardian.cc +++ b/server-tools/instance-manager/guardian.cc @@ -53,8 +53,7 @@ Guardian::Guardian(Thread_registry *thread_registry_arg, :shutdown_requested(FALSE), stopped(FALSE), thread_registry(thread_registry_arg), - instance_map(instance_map_arg), - guarded_instances(0) + instance_map(instance_map_arg) { pthread_mutex_init(&LOCK_guardian, 0); pthread_cond_init(&COND_guardian, 0); From c55862698ebc7aafdfeeba47170c9f7b89b66eec Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Tue, 12 Dec 2006 16:42:35 -0700 Subject: [PATCH 16/23] Merge cleanup --- sql/sql_yacc.yy | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index a09dfa25e31..bf5369d5553 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -1667,8 +1667,7 @@ ev_sql_stmt_inner: sp_proc_stmt_statement | sp_proc_stmt_return | sp_proc_stmt_if - | sp_proc_stmt_case_simple - | sp_proc_stmt_case + | case_stmt_specification | sp_labeled_control | sp_proc_stmt_unlabeled | sp_proc_stmt_leave @@ -2432,8 +2431,7 @@ sp_proc_stmt: sp_proc_stmt_statement | sp_proc_stmt_return | sp_proc_stmt_if - | sp_proc_stmt_case_simple - | sp_proc_stmt_case + | case_stmt_specification | sp_labeled_control | sp_proc_stmt_unlabeled | sp_proc_stmt_leave @@ -2523,14 +2521,6 @@ sp_proc_stmt_return: } ; -sp_proc_stmt_case_simple: - CASE_SYM WHEN_SYM - ; - -sp_proc_stmt_case: - CASE_SYM - ; - sp_proc_stmt_unlabeled: { /* Unlabeled controls get a secret label. */ LEX *lex= Lex; From f2347e402d870b5244a6467ccd897933a5fe7deb Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Tue, 19 Dec 2006 15:02:37 -0700 Subject: [PATCH 17/23] Bug#25183 (Compiler warnings in the sql directory (GCC 4.1)) Fixed compiler warnings. In set_var.cc, the code was not properly returning an error code if close_cached_tables() failed. In sql_tables.cc, the code was not returning properly an error code if lock_table_names() failed. Both cases are bugs, introduced in 5.1 only by recent changes. --- sql/set_var.cc | 2 +- sql/sql_table.cc | 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/sql/set_var.cc b/sql/set_var.cc index cf8751bb895..d7e42bd16fb 100644 --- a/sql/set_var.cc +++ b/sql/set_var.cc @@ -3904,7 +3904,7 @@ bool sys_var_opt_readonly::update(THD *thd, set_var *var) can cause to wait on a read lock, it's required for the client application to unlock everything, and acceptable for the server to wait on all locks. */ - if (close_cached_tables(thd, true, NULL, false)) + if (result= close_cached_tables(thd, true, NULL, false)) goto end_with_read_lock; if (result= make_global_read_lock_block_commit(thd)) diff --git a/sql/sql_table.cc b/sql/sql_table.cc index c8f6e09fecb..5ca59911c9d 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -5406,16 +5406,19 @@ bool mysql_alter_table(THD *thd,char *new_db, char *new_name, { my_message(ER_LOCK_OR_ACTIVE_TRANSACTION, ER(ER_LOCK_OR_ACTIVE_TRANSACTION), MYF(0)); - DBUG_RETURN(1); + DBUG_RETURN(TRUE); } if (wait_if_global_read_lock(thd,0,1)) - DBUG_RETURN(1); + DBUG_RETURN(TRUE); VOID(pthread_mutex_lock(&LOCK_open)); if (lock_table_names(thd, table_list)) + { + error= TRUE; goto view_err; + } - error=0; + error= FALSE; if (!do_rename(thd, table_list, new_db, new_name, new_name, 1)) { if (mysql_bin_log.is_open()) @@ -5556,6 +5559,10 @@ view_err: error=table->file->disable_indexes(HA_KEY_SWITCH_NONUNIQ_SAVE); /* COND_refresh will be signaled in close_thread_tables() */ break; + default: + DBUG_ASSERT(FALSE); + error= 0; + break; } if (error == HA_ERR_WRONG_COMMAND) { From 564309e416af895751577aa2f33dd5c9f3d61cfd Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Tue, 2 Jan 2007 17:49:05 -0700 Subject: [PATCH 18/23] Merge 5.1-main -> 5.1-runtime, added ignore entry for missing symbolic link --- .bzrignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.bzrignore b/.bzrignore index 73cb4341beb..e00d2a16187 100644 --- a/.bzrignore +++ b/.bzrignore @@ -1150,6 +1150,7 @@ libmysqld/sql_prepare.cc libmysqld/sql_rename.cc libmysqld/sql_repl.cc libmysqld/sql_select.cc +libmysqld/sql_servers.cc libmysqld/sql_show.cc libmysqld/sql_state.c libmysqld/sql_string.cc From 4cae0119fdaddd2f990c010abf44e72c74e5e750 Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Tue, 2 Jan 2007 22:16:24 -0700 Subject: [PATCH 19/23] Bug#25302 (893 reduce/reduce conflicts in parser grammar) This fix corrects build issues introduced by WL#3031: - In the SQL grammar, 'USER' is a SQL 2003 reserved keyword, and therefore should not be part of the keyword production. - In sql/sql_parse.cc, the code for CREATE SERVER and ALTER SERVER was not using proper format strings in the DBUG_PRINT statements. --- sql/sql_parse.cc | 4 ++-- sql/sql_yacc.yy | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 01aa7565e28..33ae6d96c91 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -5195,7 +5195,7 @@ create_sp_error: DBUG_PRINT("info", ("case SQLCOM_CREATE_SERVER")); if ((error= create_server(thd, &lex->server_options))) { - DBUG_PRINT("info", ("problem creating server", + DBUG_PRINT("info", ("problem creating server <%s>", lex->server_options.server_name)); my_error(error, MYF(0), lex->server_options.server_name); break; @@ -5210,7 +5210,7 @@ create_sp_error: DBUG_PRINT("info", ("case SQLCOM_ALTER_SERVER")); if ((error= alter_server(thd, &lex->server_options))) { - DBUG_PRINT("info", ("problem altering server", + DBUG_PRINT("info", ("problem altering server <%s>", lex->server_options.server_name)); my_error(error, MYF(0), lex->server_options.server_name); break; diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 6938a7b10d1..d541724a133 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -9589,7 +9589,6 @@ keyword: | TRUNCATE_SYM {} | UNICODE_SYM {} | UNINSTALL_SYM {} - | USER {} | WRAPPER_SYM {} | XA_SYM {} | UPGRADE_SYM {} From 5b4d9d87061e92649ba404a882592c8d96bda141 Mon Sep 17 00:00:00 2001 From: "kroki/tomash@moonlight.home" <> Date: Tue, 9 Jan 2007 12:24:25 +0300 Subject: [PATCH 20/23] BUG#23443: user-defined variables can consume too much memory in the server The problem was that when memory was exhausted HEAP engine could crash (GROUP BY uses HEAP TABLE). Alternatively, if SET was used, it could report an error "You may only use constant expressions with SET" instead of "Out of memory (Needed NNNNNN bytes)". The solution is: - pass MY_WME to (some) calls to my_malloc() to get correct message. - fix heap_write() so that the first key is skipped during cleanup on ENOMEM because it wasn't inserted and doesn't have to be deleted. No test case is provided because we can't test out-of-memory behaviour in our current test framework. --- heap/hp_block.c | 2 +- heap/hp_write.c | 15 ++++++++++++--- sql/item_func.cc | 5 +++-- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/heap/hp_block.c b/heap/hp_block.c index 5c052218e58..8afcaf8d945 100644 --- a/heap/hp_block.c +++ b/heap/hp_block.c @@ -47,7 +47,7 @@ int _hp_get_new_block(HP_BLOCK *block, ulong *alloc_length) break; *alloc_length=sizeof(HP_PTRS)*i+block->records_in_block* block->recbuffer; - if (!(root=(HP_PTRS*) my_malloc(*alloc_length,MYF(0)))) + if (!(root=(HP_PTRS*) my_malloc(*alloc_length,MYF(MY_WME)))) return 1; if (i == 0) diff --git a/heap/hp_write.c b/heap/hp_write.c index 18fa95e7760..b5349d74691 100644 --- a/heap/hp_write.c +++ b/heap/hp_write.c @@ -66,13 +66,22 @@ int heap_write(HP_INFO *info, const byte *record) DBUG_RETURN(0); err: - DBUG_PRINT("info",("Duplicate key: %d",key)); + if (my_errno == HA_ERR_FOUND_DUPP_KEY) + DBUG_PRINT("info",("Duplicate key: %d",key)); info->errkey= key; - do + /* + Because 'key' is unsigned, we increase it before the loop, unless + we have to skip the key that wasn't inserted yet due to OOM. In + the loop we test 'key' before decreasing it as the protection + against value wraparound. + */ + if (my_errno != ENOMEM) + key++; + while (key-- > 0) { if (_hp_delete_key(info,share->keydef+key,record,pos,0)) break; - } while (key-- > 0); + } share->deleted++; *((byte**) pos)=share->del_link; diff --git a/sql/item_func.cc b/sql/item_func.cc index e83d1f35db8..421f72ab69b 100644 --- a/sql/item_func.cc +++ b/sql/item_func.cc @@ -1892,8 +1892,9 @@ bool Item_func_set_user_var::update_hash(const void *ptr, uint length, char *pos= (char*) entry+ ALIGN_SIZE(sizeof(user_var_entry)); if (entry->value == pos) entry->value=0; - if (!(entry->value=(char*) my_realloc(entry->value, length, - MYF(MY_ALLOW_ZERO_PTR)))) + entry->value= (char*) my_realloc(entry->value, length, + MYF(MY_ALLOW_ZERO_PTR | MY_WME)); + if (!entry->value) goto err; } } From 067b80a8d3ebc84d2253231d0d630045fac85acd Mon Sep 17 00:00:00 2001 From: "kroki/tomash@moonlight.home" <> Date: Tue, 9 Jan 2007 13:07:39 +0300 Subject: [PATCH 21/23] Fix after manual merge. --- storage/heap/hp_write.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/heap/hp_write.c b/storage/heap/hp_write.c index fd51b6f00c2..86e79c9d7ec 100644 --- a/storage/heap/hp_write.c +++ b/storage/heap/hp_write.c @@ -68,7 +68,7 @@ int heap_write(HP_INFO *info, const byte *record) err: if (my_errno == HA_ERR_FOUND_DUPP_KEY) - DBUG_PRINT("info",("Duplicate key: %d", (int) keydef - share->keydef)); + DBUG_PRINT("info",("Duplicate key: %d", (int) (keydef - share->keydef))); info->errkey= keydef - share->keydef; /* We don't need to delete non-inserted key from rb-tree. Also, if From 9655f5fb0330d9eb67effe6755dec45f28f1e446 Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Thu, 11 Jan 2007 14:58:05 -0700 Subject: [PATCH 22/23] Bug#22687 (Functions UNIQUE_USERS, GROUP_UNIQUE_USERS) According to some internal communication, these two functions are place holders for future enhancements. Because they use a variable number of parameters, the implementation defined a reserved keyword for them in the parser grammar. Unfortunately, doing so creates a bug similar to Bug 21114 reported for the function FORMAT. In the 5.1 code base, due to improvements in the code implemented with bug 21114, having a reserved keyword for functions with a variable number of arguments is not needed any more by the implementation. As a result, this fix removes the place-holder implementation, and removes the unnecessary reserved keywords. Should the functions UNIQUE_USERS and GROUP_UNIQUE_USERS be finally implemented in a later release, the implementation should sub class Create_native_func in sql/item_create.cc. For example, see the class Create_func_concat. --- libmysqld/Makefile.am | 2 +- mysql-test/r/parser.result | 12 ++++---- mysql-test/t/parser.test | 16 ++++++---- sql/Makefile.am | 4 +-- sql/item.h | 1 - sql/item_sum.h | 2 +- sql/item_uniq.cc | 32 ------------------- sql/item_uniq.h | 63 -------------------------------------- sql/lex.h | 2 -- sql/mysql_priv.h | 1 + sql/sql_yacc.yy | 8 ----- 11 files changed, 21 insertions(+), 122 deletions(-) delete mode 100644 sql/item_uniq.cc delete mode 100644 sql/item_uniq.h diff --git a/libmysqld/Makefile.am b/libmysqld/Makefile.am index cf4f90d99c9..a4fbaeafaa7 100644 --- a/libmysqld/Makefile.am +++ b/libmysqld/Makefile.am @@ -51,7 +51,7 @@ sqlsources = derror.cc field.cc field_conv.cc strfunc.cc filesort.cc \ hostname.cc init.cc password.c \ item.cc item_buff.cc item_cmpfunc.cc item_create.cc \ item_func.cc item_strfunc.cc item_sum.cc item_timefunc.cc \ - item_geofunc.cc item_uniq.cc item_subselect.cc item_row.cc\ + item_geofunc.cc item_subselect.cc item_row.cc\ item_xmlfunc.cc \ key.cc lock.cc log.cc log_event.cc sql_state.c \ protocol.cc net_serv.cc opt_range.cc \ diff --git a/mysql-test/r/parser.result b/mysql-test/r/parser.result index 49008eb818b..cb44a235f25 100644 --- a/mysql-test/r/parser.result +++ b/mysql-test/r/parser.result @@ -49,7 +49,7 @@ ERROR 42000: You have an error in your SQL syntax; check the manual that corresp create table GROUP_CONCAT (a int); drop table GROUP_CONCAT; create table GROUP_UNIQUE_USERS(a int); -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'GROUP_UNIQUE_USERS(a int)' at line 1 +drop table GROUP_UNIQUE_USERS; create table GROUP_UNIQUE_USERS (a int); drop table GROUP_UNIQUE_USERS; create table MAX(a int); @@ -121,7 +121,7 @@ ERROR 42000: You have an error in your SQL syntax; check the manual that corresp create table TRIM (a int); drop table TRIM; create table UNIQUE_USERS(a int); -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'UNIQUE_USERS(a int)' at line 1 +drop table UNIQUE_USERS; create table UNIQUE_USERS (a int); drop table UNIQUE_USERS; create table VARIANCE(a int); @@ -186,9 +186,9 @@ ERROR 42000: You have an error in your SQL syntax; check the manual that corresp create table GROUP_CONCAT (a int); ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'GROUP_CONCAT (a int)' at line 1 create table GROUP_UNIQUE_USERS(a int); -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'GROUP_UNIQUE_USERS(a int)' at line 1 +drop table GROUP_UNIQUE_USERS; create table GROUP_UNIQUE_USERS (a int); -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'GROUP_UNIQUE_USERS (a int)' at line 1 +drop table GROUP_UNIQUE_USERS; create table MAX(a int); ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'MAX(a int)' at line 1 create table MAX (a int); @@ -258,9 +258,9 @@ ERROR 42000: You have an error in your SQL syntax; check the manual that corresp create table TRIM (a int); ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'TRIM (a int)' at line 1 create table UNIQUE_USERS(a int); -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'UNIQUE_USERS(a int)' at line 1 +drop table UNIQUE_USERS; create table UNIQUE_USERS (a int); -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'UNIQUE_USERS (a int)' at line 1 +drop table UNIQUE_USERS; create table VARIANCE(a int); ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'VARIANCE(a int)' at line 1 create table VARIANCE (a int); diff --git a/mysql-test/t/parser.test b/mysql-test/t/parser.test index 13b4338701e..65aa9dbb89b 100644 --- a/mysql-test/t/parser.test +++ b/mysql-test/t/parser.test @@ -77,8 +77,9 @@ create table GROUP_CONCAT(a int); create table GROUP_CONCAT (a int); drop table GROUP_CONCAT; ---error ER_PARSE_ERROR +# Limitation removed in 5.1 create table GROUP_UNIQUE_USERS(a int); +drop table GROUP_UNIQUE_USERS; create table GROUP_UNIQUE_USERS (a int); drop table GROUP_UNIQUE_USERS; @@ -167,8 +168,9 @@ create table TRIM(a int); create table TRIM (a int); drop table TRIM; ---error ER_PARSE_ERROR +# Limitation removed in 5.1 create table UNIQUE_USERS(a int); +drop table UNIQUE_USERS; create table UNIQUE_USERS (a int); drop table UNIQUE_USERS; @@ -249,10 +251,11 @@ create table GROUP_CONCAT(a int); --error ER_PARSE_ERROR create table GROUP_CONCAT (a int); ---error ER_PARSE_ERROR +# Limitation removed in 5.1 create table GROUP_UNIQUE_USERS(a int); ---error ER_PARSE_ERROR +drop table GROUP_UNIQUE_USERS; create table GROUP_UNIQUE_USERS (a int); +drop table GROUP_UNIQUE_USERS; --error ER_PARSE_ERROR create table MAX(a int); @@ -339,10 +342,11 @@ create table TRIM(a int); --error ER_PARSE_ERROR create table TRIM (a int); ---error ER_PARSE_ERROR +# Limitation removed in 5.1 create table UNIQUE_USERS(a int); ---error ER_PARSE_ERROR +drop table UNIQUE_USERS; create table UNIQUE_USERS (a int); +drop table UNIQUE_USERS; --error ER_PARSE_ERROR create table VARIANCE(a int); diff --git a/sql/Makefile.am b/sql/Makefile.am index 91fe875c73d..7020ba469cc 100644 --- a/sql/Makefile.am +++ b/sql/Makefile.am @@ -43,7 +43,7 @@ mysqld_LDADD = @MYSQLD_EXTRA_LDFLAGS@ \ $(LDADD) $(CXXLDFLAGS) $(WRAPLIBS) @LIBDL@ \ @yassl_libs@ @openssl_libs@ noinst_HEADERS = item.h item_func.h item_sum.h item_cmpfunc.h \ - item_strfunc.h item_timefunc.h item_uniq.h \ + item_strfunc.h item_timefunc.h \ item_xmlfunc.h \ item_create.h item_subselect.h item_row.h \ mysql_priv.h item_geofunc.h sql_bitmap.h \ @@ -83,7 +83,7 @@ mysqld_SOURCES = sql_lex.cc sql_handler.cc sql_partition.cc \ sql_base.cc table.cc sql_select.cc sql_insert.cc \ sql_prepare.cc sql_error.cc sql_locale.cc \ sql_update.cc sql_delete.cc uniques.cc sql_do.cc \ - procedure.cc item_uniq.cc sql_test.cc \ + procedure.cc sql_test.cc \ log.cc log_event.cc init.cc derror.cc sql_acl.cc \ unireg.cc des_key_file.cc \ discover.cc time.cc opt_range.cc opt_sum.cc \ diff --git a/sql/item.h b/sql/item.h index bda57020cf2..3eacae1682c 100644 --- a/sql/item.h +++ b/sql/item.h @@ -2135,7 +2135,6 @@ public: #include "item_strfunc.h" #include "item_geofunc.h" #include "item_timefunc.h" -#include "item_uniq.h" #include "item_subselect.h" #include "item_xmlfunc.h" #endif diff --git a/sql/item_sum.h b/sql/item_sum.h index acd23d1ff4c..33bd23602b2 100644 --- a/sql/item_sum.h +++ b/sql/item_sum.h @@ -223,7 +223,7 @@ class Item_sum :public Item_result_field public: enum Sumfunctype { COUNT_FUNC, COUNT_DISTINCT_FUNC, SUM_FUNC, SUM_DISTINCT_FUNC, AVG_FUNC, - AVG_DISTINCT_FUNC, MIN_FUNC, MAX_FUNC, UNIQUE_USERS_FUNC, STD_FUNC, + AVG_DISTINCT_FUNC, MIN_FUNC, MAX_FUNC, STD_FUNC, VARIANCE_FUNC, SUM_BIT_FUNC, UDF_SUM_FUNC, GROUP_CONCAT_FUNC }; diff --git a/sql/item_uniq.cc b/sql/item_uniq.cc deleted file mode 100644 index 9db8228b345..00000000000 --- a/sql/item_uniq.cc +++ /dev/null @@ -1,32 +0,0 @@ -/* Copyright (C) 2000 MySQL AB & MySQL Finland AB & TCX DataKonsult AB - - This program is free software; you can redistribute it and/or modify - it under the terms of the GNU General Public License as published by - the Free Software Foundation; either version 2 of the License, or - (at your option) any later version. - - This program is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - GNU General Public License for more details. - - You should have received a copy of the GNU General Public License - along with this program; if not, write to the Free Software - Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ - -/* Compability file */ - -#ifdef USE_PRAGMA_IMPLEMENTATION -#pragma implementation // gcc: Class implementation -#endif - -#include "mysql_priv.h" - -Field *Item_sum_unique_users::create_tmp_field(bool group, TABLE *table, - uint convert_blob_length) -{ - Field *field= new Field_long(9, maybe_null, name, 1); - if (field) - field->init(table); - return field; -} diff --git a/sql/item_uniq.h b/sql/item_uniq.h deleted file mode 100644 index a0aa0b96cc6..00000000000 --- a/sql/item_uniq.h +++ /dev/null @@ -1,63 +0,0 @@ -/* Copyright (C) 2000 MySQL AB & MySQL Finland AB & TCX DataKonsult AB - - This program is free software; you can redistribute it and/or modify - it under the terms of the GNU General Public License as published by - the Free Software Foundation; either version 2 of the License, or - (at your option) any later version. - - This program is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - GNU General Public License for more details. - - You should have received a copy of the GNU General Public License - along with this program; if not, write to the Free Software - Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ - -/* Compability file ; This file only contains dummy functions */ - -#ifdef USE_PRAGMA_INTERFACE -#pragma interface -#endif - -#include - -class Item_func_unique_users :public Item_real_func -{ -public: - Item_func_unique_users(Item *name_arg,int start,int end,List &list) - :Item_real_func(list) {} - double val_real() { DBUG_ASSERT(fixed == 1); return 0.0; } - void fix_length_and_dec() { decimals=0; max_length=6; } - void print(String *str) { str->append(STRING_WITH_LEN("0.0")); } - const char *func_name() const { return "unique_users"; } -}; - - -class Item_sum_unique_users :public Item_sum_num -{ -public: - Item_sum_unique_users(Item *name_arg,int start,int end,Item *item_arg) - :Item_sum_num(item_arg) {} - Item_sum_unique_users(THD *thd, Item_sum_unique_users *item) - :Item_sum_num(thd, item) {} - double val_real() { DBUG_ASSERT(fixed == 1); return 0.0; } - enum Sumfunctype sum_func () const {return UNIQUE_USERS_FUNC;} - void clear() {} - bool add() { return 0; } - void reset_field() {} - void update_field() {} - bool fix_fields(THD *thd, Item **ref) - { - DBUG_ASSERT(fixed == 0); - fixed= 1; - return FALSE; - } - Item *copy_or_same(THD* thd) - { - return new Item_sum_unique_users(thd, this); - } - void print(String *str) { str->append(STRING_WITH_LEN("0.0")); } - Field *create_tmp_field(bool group, TABLE *table, uint convert_blob_length); - const char *func_name() const { return "sum_unique_users"; } -}; diff --git a/sql/lex.h b/sql/lex.h index 254d7f10fb7..aff8149efd0 100644 --- a/sql/lex.h +++ b/sql/lex.h @@ -593,7 +593,6 @@ static SYMBOL sql_functions[] = { { "DATE_SUB", SYM(DATE_SUB_INTERVAL)}, { "EXTRACT", SYM(EXTRACT_SYM)}, { "GROUP_CONCAT", SYM(GROUP_CONCAT_SYM)}, - { "GROUP_UNIQUE_USERS", SYM(GROUP_UNIQUE_USERS)}, { "MAX", SYM(MAX_SYM)}, { "MID", SYM(SUBSTRING)}, /* unireg function */ { "MIN", SYM(MIN_SYM)}, @@ -611,7 +610,6 @@ static SYMBOL sql_functions[] = { { "SYSDATE", SYM(SYSDATE)}, { "SYSTEM_USER", SYM(USER)}, { "TRIM", SYM(TRIM)}, - { "UNIQUE_USERS", SYM(UNIQUE_USERS)}, { "VARIANCE", SYM(VARIANCE_SYM)}, { "VAR_POP", SYM(VARIANCE_SYM)}, { "VAR_SAMP", SYM(VAR_SAMP_SYM)}, diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h index 812268fc92c..09c40147a08 100644 --- a/sql/mysql_priv.h +++ b/sql/mysql_priv.h @@ -36,6 +36,7 @@ #include #include #include /* Needed by field.h */ +#include #include "sql_bitmap.h" #include "sql_array.h" diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index bf5369d5553..12617d8679e 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -537,7 +537,6 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); %token GRANTS %token GROUP /* SQL-2003-R */ %token GROUP_CONCAT_SYM -%token GROUP_UNIQUE_USERS %token GT_SYM /* OPERATOR */ %token HANDLER_SYM %token HASH_SYM @@ -864,7 +863,6 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); %token UNINSTALL_SYM %token UNION_SYM /* SQL-2003-R */ %token UNIQUE_SYM -%token UNIQUE_USERS %token UNKNOWN_SYM /* SQL-2003-R */ %token UNLOCK_SYM %token UNSIGNED @@ -6279,10 +6277,6 @@ simple_expr: } $$= new (YYTHD->mem_root) Item_func_interval((Item_row *)$1); } - | UNIQUE_USERS '(' text_literal ',' NUM ',' NUM ',' expr_list ')' - { - $$= new Item_func_unique_users($3,atoi($5.str),atoi($7.str), * $9); - } ; /* @@ -6769,8 +6763,6 @@ sum_expr: { Select->in_sum_expr--; } ')' { $$=new Item_sum_count_distinct(* $5); } - | GROUP_UNIQUE_USERS '(' text_literal ',' NUM ',' NUM ',' in_sum_expr ')' - { $$= new Item_sum_unique_users($3,atoi($5.str),atoi($7.str),$9); } | MIN_SYM '(' in_sum_expr ')' { $$=new Item_sum_min($3); } /* From eda8c901f44ee77fd87efe95b514b499a649e500 Mon Sep 17 00:00:00 2001 From: "malff/marcsql@weblab.(none)" <> Date: Fri, 12 Jan 2007 10:33:44 -0700 Subject: [PATCH 23/23] Fixed windows build break --- sql/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/CMakeLists.txt b/sql/CMakeLists.txt index 5593231b34a..3cd2334d69e 100644 --- a/sql/CMakeLists.txt +++ b/sql/CMakeLists.txt @@ -48,7 +48,7 @@ ADD_EXECUTABLE(mysqld ../sql-common/client.c derror.cc des_key_file.cc hostname.cc init.cc item.cc item_buff.cc item_cmpfunc.cc item_create.cc item_func.cc item_geofunc.cc item_row.cc item_strfunc.cc item_subselect.cc item_sum.cc item_timefunc.cc - item_uniq.cc key.cc log.cc lock.cc log_event.cc message.rc + key.cc log.cc lock.cc log_event.cc message.rc message.h mf_iocache.cc my_decimal.cc ../sql-common/my_time.c mysqld.cc net_serv.cc nt_servc.cc nt_servc.h opt_range.cc opt_range.h opt_sum.cc