Merge weblab.(none):/home/marcsql/TREE/mysql-5.1-base

into  weblab.(none):/home/marcsql/TREE/mysql-5.1-merge


mysql-test/r/sp.result:
  Auto merged
server-tools/instance-manager/commands.cc:
  Auto merged
server-tools/instance-manager/commands.h:
  Auto merged
server-tools/instance-manager/guardian.cc:
  Auto merged
server-tools/instance-manager/guardian.h:
  Auto merged
server-tools/instance-manager/instance.cc:
  Auto merged
server-tools/instance-manager/instance.h:
  Auto merged
server-tools/instance-manager/instance_map.cc:
  Auto merged
server-tools/instance-manager/instance_map.h:
  Auto merged
server-tools/instance-manager/instance_options.h:
  Auto merged
server-tools/instance-manager/listener.cc:
  Auto merged
server-tools/instance-manager/manager.cc:
  Auto merged
server-tools/instance-manager/manager.h:
  Auto merged
server-tools/instance-manager/user_map.cc:
  Auto merged
sql/event_data_objects.cc:
  Auto merged
sql/event_queue.cc:
  Auto merged
sql/handler.cc:
  Auto merged
sql/lock.cc:
  Auto merged
sql/set_var.cc:
  Auto merged
sql/set_var.h:
  Auto merged
sql/sp_head.cc:
  Auto merged
sql/sp_head.h:
  Auto merged
sql/sql_class.cc:
  Auto merged
sql/sql_class.h:
  Auto merged
sql/sql_lex.cc:
  Auto merged
sql/sql_lex.h:
  Auto merged
sql/sql_prepare.cc:
  Auto merged
sql/sql_yacc.yy:
  Auto merged
storage/csv/ha_tina.cc:
  Auto merged
