WL#3602 (SET GLOBAL READONLY)

Bug#11733 (COMMITs should not happen if read-only is set)
Bug#22009 (Can write to a read-only server under some circumstances)

See the work log for details

The change consist of
a) acquiring the global read lock in SET GLOBAL READONLY
b) honoring opt_readonly in ha_commit_trans(),
c) honoring opt_readonly in mysql_lock_tables().

a) takes care of the server stability,
b) makes the transactional tables safe (Bug 11733)
c) makes the non transactional tables safe (Bug 22009)

mysql-test/r/read_only.result

@@ -39,6 +39,56 @@ delete t1 from t1,t3 where t1.a=t3.a;
 drop table t1;
 insert into t1 values(1);
 ERROR HY000: The MySQL server is running with the --read-only option so it cannot execute this statement
+set global read_only=0;
+lock table t1 write;
+lock table t2 write;
+set global read_only=1;
+ERROR HY000: Can't execute the given command because you have active locked tables or an active transaction
+unlock tables ;
+set global read_only=1;
+select @@global.read_only;
+@@global.read_only
+0
+unlock tables ;
+select @@global.read_only;
+@@global.read_only
+1
+set global read_only=0;
+lock table t1 read;
+lock table t2 read;
+set global read_only=1;
+ERROR HY000: Can't execute the given command because you have active locked tables or an active transaction
+unlock tables ;
+set global read_only=1;
+select @@global.read_only;
+@@global.read_only
+0
+unlock tables ;
+select @@global.read_only;
+@@global.read_only
+1
+set global read_only=0;
+BEGIN;
+BEGIN;
+set global read_only=1;
+ERROR HY000: Can't execute the given command because you have active locked tables or an active transaction
+ROLLBACK;
+set global read_only=1;
+select @@global.read_only;
+@@global.read_only
+1
+ROLLBACK;
+set global read_only=0;
+flush tables with read lock;
+set global read_only=1;
+unlock tables;
+set global read_only=0;
+flush tables with read lock;
+set global read_only=1;
+select @@global.read_only;
+@@global.read_only
+1
+unlock tables;
+set global read_only=0;
 drop table t1,t2;
 drop user test@localhost;
-set global read_only=0;

mysql-test/r/read_only_innodb.result

+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/t/read_only.test

@@ -101,8 +101,114 @@ drop table t1;
 --error 1290
 insert into t1 values(1);
 
+#
+# BUG#11733: COMMITs should not happen if read-only is set
+#
+
+# LOCK TABLE ... WRITE / READ_ONLY
+# - is an error in the same connection
+# - is ok in a different connection
+
 connection default;
-drop table t1,t2;
-drop user test@localhost;
+set global read_only=0;
+lock table t1 write;
+
+connection con1;
+lock table t2 write;
+
+connection default;
+--error ER_LOCK_OR_ACTIVE_TRANSACTION
+set global read_only=1;
+unlock tables ;
+# The following call blocks until con1 releases the write lock.
+# Blocking is expected.
+send set global read_only=1;
+
+connection con1;
+--sleep 1
+select @@global.read_only;
+unlock tables ;
+--sleep 1
+select @@global.read_only;
+
+connection default;
+reap;
 
+# LOCK TABLE ... READ / READ_ONLY
+# - is an error in the same connection
+# - is ok in a different connection
+
+connection default;
 set global read_only=0;
+lock table t1 read;
+
+connection con1;
+lock table t2 read;
+
+connection default;
+--error ER_LOCK_OR_ACTIVE_TRANSACTION
+set global read_only=1;
+unlock tables ;
+# The following call blocks until con1 releases the read lock.
+# Blocking is a limitation, and could be improved.
+send set global read_only=1;
+
+connection con1;
+--sleep 1
+select @@global.read_only;
+unlock tables ;
+--sleep 1
+select @@global.read_only;
+
+connection default;
+reap;
+
+# pending transaction / READ_ONLY
+# - is an error in the same connection
+# - is ok in a different connection
+
+connection default;
+set global read_only=0;
+BEGIN;
+
+connection con1;
+BEGIN;
+
+connection default;
+--error ER_LOCK_OR_ACTIVE_TRANSACTION
+set global read_only=1;
+ROLLBACK;
+set global read_only=1;
+
+connection con1;
+select @@global.read_only;
+ROLLBACK;
+
+# Verify that FLUSH TABLES WITH READ LOCK do not block READ_ONLY
+# - in the same SUPER connection
+# - in another SUPER connection
+
+connection default;
+set global read_only=0;
+flush tables with read lock;
+set global read_only=1;
+unlock tables;
+
+connect (root2,localhost,root,,test);
+
+connection default;
+set global read_only=0;
+flush tables with read lock;
+
+connection root2;
+set global read_only=1;
+
+connection default;
+select @@global.read_only;
+unlock tables;
+
+# Cleanup
+connection default;
+set global read_only=0;
+drop table t1,t2;
+drop user test@localhost;

mysql-test/t/read_only_innodb.test

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

sql/handler.cc

