From fd5b0cace97b07d97ea28ce3391f8125a8ebc68b Mon Sep 17 00:00:00 2001 From: Chris Giard Date: Mon, 15 Dec 2014 13:04:27 -0800 Subject: [PATCH] Fix SRO + multi-table update statements in replication when logged in statement form. Summary: This was contributed as: https://github.com/facebook/mysql-5.6/pull/11 This is a bug fix for super-read-only not being ignored for the replication thread on multi-table update statements in sql_parse.cc that would cause replication the stop with the error "Error 'The MySQL server is running with the --read-only (super) option so it cannot execute this statement' on query. Default database: 'test'. Query: 'update t2 natural join t1 set b=3'". The same issue was found in transaction.cc. To help avoid this issue in the future the logic for testing if read-only should be enforced is refactored into a new inline function, and existing code that performed the logic in each location was changed to instead call this function. Test Plan: mtr test Reviewers: steaphan, jtolmer Reviewed By: jtolmer --- .../rpl/r/rpl_ignore_super_read_only.result | 14 ++++++++++++-- .../rpl/t/rpl_ignore_super_read_only.test | 9 +++++++-- sql/handler.cc | 11 ++--------- sql/lock.cc | 10 ++++------ sql/sql_parse.cc | 8 ++------ sql/sql_readonly.h | 18 ++++++++++++++++++ sql/sql_trigger.cc | 6 ++---- sql/transaction.cc | 6 ++---- 8 files changed, 49 insertions(+), 33 deletions(-) create mode 100644 sql/sql_readonly.h diff --git a/mysql-test/suite/rpl/r/rpl_ignore_super_read_only.result b/mysql-test/suite/rpl/r/rpl_ignore_super_read_only.result index 586ed5d672c2..dceddb5321c4 100644 --- a/mysql-test/suite/rpl/r/rpl_ignore_super_read_only.result +++ b/mysql-test/suite/rpl/r/rpl_ignore_super_read_only.result @@ -1,5 +1,6 @@ # This unit test is only to test super_read_only=1 doesn't cause -# errors when executing "CHANGE MASTER". +# errors when executing "CHANGE MASTER" or from statements executed +# via replication. # # It is not intended to test general super_read_only functionality. # @@ -12,18 +13,27 @@ Note #### Storing MySQL user name or password information in the master info rep [connection master] create table t1(a int); insert into t1 values(1); +create table t2(a int, b int); +insert into t2 (a) values (1), (2), (3), (4); set @@global.super_read_only=1; include/stop_slave.inc change master to master_connect_retry=20; include/start_slave.inc insert into t1 values(2); +update t2 natural join t1 set b=3; select * from t1; a 1 2 +select * from t2; +a b +1 3 +2 3 +3 NULL +4 NULL set @@global.read_only_slave=0; set @@global.super_read_only=0; set @@global.read_only=0; set @@global.read_only_slave=1; -drop table t1; +drop table t1, t2; include/rpl_end.inc diff --git a/mysql-test/suite/rpl/t/rpl_ignore_super_read_only.test b/mysql-test/suite/rpl/t/rpl_ignore_super_read_only.test index 914f9411b420..d6e49ed85f2f 100644 --- a/mysql-test/suite/rpl/t/rpl_ignore_super_read_only.test +++ b/mysql-test/suite/rpl/t/rpl_ignore_super_read_only.test @@ -1,5 +1,6 @@ --echo # This unit test is only to test super_read_only=1 doesn't cause ---echo # errors when executing "CHANGE MASTER". +--echo # errors when executing "CHANGE MASTER" or from statements executed +--echo # via replication. --echo # --echo # It is not intended to test general super_read_only functionality. --echo # @@ -11,6 +12,8 @@ source include/master-slave.inc; connection master; create table t1(a int); insert into t1 values(1); +create table t2(a int, b int); +insert into t2 (a) values (1), (2), (3), (4); sync_slave_with_master; set @@global.super_read_only=1; @@ -20,15 +23,17 @@ source include/start_slave.inc; connection master; insert into t1 values(2); +update t2 natural join t1 set b=3; sync_slave_with_master; select * from t1; +select * from t2; set @@global.read_only_slave=0; set @@global.super_read_only=0; set @@global.read_only=0; set @@global.read_only_slave=1; connection master; -drop table t1; +drop table t1, t2; source include/rpl_end.inc; diff --git a/sql/handler.cc b/sql/handler.cc index 90989667a3c5..0862b0d946fd 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -40,6 +40,7 @@ #include "debug_sync.h" // DEBUG_SYNC #include #include +#include "sql_readonly.h" // check_ro #ifdef WITH_PARTITION_STORAGE_ENGINE #include "ha_partition.h" @@ -1465,18 +1466,10 @@ int ha_commit_trans(THD *thd, bool all, bool async, DEBUG_SYNC(thd, "ha_commit_trans_after_acquire_commit_lock"); } - bool enforce_ro = true; - if (!opt_super_readonly) - enforce_ro = !(thd->security_ctx->master_access & SUPER_ACL); // Ignore super_read_only when ignore_global_read_lock is set. // ignore_global_read_lock is set for transactions on replication // repository tables. - if (ignore_global_read_lock) - enforce_ro = false; - if (rw_trans && - opt_readonly && - enforce_ro && - !thd->slave_thread) + if (rw_trans && check_ro(thd) && !ignore_global_read_lock) { if (opt_super_readonly) { diff --git a/sql/lock.cc b/sql/lock.cc index 654f3d64fc87..814f7e93dc95 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -82,6 +82,7 @@ #include "sql_acl.h" // SUPER_ACL #include #include +#include "sql_readonly.h" // check_ro /** @defgroup Locking Locking @@ -116,7 +117,6 @@ static int lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) { uint system_count= 0, i= 0; - bool enforce_ro = true; /* Identifies if the executed sql command can updated either a log or rpl info table. @@ -125,8 +125,6 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) DBUG_ENTER("lock_tables_check"); - if (!opt_super_readonly) - enforce_ro = !(thd->security_ctx->master_access & SUPER_ACL); log_table_write_query= is_log_table_write_query(thd->lex->sql_command); @@ -197,10 +195,10 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) */ if (!(flags & MYSQL_LOCK_IGNORE_GLOBAL_READ_ONLY) && !t->s->tmp_table) { - if (t->reginfo.lock_type >= TL_WRITE_ALLOW_WRITE && - enforce_ro && opt_readonly && !thd->slave_thread) + if (t->reginfo.lock_type >= TL_WRITE_ALLOW_WRITE && check_ro(thd)) { - my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only"); + my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), + opt_super_readonly ? "--read-only (super)" : "--read-only"); DBUG_RETURN(1); } } diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 4799921c87c1..0c5e4a0ee0dc 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -102,6 +102,7 @@ #include "sql_analyse.h" #include "table_cache.h" // table_cache_manager #include "sql_timer.h" // thd_timer_set, thd_timer_reset +#include "sql_readonly.h" // check_ro #ifdef HAVE_JEMALLOC #ifndef EMBEDDED_LIBRARY @@ -3828,12 +3829,7 @@ case SQLCOM_PREPARE: #endif /* HAVE_REPLICATION */ if (res) break; - bool enforce_ro = true; - if (!opt_super_readonly) - enforce_ro = !(thd->security_ctx->master_access & SUPER_ACL); - if (opt_readonly && - enforce_ro && - some_non_temp_table_to_be_updated(thd, all_tables)) + if (check_ro(thd) && some_non_temp_table_to_be_updated(thd, all_tables)) { if (opt_super_readonly) { diff --git a/sql/sql_readonly.h b/sql/sql_readonly.h new file mode 100644 index 000000000000..bdb06373dda9 --- /dev/null +++ b/sql/sql_readonly.h @@ -0,0 +1,18 @@ +#ifndef SQL_READONLY_INCLUDED +#define SQL_READONLY_INCLUDED + +#include "sql_acl.h" /* SUPER_ACL */ +#include "sql_class.h" /* THD class */ +#include "mysqld.h" /* opt_readonly and opt_super_readonly */ + +static inline bool check_ro(THD *thd) +{ + return ( + opt_readonly && ( + !(thd->security_ctx->master_access & SUPER_ACL) || + (opt_super_readonly && !(thd->slave_thread)) + ) + ); +} + +#endif /* SQL_READONLY_INCLUDED */ diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc index 661dae6a4169..86e36d26787d 100644 --- a/sql/sql_trigger.cc +++ b/sql/sql_trigger.cc @@ -33,6 +33,7 @@ #include "sql_handler.h" // mysql_ha_rm_tables #include "sp_cache.h" // sp_invalidate_cache #include +#include "sql_readonly.h" // check_ro /*************************************************************************/ @@ -444,9 +445,6 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create) if (!create) { bool if_exists= thd->lex->drop_if_exists; - bool enforce_ro= true; - if (!opt_super_readonly) - enforce_ro= !(thd->security_ctx->master_access & SUPER_ACL); /* Protect the query table list from the temporary and potentially @@ -461,7 +459,7 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create) */ thd->lex->sql_command= backup.sql_command; - if (opt_readonly && enforce_ro && !thd->slave_thread) + if (check_ro(thd)) { my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), opt_super_readonly ? "--read-only (super)" : "--read-only"); diff --git a/sql/transaction.cc b/sql/transaction.cc index fe65ec220a51..8d95c190376f 100644 --- a/sql/transaction.cc +++ b/sql/transaction.cc @@ -20,6 +20,7 @@ #include "rpl_master.h" #include "debug_sync.h" // DEBUG_SYNC #include "sql_acl.h" // SUPER_ACL +#include "sql_readonly.h" // check_ro /** Check if we have a condition where the transaction state must @@ -172,10 +173,7 @@ bool trans_begin(THD *thd, uint flags, bool* need_ok) Implicitly starting a RW transaction is allowed for backward compatibility. */ - bool enforce_ro = true; - if (!opt_super_readonly) - enforce_ro = !(thd->security_ctx->master_access & SUPER_ACL); - if (opt_readonly && enforce_ro) + if (check_ro(thd)) { my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), opt_super_readonly ? "--read-only (super)" : "--read-only");