tests/mysql_client_test.c:
  Auto merged

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;

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` (

mysql-test/r/read_only.result

@@ -39,11 +39,61 @@ 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;
 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;
-set global read_only=0;

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;

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;

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;

mysql-test/r/sp_stress_case.result

@@ -0,0 +1,120 @@
+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_simple(2);
+str
+2
+CALL bug_19194_simple(1000);
+str
+1000
+CALL bug_19194_simple(4998);
+str
+4998
+CALL bug_19194_simple(4999);
+str
+4999
+CALL bug_19194_simple(9999);
+str
+unknown
+CALL bug_19194_searched(1);
+str
+1
+CALL bug_19194_searched(2);
+str
+2
+CALL bug_19194_searched(1000);
+str
+1000
+CALL bug_19194_searched(4998);
+str
+4998
+CALL bug_19194_searched(4999);
+str
+4999
+CALL bug_19194_searched(9999);
+str
+unknown
+DROP PROCEDURE proc_19194_codegen;
+DROP PROCEDURE bug_19194_simple;
+DROP PROCEDURE bug_19194_searched;

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;

mysql-test/t/log.sh

@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh
 
 ###########################################################################
 

mysql-test/t/read_only.test

@@ -102,6 +102,111 @@ drop table t1;
 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;
+
 # BUG #22077 "DROP TEMPORARY TABLE fails with wrong error if read_only is set"
 #
 # check if DROP TEMPORARY on a non-existing temporary table returns the right
@@ -113,8 +218,10 @@ drop temporary table ttt;
 # check if DROP TEMPORARY TABLE IF EXISTS produces a warning with read_only set
 drop temporary table if exists ttt;
 
+#
+# Cleanup
+#
 connection default;
+set global read_only=0;
 drop table t1,t2;
 drop user test@localhost;
-
-set global read_only=0;

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;
+

mysql-test/t/rpl_read_only-slave.opt

@@ -0,0 +1 @@
+--innodb

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;
+

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

mysql-test/t/sp_stress_case.test

@@ -0,0 +1,89 @@
+#
+# Bug#19194 (Right recursion in parser for CASE causes excessive stack
+#   usage, limitation)
+#
+
+--disable_warnings
+DROP PROCEDURE IF EXISTS proc_19194_codegen;
+DROP PROCEDURE IF EXISTS bug_19194_simple;
+DROP PROCEDURE IF EXISTS bug_19194_searched;
+--enable_warnings
+
+delimiter |;
+
+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|
+
+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;
+

server-tools/instance-manager/commands.cc

@@ -28,6 +28,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"
@@ -35,8 +36,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.
 */
@@ -45,38 +49,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()
+    parse_version_number()
-    buff              buffer to add the string
+    version_str
-    str               string to add
+    version
-    position          offset in the buff to add a string
+    version_size
 
   DESCRIPTION
+    TODO
 
-  Function to add a string to the buffer. It is different from
+  TODO: Move this function to Instance_options and parse version number
-  store_to_protocol_packet, which is used in the protocol.cc.
+  only once.
-  The last one also stores the length of the string in a special way.
-  This is required for MySQL client/server protocol support only.
 
-  RETURN
+  NOTE: This function is used only in SHOW INSTANCE STATUS statement at the
-    0 - ok
+  moment.
-    1 - error occured
 */
 
-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)
 {
@@ -101,6 +92,9 @@ static int parse_version_number(const char *version_str, char *version,
   return 0;
 }
 
+/**************************************************************************
+  }}}
+**************************************************************************/
 
 /**************************************************************************
  Implementation of Instance_name.
@@ -121,7 +115,7 @@ Instance_name::Instance_name(const LEX_STRING *name)
  Implementation of Show_instances.
 **************************************************************************/
 
-/*
+/**
   Implementation of SHOW INSTANCES statement.
 
   Possible error codes:
@@ -171,7 +165,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()))
@@ -179,20 +172,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;
 }
@@ -202,7 +200,7 @@ int Show_instances::write_data(st_net *net)
  Implementation of Flush_instances.
 **************************************************************************/
 
-/*
+/**
   Implementation of FLUSH INSTANCES statement.
 
   Possible error codes:
@@ -212,36 +210,19 @@ int Show_instances::write_data(st_net *net)
 
 int Flush_instances::execute(st_net *net, ulong connection_id)
 {
-  instance_map->guardian->lock();
+  if (Manager::flush_instances())
-  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();
     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_cmd::Instance_cmd(const LEX_STRING *instance_name_arg):
-  :instance_name(instance_name_arg)
+   instance_name(instance_name_arg)
 {
   /*
     MT-NOTE: we can not make a search for Instance object here,
@@ -250,26 +231,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());
+    instance_map->unlock();
-
+    return ER_BAD_INSTANCE_NAME;
-    if (!instance)
-    {
-      instance_map->unlock();
-      return ER_BAD_INSTANCE_NAME;
-    }
-
-    err_code= execute_impl(net, instance);
   }
 
+  instance->lock();
   instance_map->unlock();
 
+  err_code= execute_impl(net, instance);
+
+  instance->unlock();
+
   if (!err_code)
     err_code= send_ok_response(net, connection_id);
 
@@ -287,7 +281,7 @@ Show_instance_status::Show_instance_status(const LEX_STRING *instance_name_arg)
 }
 
 
-/*
+/**
   Implementation of SHOW INSTANCE STATUS statement.
 
   Possible error codes:
@@ -362,19 +356,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;
+  const char *mysqld_compatible_status=
-
+    instance->is_mysqld_compatible() ? "yes" : "no";
-  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();
 
   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;
@@ -408,7 +397,7 @@ Show_instance_options::Show_instance_options(
 }
 
 
-/*
+/**
   Implementation of SHOW INSTANCE OPTIONS statement.
 
   Possible error codes:
@@ -504,23 +493,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()))
+  if (instance->is_active())
-    return err_code;
+    return ER_INSTANCE_ALREADY_STARTED;
 
-  if (!(instance->options.nonguarded))
+  if (instance->start_mysqld())
-    instance_map->guardian->guard(instance);
+    return ER_CANNOT_START_INSTANCE;
+
+  instance->reset_stat();
+  instance->set_state(Instance::NOT_STARTED);
 
   return 0;
 }
@@ -545,25 +544,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->set_state(Instance::STOPPED);
-    instance_map->guardian->stop_guard(instance);
 
-  if ((err_code= instance->stop()))
+  return instance->stop_mysqld() ? ER_STOP_INSTANCE : 0;
-    return err_code;
-
-  return 0;
 }
 
 
@@ -581,12 +581,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
@@ -603,7 +603,7 @@ bool Create_instance::init(const char **text)
 }
 
 
-/*
+/**
   This operation parses CREATE INSTANCE options.
 
   SYNOPSIS
@@ -723,7 +723,7 @@ bool Create_instance::parse_args(const char **text)
 }
 
 
-/*
+/**
   Implementation of CREATE INSTANCE statement.
 
   Possible error codes:
@@ -735,6 +735,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. */
 
@@ -760,17 +761,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());
+    instance_map->remove_instance(instance); /* instance is deleted here. */
-
-    if (instance)
-      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();
@@ -789,12 +799,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:
@@ -803,14 +813,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);
@@ -823,27 +857,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)
+  /* Unlock the instance before destroy. */
-    return modify_defaults_to_im_error[err_code];
 
-  /* Remove instance from the instance map hash and Guardian's list. */
+  instance->unlock();
 
-  if (!instance->options.nonguarded)
+  /*
-    instance_map->guardian->stop_guard(instance);
+    Remove instance from the instance map
-
+    (the instance will be also destroyed here).
-  if ((err_code= instance->stop()))
+  */
-    return err_code;
 
   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;
 
@@ -866,7 +903,7 @@ Show_instance_log::Show_instance_log(const LEX_STRING *instance_name_arg,
 }
 
 
-/*
+/**
   Implementation of SHOW INSTANCE LOG statement.
 
   Possible error codes:
@@ -1011,7 +1048,7 @@ Show_instance_log_files::Show_instance_log_files
 }
 
 
-/*
+/**
   Implementation of SHOW INSTANCE LOG FILES statement.
 
   Possible error codes:
@@ -1132,7 +1169,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.
 */
@@ -1250,7 +1287,7 @@ bool Abstract_option_cmd::init(const char **text)
 }
 
 
-/*
+/**
   Correct the option file. The "skip" option is used to remove the found
   option.
 
@@ -1289,8 +1326,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
@@ -1340,6 +1377,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;
@@ -1351,12 +1393,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;
   }
 
@@ -1367,6 +1415,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);
@@ -1376,6 +1426,8 @@ int Abstract_option_cmd::execute_impl(st_net *net, ulong connection_id)
         break;
     }
 
+    lst->instance->unlock();
+
     if (err_code)
       break;
   }
@@ -1391,7 +1443,7 @@ int Abstract_option_cmd::execute_impl(st_net *net, ulong connection_id)
  Implementation of Set_option.
 **************************************************************************/
 
-/*
+/**
   This operation parses SET options.
 
   SYNOPSIS
@@ -1565,7 +1617,7 @@ int Set_option::process_option(Instance *instance, Named_value *option)
  Implementation of Unset_option.
 **************************************************************************/
 
-/*
+/**
   This operation parses UNSET options.
 
   SYNOPSIS
@@ -1661,7 +1713,7 @@ bool Unset_option::parse_args(const char **text)
 }
 
 
-/*
+/**
   Implementation of UNSET statement.
 
   Possible error codes:

server-tools/instance-manager/commands.h

@@ -29,7 +29,7 @@
 #endif
 
 
-/*
+/**
   Print all instances of this instance manager.
   Grammar: SHOW INSTANCES
 */
@@ -49,7 +49,7 @@ private:
 };
 
 
-/*
+/**
   Reread configuration file and refresh internal cache.
   Grammar: FLUSH INSTANCES
 */
@@ -65,29 +65,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);
+  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;
 
 protected:
   inline const LEX_STRING *get_instance_name() const
@@ -100,7 +91,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 <instance_name>
 */
@@ -120,7 +154,7 @@ private:
 };
 
 
-/*
+/**
   Print options of chosen instance.
   Grammar: SHOW INSTANCE OPTIONS <instance_name>
 */
@@ -140,7 +174,7 @@ private:
 };
 
 
-/*
+/**
   Start an instance.
   Grammar: START INSTANCE <instance_name>
 */
@@ -156,7 +190,7 @@ protected:
 };
 
 
-/*
+/**
   Stop an instance.
   Grammar: STOP INSTANCE <instance_name>
 */
@@ -172,12 +206,12 @@ protected:
 };
 
 
-/*
+/**
   Create an instance.
   Grammar: CREATE INSTANCE <instance_name> [<options>]
 */
 
-class Create_instance: public Command
+class Create_instance: public Instance_cmd
 {
 public:
   Create_instance(const LEX_STRING *instance_name_arg);
@@ -188,22 +222,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 <instance_name>
 
@@ -212,18 +239,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 execute(st_net *net, ulong connection_id);
-  virtual int send_ok_response(st_net *net, ulong connection_id);
 };
 
 
-/*
+/**
   Print requested part of the log.
   Grammar:
     SHOW <instance_name> LOG {ERROR | SLOW | GENERAL} size[, offset_from_end]
@@ -251,7 +277,7 @@ private:
 };
 
 
-/*
+/**
   Shows the list of the log files, used by an instance.
   Grammar: SHOW <instance_name> LOG FILES
 */
@@ -271,7 +297,7 @@ private:
 };
 
 
-/*
+/**
   Abstract class for option-management commands.
 */
 
@@ -311,7 +337,7 @@ private:
 };
 
 
-/*
+/**
   Set an option for the instance.
   Grammar: SET instance_name.option[=option_value][, ...]
 */
@@ -328,7 +354,7 @@ protected:
 };
 
 
-/*
+/**
   Remove option of the instance.
   Grammar: UNSET instance_name.option[, ...]
 */
@@ -345,7 +371,7 @@ protected:
 };
 
 
-/*
+/**
   Syntax error command.
 
   This command is issued if parser reported a syntax error. We need it to

server-tools/instance-manager/guardian.cc

@@ -27,102 +27,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:
+  SYNOPSIS
-    return "failed";
+    Guardian()
+    thread_registry_arg
+    instance_map_arg
 
-  case CRASHED:
+  DESCRIPTION
-    return "crashed";
+    Nominal contructor intended for assigning references and initialize
-
+    trivial objects. Real initialization is made by init() method.
-  case CRASHED_AND_ABANDONED:
+*/
-    return "abandoned";
-
-  case STOPPING:
-    return "stopping";
-  }
-
-  return NULL; /* just to ignore compiler warning. */
-}
-
-/* {{{ Constructor & destructor. */
 
 Guardian::Guardian(Thread_registry *thread_registry_arg,
-                   Instance_map *instance_map_arg,
+                   Instance_map *instance_map_arg)
-                   uint monitoring_interval_arg)
+  :shutdown_requested(FALSE),
-  :stopped(FALSE),
+  stopped(FALSE),
-  monitoring_interval(monitoring_interval_arg),
   thread_registry(thread_registry_arg),
-  instance_map(instance_map_arg),
+  instance_map(instance_map_arg)
-  guarded_instances(0),
-  shutdown_requested(FALSE)
 {
   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);
+    NOTE: it's necessary to synchronize here, because Guiardian thread can be
-  free_root(&alloc, MYF(0));
+    still alive an hold the mutex (because it is detached and we have no
-  pthread_mutex_unlock(&LOCK_guardian);
+    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,
+  Process an instance.
-                                LIST **guarded_instances,
+
-                                LIST *node)
+  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
+      log_info("Guardian: pretend that '%s' is killed.",
-        the node from the list. The pointer to the next one is still valid.
+               (const char *) instance->get_name()->str);
-      */
+
-      *guarded_instances= list_delete(*guarded_instances, node);
+      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 +157,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 &&
+    if (instance->get_state() == Instance::STARTING &&
-        current_node->instance->options.load_pid() == 0)
+        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;
+      instance->reset_stat();
-      current_node->crash_moment= 0;
+      instance->set_state(Instance::STARTED);
-      current_node->state= STARTED;
     }
   }
   else
   {
-    switch (current_node->state) {
+    switch (instance->get_state()) {
-    case NOT_STARTED:
+    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 */
+      /* NOTE: set state to STARTING _before_ start() is called. */
-      current_node->state= STARTING;
+      instance->set_state(Instance::STARTING);
-      instance->start();
+      instance->last_checked= current_time;
-      current_node->last_checked= current_time;
+
-      break;
+      instance->start_mysqld();
-    case STARTED:     /* fallthrough */
+
-    case STARTING:    /* let the instance start or crash */
+      return;
-      if (instance->is_crashed())
+
-      {
+    case Instance::STARTED:     /* fallthrough */
-        current_node->crash_moment= current_time;
+    case Instance::STARTING:    /* let the instance start or crash */
-        current_node->last_checked= current_time;
+      if (!instance->is_crashed())
-        current_node->state= JUST_CRASHED;
+        return;
-        /* fallthrough -- restart an instance immediately */
+
-      }
+      instance->crash_moment= current_time;
-      else
+      instance->last_checked= current_time;
-        break;
+      instance->set_state(Instance::JUST_CRASHED);
-    case JUST_CRASHED:
+      /* fallthrough -- restart an instance immediately */
-      if (current_time - current_node->crash_moment <= 2)
+
+    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;
+        instance->set_state(Instance::CRASHED);
-      break;
+
-    case CRASHED:    /* just regular restarts */
+      return;
-      if (current_time - current_node->last_checked >
+
-          monitoring_interval)
+    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_mysqld();
-          {
+          instance->last_checked= current_time;
-            instance->start();
+
-            current_node->last_checked= current_time;
+          log_info("Guardian: restarting '%s'...",
-            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",
                    (const char *) instance->options.instance_name.str);
-          current_node->state= CRASHED_AND_ABANDONED;
         }
       }
-      break;
+      else
-    case CRASHED_AND_ABANDONED:
+      {
-      break; /* do nothing */
+        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 +248,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
+    Check for all guarded instances and restart them if needed.
-    is fine go and sleep for some time.
 */
 
 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 (true)
-  while (!(shutdown_requested && (guarded_instances == NULL)))
   {
-    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->lock();
-      instance= ((GUARD_NODE *) node->data)->instance;
-      process_instance(instance, current_node, &guarded_instances, node);
 
-      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 */
+    thread_registry->cond_timedwait(&thread_info, &COND_guardian,
-    if (!(shutdown_requested && (!(guarded_instances))))
+                                    &LOCK_guardian, &timeout);
-      thread_registry->cond_timedwait(&thread_info, &COND_guardian,
+    unlock();
-                                      &LOCK_guardian, &timeout);
   }
 
   log_info("Guardian: stopped.");
 
-  stopped= TRUE;
+  /* Now, when the Guardian is stopped we can stop the IM. */
-  pthread_mutex_unlock(&LOCK_guardian);
+
-  /* now, when the Guardian is stopped we can stop the IM */
   thread_registry->unregister_thread(&thread_info);
   thread_registry->request_shutdown();
 
@@ -274,129 +327,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
+  Wake up Guardian thread.
-  add all of the instances, which don't have 'nonguarded' option specified.
 
-  SYNOPSIS
+  MT-NOTE: though usually the mutex associated with condition variable should
-    Guardian::init()
+  be acquired before signalling the variable, here this is not needed.
-
+  Signalling under locked mutex is used to avoid lost signals. In the current
-  NOTE: The operation should be invoked with the following locks acquired:
+  logic however locking mutex does not guarantee that the signal will not be
-    - Guardian;
+  lost.
-    - Instance_map;
-
-  RETURN
-    0 - ok
-    1 - error occurred
 */
 
-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)
+    instance->lock();
-      continue;
 
-    if (guard(instance, TRUE))                /* do not lock guardian */
+    instance->reset_stat();
-      return 1;
+    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 +398,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;
+  Instance_map::Iterator instances_it(instance_map);
-  node= guarded_instances;
+  Instance *instance;
-  while (node != NULL)
+
+  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;
+      instance->set_state(Instance::STOPPING);
-      current_node->last_checked= time(NULL);
+      instance->last_checked= time(NULL);
     }
     else
-      /* otherwise remove it from the list */
+    {
-      guarded_instances= list_delete(guarded_instances, node);
+      /* Otherwise mark it as STOPPED. */
-    /* But try to kill it anyway. Just in case */
+      instance->set_state(Instance::STOPPED);
-    current_node->instance->kill_mysqld(SIGTERM);
+    }
-    node= node->next;
+
+    /* 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();
-}

server-tools/instance-manager/guardian.h

@@ -16,10 +16,12 @@
    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA */
 
 
-#include "thread_registry.h"
+#include <my_global.h>
 #include <my_sys.h>
 #include <my_list.h>
 
+#include "thread_registry.h"
+
 #if defined(__GNUC__) && defined(USE_PRAGMA_INTERFACE)
 #pragma interface
 #endif
@@ -27,7 +29,6 @@
 class Instance;
 class Instance_map;
 class Thread_registry;
-struct GUARD_NODE;
 
 /**
   The guardian thread is responsible for monitoring and restarting of guarded
@@ -37,97 +38,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,
+           Instance_map *instance_map_arg);
-           uint monitoring_interval_arg);
+  ~Guardian();
-  virtual ~Guardian();
+
-  /* Initialize or refresh the list of guarded instances */
+  void init();
-  int init();
+
-  /* Request guardian shutdown. Stop instances if needed */
+public:
   void request_shutdown();
-  /* Start instance protection */
+
-  int guard(Instance *instance, bool nolock= FALSE);
+  bool is_stopped();
-  /* Stop instance protection */
+
-  int stop_guard(Instance *instance);
-  /* Returns TRUE if guardian thread is stopped */
-  int is_stopped();
   void lock();
   void unlock();
 
-  /*
+  void ping();
-    Return an internal list node for the given instance if the instance is
-    managed by Guardian. Otherwise, return NULL.
 
-    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 */
+    This variable is set to TRUE, when Manager thread is shutting down.
-  int stop_instances();
+    The flag is used by Guardian thread to understand that it's time to
-  /* check instance state and act accordingly */
+    finish.
-  void process_instance(Instance *instance, GUARD_NODE *current_node,
+  */
-                        LIST **guarded_instances, LIST *elem);
+  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;
+private:
-  /* this variable is set to TRUE when we want to stop Guardian thread */
+  Guardian(const Guardian &);
-  bool shutdown_requested;
+  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 */

server-tools/instance-manager/instance.cc

@@ -35,7 +35,9 @@
 #include "thread_registry.h"
 #include "instance_map.h"
 
-/* {{{ Platform-specific functions. */
+/*************************************************************************
+  {{{ Platform-specific functions.
+*************************************************************************/
 
 #ifndef __WIN__
 typedef pid_t My_process_info;
@@ -43,34 +45,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
 
@@ -284,113 +258,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()
+  Proxy thread is a simple way to avoid all pitfalls of the threads
-    old_instance_options   Pointer to the options of the instance to be
+  implementation in the OS (e.g. LinuxThreads). With such a thread we
-                           launched. This info is likely to become obsolete
+  don't have to process SIGCHLD, which is a tricky business if we want
-                           when function returns from wait_process()
+  to do it in a portable way.
-    instance_map           Pointer to the instance_map. We use it to protect
-                           the instance from deletion, while we are working
-                           with it.
 
-  DESCRIPTION
+  Instance Monitor Thread forks a child process, execs mysqld and waits for
-    Fork a child, then exec and monitor it. When the child is dead,
+  the child to die.
-    find appropriate instance (for this purpose we save its name),
-    set appropriate flags and wake all threads waiting for instance
-    to stop.
 
-  NOTE
+  Instance Monitor assumes that the monitoring instance will not be dropped.
-    A separate thread for starting/monitoring instance is a simple way
+  This is guaranteed by having flag monitoring_thread_active and
-    to avoid all pitfalls of the threads implementation in the OS (e.g.
+  Instance::is_active() operation.
-    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
 */
 
-void
+class Instance_monitor: public Thread
-Instance_monitor::
-start_and_monitor_instance(Instance_options *old_instance_options,
-                           Instance_map *instance_map,
-                           Thread_registry *thread_registry)
 {
-  Instance_name instance_name(&old_instance_options->instance_name);
+public:
-  Instance *current_instance;
+  Instance_monitor(Instance *instance_arg) :instance(instance_arg) {}
-  My_process_info process_info;
+protected:
-  Thread_info thread_info;
+  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())
   {
-    /*
+    thread_registry->register_thread(&monitor_thread_info, FALSE);
-      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);
   }
 
-  /*
+  /* Starting mysqld. */
-    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")
-  */
 
   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();
+    instance->lock();
-    return;                                     /* error is logged */
+    instance->monitoring_thread_active= FALSE;
+    instance->unlock();
+
+    return;
   }
 
-  /* allow users to delete instances */
+  /* Waiting for mysqld to die. */
-  instance_map->unlock();
 
   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)
+  instance->lock();
-    current_instance->set_crash_flag_n_wake_all();
 
-  instance_map->unlock();
+  if (instance->is_guarded())
+    thread_registry->unregister_thread(&monitor_thread_info);
 
-  if (!old_instance_options->nonguarded)
+  instance->crashed= TRUE;
-    thread_registry->unregister_thread(&thread_info);
+  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)
 {
@@ -404,21 +414,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),
+  :monitoring_thread_active(FALSE),
-  configured(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);
 }
 
 
@@ -426,13 +498,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
@@ -452,7 +522,7 @@ bool Instance::init(const LEX_STRING *name_arg)
 }
 
 
-/*
+/**
   Complete instance options initialization.
 
   SYNOPSIS
@@ -473,7 +543,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
@@ -483,7 +593,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
@@ -507,8 +617,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,
@@ -522,7 +630,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
@@ -530,145 +637,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()
+    start_mysqld()
-
-  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()
 
   DESCRIPTION
-    The method is called when an instance is crashed or terminated.
+    Reset flags and start Instance Monitor thread, which will start mysqld.
-    In the former case it might indicate that guardian probably should
+
-    restart it.
+    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 */
+  Instance_monitor *instance_monitor;
-  pthread_mutex_lock(&LOCK_instance);
-  crashed= TRUE;
-  pthread_mutex_unlock(&LOCK_instance);
 
   /*
-    Wake connection threads waiting for an instance to stop. This
+    Prepare instance to start Instance Monitor thread.
-    is needed if a user issued command to stop an instance via
+
-    mysql connection. This is not the case if Guardian stop the 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;
 
-/*
+  remove_pid();
-  Stop an instance.
 
-  SYNOPSIS
+  /* Create and start the Instance Monitor thread. */
-    stop()
 
-  RETURN:
+  instance_monitor= new Instance_monitor(this);
-    0                            ok
-    ER_INSTANCE_IS_NOT_STARTED   Looks like the instance it is not started
-    ER_STOP_INSTANCE             mysql_shutdown reported an error
-*/
 
-int Instance::stop()
+  if (instance_monitor == NULL || instance_monitor->start(Thread::DETACHED))
-{
-  struct timespec timeout;
-  uint waitchild= (uint)  DEFAULT_SHUTDOWN_DELAY;
-
-  if (is_mysqld_running())
   {
-    waitchild= options.get_shutdown_delay();
+    delete instance_monitor;
+    monitoring_thread_active= FALSE;
 
-    kill_mysqld(SIGTERM);
+    log_error("Instance '%s': can not create instance monitor thread.",
-    /* sleep on condition to wait for SIGCHLD */
+              (const char *) get_name()->str);
 
-    timeout.tv_sec= time(NULL) + waitchild;
+    return TRUE;
-    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 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)
@@ -706,27 +813,91 @@ void Instance::kill_mysqld(int signum)
   }
 }
 
-/*
-  Return crashed flag.
 
-  SYNOPSIS
+/**
-    is_crashed()
+  Lock instance.
-
-  RETURN
-    TRUE  - mysqld crashed
-    FALSE - mysqld hasn't crashed yet
 */
 
-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.
 */
 
@@ -743,3 +914,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);
+  }
+}
+
+/**************************************************************************
+  }}}
+**************************************************************************/

server-tools/instance-manager/instance.h

@@ -29,7 +29,7 @@ class Instance_map;
 class Thread_registry;
 
 
-/*
+/**
   Instance_name -- the class represents instance name -- a string of length
   less than MAX_INSTANCE_NAME_SIZE.
 
@@ -67,72 +67,127 @@ private:
 class Instance
 {
 public:
-  /*
+  /* States of an instance. */
-    The following two constants defines name of the default mysqld-instance
+  enum enum_instance_state
-    ("mysqld").
+  {
+    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();
+public:
-  int start();
+  bool is_active();
-  int stop();
-  /* send a signal to the instance */
-  void kill_mysqld(int signo);
-  bool is_crashed();
-  void set_crash_flag_n_wake_all();
 
-  /*
+  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 specifies whether the instance is mysqld-compatible
-    Mysqld-compatible instances can contain only mysqld-specific options.
+    or not. Mysqld-compatible instances can contain only mysqld-specific
-    At the moment an instance is mysqld-compatible if its name is "mysqld".
+    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
@@ -141,18 +196,36 @@ private:
   bool mysqld_compatible;
 
   /*
-    Mutex protecting the instance. Currently we use it to avoid the
+    Mutex protecting the instance.
-    double start of the instance. This happens when the instance is starting
-    and we issue the start command once more.
   */
   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;
 };
 
 
@@ -168,9 +241,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 */

server-tools/instance-manager/instance_map.cc

@@ -24,26 +24,18 @@
 #include <mysql_com.h>
 
 #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)
@@ -53,14 +45,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()
@@ -95,8 +91,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()
@@ -136,7 +132,7 @@ static void parse_option(const char *option_str,
 }
 
 
-/*
+/**
   Process one option from the configuration file.
 
   SYNOPSIS
@@ -150,6 +146,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)
@@ -212,92 +212,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()
+  Check if there is an active instance or not.
-
-  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;
 */
 
-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;
@@ -305,29 +310,50 @@ bool Instance_map::is_there_active_instance()
 
   while ((instance= iterator.next()))
   {
-    if (guardian->find_instance_node(instance) != NULL ||
+    bool active_instance_found;
-        instance->is_mysqld_running())
+
-    {
+    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)
 {
@@ -391,12 +417,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;
@@ -454,7 +490,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()
 {
@@ -504,8 +543,9 @@ int Instance_map::load()
 }
 
 
-/*--- Implementaton of the Instance map iterator class  ---*/
+/*************************************************************************
-
+  {{{ Instance_map::Iterator implementation.
+*************************************************************************/
 
 void Instance_map::Iterator::go_to_first()
 {
@@ -521,29 +561,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

server-tools/instance-manager/instance_map.h

@@ -36,14 +36,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:
@@ -57,79 +60,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 */

server-tools/instance-manager/instance_options.h

@@ -45,7 +45,6 @@ public:
   Instance_options();
   ~Instance_options();
 
-  /* fills in argv */
   bool complete_initialization();
 
   bool set_option(Named_value *option);

server-tools/instance-manager/listener.cc

@@ -153,7 +153,7 @@ void Listener::run()
           else
           {
             shutdown(client_fd, SHUT_RDWR);
-            close(client_fd);
+            closesocket(client_fd);
           }
         }
       }
@@ -165,7 +165,7 @@ void Listener::run()
   log_info("Listener: shutdown requested, exiting...");
 
   for (i= 0; i < num_sockets; i++)
-    close(sockets[i]);
+    closesocket(sockets[i]);
 
 #ifndef __WIN__
   unlink(unix_socket_address.sun_path);
@@ -179,7 +179,7 @@ void Listener::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();
@@ -227,7 +227,7 @@ int Listener::create_tcp_socket()
   {
     log_error("Listener: bind(ip socket) failed: %s.",
               (const char *) strerror(errno));
-    close(ip_socket);
+    closesocket(ip_socket);
     return -1;
   }
 
@@ -235,7 +235,7 @@ int Listener::create_tcp_socket()
   {
     log_error("Listener: listen(ip socket) failed: %s.",
               (const char *) strerror(errno));
-    close(ip_socket);
+    closesocket(ip_socket);
     return -1;
   }
 

server-tools/instance-manager/manager.cc

@@ -36,6 +36,9 @@
 #include "user_map.h"
 
 
+/**********************************************************************
+ {{{ Platform-specific implementation.
+**********************************************************************/
 
 #ifndef __WIN__
 void set_signals(sigset_t *mask)
@@ -91,9 +94,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 */
@@ -136,6 +143,10 @@ bool check_if_linux_threads(bool *linux_threads)
 
 }
 
+/**********************************************************************
+  }}}
+***********************************************************************/
+
 
 /**********************************************************************
   Manager implementation
@@ -151,25 +162,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
+  Main manager function.
-  listener thread, write pid file and enter into signal handling.
+
-  See also comments in mysqlmanager.cc to picture general Instance Manager
+  SYNOPSIS
-  architecture.
+    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.
 */
@@ -193,22 +216,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
+    All objects created in the Manager object live as long as thread_registry
-    thread_registry lives, and thread_registry is alive until there are
+    lives, and thread_registry is alive until there are working threads.
-    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,
+  Guardian guardian(&thread_registry, &instance_map);
-                    Options::Main::monitoring_interval);
 
   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;
 
@@ -248,7 +282,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,
@@ -289,6 +323,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.");
@@ -297,21 +332,11 @@ int Manager::main()
 
   /* Load instances. */
 
+  if (Manager::flush_instances())
   {
-    instance_map.guardian->lock();
+    log_error("Can not init instances repository.");
-    instance_map.lock();
+    stop_all_threads();
-
+    goto err;
-    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;
-    }
   }
 
   /* Initialize the Listener. */
@@ -327,7 +352,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. */
 
@@ -380,7 +406,6 @@ int Manager::main()
       if (!guardian.is_stopped())
       {
         guardian.request_shutdown();
-        pthread_cond_signal(&guardian.COND_guardian);
       }
       else
       {
@@ -405,3 +430,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;
+}

server-tools/instance-manager/manager.h

@@ -19,6 +19,7 @@
 #if defined(__GNUC__) && defined(USE_PRAGMA_INTERFACE)
 #pragma interface
 #endif
+
 #include <my_global.h>
 
 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__

server-tools/instance-manager/user_map.cc

@@ -41,7 +41,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;
@@ -53,7 +53,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;
@@ -63,10 +63,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'.",
+              "User line: '%s'.",
-             (int) user_length,
+              (int) user_length,
-             (int) USERNAME_LENGTH,
+              (int) USERNAME_LENGTH,
-             (const char *) line);
+              (const char *) line);
     return 1;
   }
 
@@ -74,10 +74,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'.",
+              "User line: '%s'.",
-             (int) password_length,
+              (int) password_length,
-             (int) SCRAMBLED_PASSWORD_CHAR_LENGTH,
+              (int) SCRAMBLED_PASSWORD_CHAR_LENGTH,
-             line);
+              (const char *) line);
     return 1;
   }
 

sql/event_data_objects.cc

@@ -394,7 +394,14 @@ Event_parse_data::init_starts(THD *thd)
   if ((not_used= item_starts->get_date(&ltime, 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());
 
@@ -406,12 +413,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.
+    Again, after 2038 this code won't work. As
-    CONVERT_TZ has similar problem.
+    mysql_priv.h currently lists
-    mysql_priv.h currently lists 
       #define TIMESTAMP_MAX_YEAR 2038 (see TIME_to_timestamp())
   */
-  my_tz_UTC->gmt_sec_to_TIME(&ltime,t=TIME_to_timestamp(thd, &ltime, &not_used));
+  my_tz_UTC->gmt_sec_to_TIME(&ltime,t=TIME_to_timestamp(thd, &ltime,
+                                                        &not_used));
   if (!t)
     goto wrong_value;
 
@@ -464,13 +471,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.
+    Again, after 2038 this code won't work. As
-    CONVERT_TZ has similar problem.
+    mysql_priv.h currently lists
-    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(&ltime,t=TIME_to_timestamp(thd, &ltime, &not_used));
+  my_tz_UTC->gmt_sec_to_TIME(&ltime,t=TIME_to_timestamp(thd, &ltime,
+                                                        &not_used));
   if (!t)
     goto error_bad_params;
 

sql/event_queue.cc

@@ -159,7 +159,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;
   }
 

sql/handler.cc

@@ -691,6 +691,19 @@ int ha_commit_trans(THD *thd, bool all)
       ha_rollback_trans(thd, all);
       DBUG_RETURN(1);
     }
+
+    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);
+      error= 1;
+      goto end;
+    }
+
     DBUG_EXECUTE_IF("crash_commit_before", abort(););
 
     /* Close all cursors that can not survive COMMIT */

sql/lock.cc

@@ -150,6 +150,23 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count,
       }
     }
 
+    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.
+        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))

sql/set_var.cc

@@ -369,7 +369,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",
@@ -3857,6 +3857,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)
 {

sql/set_var.h

@@ -904,6 +904,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:

sql/sp_head.cc

@@ -628,27 +628,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
@@ -2229,7 +2208,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
@@ -2242,7 +2221,7 @@ void sp_head::optimize()
   sp_instr *i;
   uint src, dst;
 
-  opt_mark(0);
+  opt_mark();
 
   bp.empty();
   src= dst= 0;
@@ -2276,13 +2255,50 @@ void sp_head::optimize()
   bp.empty();
 }
 
-void
+void sp_head::add_mark_lead(uint ip, List<sp_instr> *leads)
-sp_head::opt_mark(uint ip)
 {
-  sp_instr *i;
+  sp_instr *i= get_instr(ip);
 
-  while ((i= get_instr(ip)) && !i->marked)
+  if (i && ! i->marked)
-    ip= i->opt_mark(this);
+    leads->push_front(i);
+}
+
+void
+sp_head::opt_mark()
+{
+  uint ip;
+  sp_instr *i;
+  List<sp_instr> 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);
+    }
+  }
 }
 
 
@@ -2684,7 +2700,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<sp_instr> *leads)
 {
   m_dest= opt_shortcut_jump(sp, this);
   if (m_dest != m_ip+1)		/* Jumping to following instruction? */
@@ -2778,7 +2794,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<sp_instr> *leads)
 {
   sp_instr *i;
 
@@ -2788,13 +2804,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;
 }
 
@@ -2915,7 +2931,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<sp_instr> *leads)
 {
   sp_instr *i;
 
@@ -2925,7 +2941,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;
 }
 
@@ -2990,15 +3006,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<sp_instr> *leads)
 {
   if (m_dest)
-    return sp_instr_jump::opt_mark(sp);
+    return sp_instr_jump::opt_mark(sp, leads);
-  else
+
-  {
+  marked= 1;
-    marked= 1;
+  return UINT_MAX;
-    return UINT_MAX;
-  }
 }
 
 
@@ -3341,7 +3355,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<sp_instr> *leads)
 {
   sp_instr *i;
 
@@ -3351,7 +3365,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;
 }
 

sql/sp_head.h

@@ -303,8 +303,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 <code>opt_mark()</code>.
+    @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<sp_instr> *leads);
 
   void recursion_level_error(THD *thd);
 
@@ -413,6 +424,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.
@@ -480,10 +497,10 @@ public:
 
   /*
     Mark this instruction as reachable during optimization and return the
-    index to the next instruction. Jump instruction will mark their
+    index to the next instruction. Jump instruction will add their
-    destination too recursively.
+    destination to the leads list.
   */
-  virtual uint opt_mark(sp_head *sp)
+  virtual uint opt_mark(sp_head *sp, List<sp_instr> *leads)
   {
     marked= 1;
     return m_ip+1;
@@ -755,7 +772,7 @@ public:
 
   virtual void print(String *str);
 
-  virtual uint opt_mark(sp_head *sp);
+  virtual uint opt_mark(sp_head *sp, List<sp_instr> *leads);
 
   virtual uint opt_shortcut_jump(sp_head *sp, sp_instr *start);
 
@@ -805,7 +822,7 @@ public:
 
   virtual void print(String *str);
 
-  virtual uint opt_mark(sp_head *sp);
+  virtual uint opt_mark(sp_head *sp, List<sp_instr> *leads);
 
   /* Override sp_instr_jump's shortcut; we stop here */
   virtual uint opt_shortcut_jump(sp_head *sp, sp_instr *start)
@@ -851,7 +868,7 @@ public:
 
   virtual void print(String *str);
 
-  virtual uint opt_mark(sp_head *sp)
+  virtual uint opt_mark(sp_head *sp, List<sp_instr> *leads)
   {
     marked= 1;
     return UINT_MAX;
@@ -888,7 +905,7 @@ public:
 
   virtual void print(String *str);
 
-  virtual uint opt_mark(sp_head *sp);
+  virtual uint opt_mark(sp_head *sp, List<sp_instr> *leads);
 
   /* Override sp_instr_jump's shortcut; we stop here. */
   virtual uint opt_shortcut_jump(sp_head *sp, sp_instr *start)
@@ -953,7 +970,7 @@ public:
 
   virtual void print(String *str);
 
-  virtual uint opt_mark(sp_head *sp);
+  virtual uint opt_mark(sp_head *sp, List<sp_instr> *leads);
 
 private:
 
@@ -1123,7 +1140,7 @@ public:
 
   virtual void print(String *str);
 
-  virtual uint opt_mark(sp_head *sp)
+  virtual uint opt_mark(sp_head *sp, List<sp_instr> *leads)
   {
     marked= 1;
     return UINT_MAX;
@@ -1156,7 +1173,7 @@ public:
 
   virtual void print(String *str);
 
-  virtual uint opt_mark(sp_head *sp);
+  virtual uint opt_mark(sp_head *sp, List<sp_instr> *leads);
 
   virtual void opt_move(uint dst, List<sp_instr> *ibp);
 

sql/sql_class.cc

@@ -984,6 +984,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);
@@ -1624,6 +1631,13 @@ int select_dumpvar::prepare(List<Item> &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()
 {
   row_count= 0;

sql/sql_class.h

@@ -1702,7 +1702,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
@@ -1740,7 +1747,7 @@ public:
   bool send_fields(List<Item> &list, uint flags);
   bool send_data(List<Item> &items);
   bool send_eof();
-  bool simple_select() { return 1; }
+  virtual bool check_simple_select() const { return FALSE; }
   void abort();
 };
 
@@ -2186,6 +2193,7 @@ public:
   int prepare(List<Item> &list, SELECT_LEX_UNIT *u);
   bool send_data(List<Item> &items);
   bool send_eof();
+  virtual bool check_simple_select() const;
   void cleanup();
 };
 

sql/sql_lex.cc

@@ -1193,7 +1193,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();

sql/sql_lex.h

@@ -552,7 +552,6 @@ public:
 
   SQL_LIST order_list;                /* ORDER clause */
   List<List_item>     expr_list;
-  List<List_item>     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

sql/sql_prepare.cc

@@ -2945,10 +2945,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;
   }
 

sql/sql_yacc.yy

@@ -107,6 +107,187 @@ static bool is_native_function(THD *thd, const LEX_STRING *name)
   return false;
 }
 
+
+/**
+  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:
+<pre>
+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
+</pre>
+  The actions are used to generate the following code:
+<pre>
+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"
+</pre>
+
+  @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"
+    (Instruction 12 in the example)
+  */
+
+  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"
+    (jump_if_not from instruction 2 to 5, 5 to 8 ... in the example)
+  */
+
+  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"
+    (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_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());
+}
+
+/**
+  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"
+    (jump from instruction 4 to 12, 7 to 12 ... in the example)
+  */
+  lex->sphead->backpatch(lex->spcont->pop_label());
+
+  if (simple)
+    lex->spcont->pop_case_expr_id();
+
+  lex->sphead->do_cont_backpatch();
+}
+
 %}
 %union {
   int  num;
@@ -886,7 +1067,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
@@ -922,10 +1103,11 @@ END_OF_INPUT
 
 %type <NONE> call sp_proc_stmts sp_proc_stmts1 sp_proc_stmt
 %type <NONE> sp_proc_stmt_statement sp_proc_stmt_return
-%type <NONE> sp_proc_stmt_if sp_proc_stmt_case_simple sp_proc_stmt_case
+%type <NONE> sp_proc_stmt_if
 %type <NONE> sp_labeled_control sp_proc_stmt_unlabeled sp_proc_stmt_leave
 %type <NONE> sp_proc_stmt_iterate
 %type <NONE> sp_proc_stmt_open sp_proc_stmt_fetch sp_proc_stmt_close
+%type <NONE> case_stmt_specification simple_case_stmt searched_case_stmt
 
 %type <num>  sp_decl_idents sp_opt_inout sp_handler_type sp_hcond_list
 %type <spcondtype> sp_cond sp_hcond
@@ -1546,8 +1728,7 @@ ev_sql_stmt_inner:
           sp_proc_stmt_statement
         | sp_proc_stmt_return
         | sp_proc_stmt_if
-        | sp_proc_stmt_case_simple
+        | case_stmt_specification
-        | sp_proc_stmt_case
         | sp_labeled_control
         | sp_proc_stmt_unlabeled
         | sp_proc_stmt_leave
@@ -2311,8 +2492,7 @@ sp_proc_stmt:
 	  sp_proc_stmt_statement
         | sp_proc_stmt_return
 	| sp_proc_stmt_if
-	| sp_proc_stmt_case_simple
+	| case_stmt_specification
-        | sp_proc_stmt_case
 	| sp_labeled_control
 	| sp_proc_stmt_unlabeled
 	| sp_proc_stmt_leave
@@ -2402,49 +2582,6 @@ sp_proc_stmt_return:
 	  }
         ;
 
-sp_proc_stmt_case_simple:
-	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(); }
-        ;
-        
-sp_proc_stmt_case:
-          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();
-	  }
-        ;
-
 sp_proc_stmt_unlabeled:
 	  { /* Unlabeled controls get a secret label. */
 	    LEX *lex= Lex;
@@ -2669,72 +2806,114 @@ sp_elseifs:
 	| ELSE sp_proc_stmts1
 	;
 
-sp_case:
+case_stmt_specification:
-	  { Lex->sphead->reset_lex(YYTHD); }
+          simple_case_stmt
-          expr THEN_SYM
+        | searched_case_stmt
-	  {
+        ;
+
+simple_case_stmt:
+          CASE_SYM
+          {
             LEX *lex= Lex;
-	    sp_head *sp= lex->sphead;
+            case_stmt_action_case(lex);
-	    sp_pcontext *ctx= Lex->spcont;
+            lex->sphead->reset_lex(YYTHD); /* For expr $3 */
-	    uint ip= sp->instructions();
+          }
-	    sp_instr_jump_if_not *i;
+          expr
+          {
+            LEX *lex= Lex;
+            if (case_stmt_action_expr(lex, $3))
+              YYABORT;
 
-	    if (! (sp->m_flags & sp_head::IN_SIMPLE_CASE))
+            lex->sphead->restore_lex(YYTHD); /* For expr $3 */
-	      i= new sp_instr_jump_if_not(ip, ctx, $2, lex);
+          }
-	    else
+          simple_when_clause_list
-	    { /* Simple case: <caseval> = <whenval> */
+          else_clause_opt
+          END
+          CASE_SYM
+          {
+            LEX *lex= Lex;
+            case_stmt_action_end_case(lex, true);
+          }
+        ;
 
-	      Item_case_expr *var;
+searched_case_stmt:
-              Item *expr;
+          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
+searched_when_clause_list:
-              if (var)
+          searched_when_clause
-                var->m_sp= sp;
+        | searched_when_clause_list searched_when_clause
-#endif
+        ;
 
-	      expr= new Item_func_eq(var, $2);
+simple_when_clause:
+          WHEN_SYM
+          {
+            Lex->sphead->reset_lex(YYTHD); /* For expr $3 */
+          }
+          expr
+          {
+            /* Simple case: <caseval> = <whenval> */
 
-	      i= new sp_instr_jump_if_not(ip, ctx, expr, lex);
+            LEX *lex= Lex;
-	    }
+            case_stmt_action_when(lex, $3, true);
-	    sp->push_backpatch(i, ctx->push_label((char *)"", 0));
+            lex->sphead->restore_lex(YYTHD); /* For expr $3 */
-            sp->add_cont_backpatch(i);
+          }
+          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);
+          }
-	  }
+        | ELSE sp_proc_stmts1
-	  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 {}
-	;
 
 sp_labeled_control:
 	  label_ident ':'
@@ -4922,7 +5101,6 @@ opt_ev_sql_stmt: /* empty*/ { $$= 0;}
         | DO_SYM ev_sql_stmt { $$= 1; }
         ;
 
-
 ident_or_empty:
 	/* empty */  { $$.str= 0; $$.length= 0; }
 	| ident      { $$= $1; };
@@ -6130,8 +6308,8 @@ simple_expr:
             if (!$$)
               YYABORT;
 	  }
-	| CASE_SYM opt_expr WHEN_SYM when_list opt_else END
+	| CASE_SYM opt_expr when_list opt_else END
-	  { $$= new (YYTHD->mem_root) Item_func_case(* $4, $2, $5 ); }
+	  { $$= new (YYTHD->mem_root) Item_func_case(* $3, $2, $4 ); }
 	| CONVERT_SYM '(' expr ',' cast_type ')'
 	  {
 	    $$= create_func_cast(YYTHD, $3, $5,
@@ -6349,7 +6527,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.
 */
@@ -6564,7 +6742,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).
           */
@@ -6666,7 +6844,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 ')'
@@ -6830,23 +7008,19 @@ opt_else:
 	| ELSE expr    { $$= $2; };
 
 when_list:
-        { Select->when_list.push_front(new List<Item>); }
+          WHEN_SYM expr THEN_SYM expr
-	when_list2
+          {
-	{ $$= Select->when_list.pop(); };
+            $$= new List<Item>;
-
+            $$->push_back($2);
-when_list2:
+            $$->push_back($4);
-	expr THEN_SYM expr
+          }
-	  {
+        | when_list WHEN_SYM expr THEN_SYM expr
-	    SELECT_LEX *sel=Select;
+          {
-	    sel->when_list.head()->push_back($1);
+            $1->push_back($3);
-	    sel->when_list.head()->push_back($3);
+            $1->push_back($5);
-	}
+            $$= $1;
-	| 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);
-	  };
 
 /* Warning - may return NULL in case of incomplete SELECT */
 table_ref:

storage/csv/ha_tina.cc

@@ -516,14 +516,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

tests/mysql_client_test.c

@@ -15683,6 +15683,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
@@ -15965,6 +15992,7 @@ static struct my_tests_st my_tests[]= {
   { "test_bug23383", test_bug23383 },
   { "test_bug21635", test_bug21635 },
   { "test_status", test_status},
+  { "test_bug24179", test_bug24179 },
   { 0, 0 }
 };