@@ -692,6 +692,15 @@ int ha_commit_trans(THD *thd, bool all)
       ha_rollback_trans(thd, all);
       DBUG_RETURN(1);
     }
+
+    if (is_real_trans && opt_readonly)
+    {
+      my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
+      ha_rollback_trans(thd, all);
+      error= 1;
+      goto end;
+    }
+
     DBUG_EXECUTE_IF("crash_commit_before", abort(););
 
     /* Close all cursors that can not survive COMMIT */

sql/lock.cc

@@ -151,6 +151,19 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count,
       }
     }
 
+    if (write_lock_used && opt_readonly)
+    {
+      /*
+	Someone has issued SET GLOBAL READ_ONLY=1 and we want a write lock.
+        We do not wait for READ_ONLY=0, and fail.
+      */
+      reset_lock_data(sql_lock);
+      my_free((gptr) sql_lock, MYF(0));
+      sql_lock=0;
+      my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
+      break;
+    }
+
     thd->proc_info="System lock";
     DBUG_PRINT("info", ("thd->proc_info %s", thd->proc_info));
     if (lock_external(thd, tables, count))

sql/set_var.cc

@@ -370,7 +370,7 @@ sys_var_thd_ulong       sys_preload_buff_size("preload_buffer_size",
                                               &SV::preload_buff_size);
 sys_var_thd_ulong	sys_read_buff_size("read_buffer_size",
 					   &SV::read_buff_size);
-sys_var_bool_ptr	sys_readonly("read_only", &opt_readonly);
+sys_var_opt_readonly    sys_readonly("read_only", &opt_readonly);
 sys_var_thd_ulong	sys_read_rnd_buff_size("read_rnd_buffer_size",
 					       &SV::read_rnd_buff_size);
 sys_var_thd_ulong	sys_div_precincrement("div_precision_increment",
@@ -3880,6 +3880,70 @@ bool sys_var_trust_routine_creators::update(THD *thd, set_var *var)
   return sys_var_bool_ptr::update(thd, var);
 }
 
+bool sys_var_opt_readonly::update(THD *thd, set_var *var)
+{
+  bool result;
+
+  DBUG_ENTER("sys_var_opt_readonly::update");
+
+  /* Prevent self dead-lock */
+  if (thd->locked_tables || thd->active_transaction())
+  {
+    my_error(ER_LOCK_OR_ACTIVE_TRANSACTION, MYF(0));
+    DBUG_RETURN(true);
+  }
+
+  if (thd->global_read_lock)
+  {
+    /*
+      This connection already holds the global read lock.
+      This can be the case with:
+      - FLUSH TABLES WITH READ LOCK
+      - SET GLOBAL READ_ONLY = 1
+    */
+    result= sys_var_bool_ptr::update(thd, var);
+    DBUG_RETURN(result);
+  }
+
+  /*
+    Perform a 'FLUSH TABLES WITH READ LOCK'.
+    This is a 3 step process:
+    - [1] lock_global_read_lock()
+    - [2] close_cached_tables()
+    - [3] make_global_read_lock_block_commit()
+    [1] prevents new connections from obtaining tables locked for write.
+    [2] waits until all existing connections close their tables.
+    [3] prevents transactions from being committed.
+  */
+
+  if (lock_global_read_lock(thd))
+    DBUG_RETURN(true);
+
+  /*
+    This call will be blocked by any connection holding a READ or WRITE lock.
+    Ideally, we want to wait only for pending WRITE locks, but since:
+    con 1> LOCK TABLE T FOR READ;
+    con 2> LOCK TABLE T FOR WRITE; (blocked by con 1)
+    con 3> SET GLOBAL READ ONLY=1; (blocked by con 2)
+    can cause to wait on a read lock, it's required for the client application
+    to unlock everything, and acceptable for the server to wait on all locks.
+  */
+  if (close_cached_tables(thd, true, NULL, false))
+    goto end_with_read_lock;
+
+  if (result= make_global_read_lock_block_commit(thd))
+    goto end_with_read_lock;
+
+  /* Change the opt_readonly system variable, safe because the lock is held */
+  result= sys_var_bool_ptr::update(thd, var);
+
+end_with_read_lock:
+  /* Release the lock */
+  unlock_global_read_lock(thd);
+  DBUG_RETURN(result);
+}
+
+
 /* even session variable here requires SUPER, because of -#o,file */
 bool sys_var_thd_dbug::check(THD *thd, set_var *var)
 {

sql/set_var.h

@@ -905,6 +905,20 @@ public:
 };
 
 
+/**
+  Handler for setting the system variable --read-only.
+*/
+
+class sys_var_opt_readonly :public sys_var_bool_ptr
+{
+public:
+  sys_var_opt_readonly(const char *name_arg, my_bool *value_arg) :
+    sys_var_bool_ptr(name_arg, value_arg) {};
+  ~sys_var_opt_readonly() {};
+  bool update(THD *thd, set_var *var);
+};
+
+
 class sys_var_thd_lc_time_names :public sys_var_thd
 {
 public: