Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix SRO + multi-table update statements in replication #11

Closed
wants to merge 1 commit into from
Closed

Fix SRO + multi-table update statements in replication #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 12 additions & 2 deletions mysql-test/suite/rpl/r/rpl_ignore_super_read_only.result
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
#
Expand All @@ -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
9 changes: 7 additions & 2 deletions mysql-test/suite/rpl/t/rpl_ignore_super_read_only.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 #
Expand All @@ -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;
Expand All @@ -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;
11 changes: 2 additions & 9 deletions sql/handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include "debug_sync.h" // DEBUG_SYNC
#include <my_bit.h>
#include <list>
#include "sql_readonly.h" // check_ro

#ifdef WITH_PARTITION_STORAGE_ENGINE
#include "ha_partition.h"
Expand Down Expand Up @@ -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)
{
Expand Down
9 changes: 3 additions & 6 deletions sql/lock.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
#include "sql_acl.h" // SUPER_ACL
#include <hash.h>
#include <assert.h>
#include "sql_readonly.h" // check_ro

/**
@defgroup Locking Locking
Expand Down Expand Up @@ -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.
Expand All @@ -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);

Expand Down Expand Up @@ -197,10 +195,9 @@ 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);
}
}
Expand Down
8 changes: 2 additions & 6 deletions sql/sql_parse.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
{
Expand Down
18 changes: 18 additions & 0 deletions sql/sql_readonly.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Santosh,

The error is related to running a slave in super-read-only with statement or mixed replication, then executing a multi-table update statement on the master. Specifically, if you have SRO set on the slave, then execute the following on the master, replication will break on the update statement:

create table t1(a int);
create table t2(a int, b int);
insert into t1 values(1), (2);
insert into t2 (a) values (1), (2), (3), (4);
update t2 natural join t1 set b=3;

The message when it breaks is:

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 problem is in sql_parse.cc lines 3834-3836, in that it isn't considering the value of thd->slave_thread (which is considered on lock.cc:201, handler.cc:1479, and sql_trigger.cc:464). I included the change to transaction.cc because it used similar logic and was also missing the check for slave_thread, though I'm not sure if/what error that one would generate.

The reason for the refactoring was to prevent any future issues with this, since everything could just start checking the function instead.

)
);
}

#endif /* SQL_READONLY_INCLUDED */
6 changes: 2 additions & 4 deletions sql/sql_trigger.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "sql_handler.h" // mysql_ha_rm_tables
#include "sp_cache.h" // sp_invalidate_cache
#include <mysys_err.h>
#include "sql_readonly.h" // check_ro

/*************************************************************************/

Expand Down Expand Up @@ -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
Expand All @@ -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");
Expand Down
6 changes: 2 additions & 4 deletions sql/transaction.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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");
Expand Down