diff --git a/mysql-test/main/read_only_debug.result b/mysql-test/main/read_only_debug.result new file mode 100644 index 0000000000000..559e5efe367e0 --- /dev/null +++ b/mysql-test/main/read_only_debug.result @@ -0,0 +1,84 @@ +set @start_read_only= @@global.read_only; +create user test@localhost; +grant SUPER on *.* to test@localhost; +grant SELECT, INSERT, UPDATE, LOCK TABLES on test.* to test@localhost; +create table t1 (a int); +insert into t1 values (1),(2); +connect con1,localhost,test,,test; +connection default; +# +# read_only turned off between the statement check and the error +# +set global read_only='NO_LOCK'; +connection con1; +set debug_sync='after_read_only_check SIGNAL parked WAIT_FOR go'; +update t1 set a= a + 1; +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='OFF'; +set debug_sync='now SIGNAL go'; +connection con1; +ERROR HY000: The MariaDB server is running with the --read-only=NO_LOCK option so it cannot execute this statement +set debug_sync='RESET'; +# +# read_only changed to another level between the statement check and +# the error. The error must still name the level that was rejected on. +# +connection default; +set global read_only='NO_LOCK'; +connection con1; +set debug_sync='after_read_only_check SIGNAL parked WAIT_FOR go'; +update t1 set a= a + 1; +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='ON'; +set debug_sync='now SIGNAL go'; +connection con1; +ERROR HY000: The MariaDB server is running with the --read-only=NO_LOCK option so it cannot execute this statement +set debug_sync='RESET'; +# +# read_only turned off between the lock check and the error +# +connection default; +set global read_only='NO_LOCK'; +connection con1; +set debug_sync='after_lock_tables_read_only_check SIGNAL parked WAIT_FOR go'; +lock tables t1 read; +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='OFF'; +set debug_sync='now SIGNAL go'; +connection con1; +ERROR HY000: The MariaDB server is running with the --read-only=NO_LOCK option so it cannot execute this statement +unlock tables; +set debug_sync='RESET'; +# +# read_only turned off between the start transaction check and the +# error +# +connection default; +set global read_only='ON'; +connection con1; +set debug_sync='after_trans_begin_read_only_check SIGNAL parked WAIT_FOR go'; +start transaction read write; +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='OFF'; +set debug_sync='now SIGNAL go'; +connection con1; +ERROR HY000: The MariaDB server is running with the --read-only=ON option so it cannot execute this statement +set debug_sync='RESET'; +# +# The rows must be unchanged +# +select * from t1; +a +1 +2 +disconnect con1; +connection default; +set debug_sync='RESET'; +drop table t1; +drop user test@localhost; +set global read_only= @start_read_only; +# End of 13.1 tests diff --git a/mysql-test/main/read_only_debug.test b/mysql-test/main/read_only_debug.test new file mode 100644 index 0000000000000..486a03b9a7d80 --- /dev/null +++ b/mysql-test/main/read_only_debug.test @@ -0,0 +1,124 @@ +# Test that changing the read_only option concurrently, while another +# connection has already decided to reject a statement because of it, does +# not confuse the rejection. The error must still be given and must name +# the read_only value the rejection was decided on, not whatever the option +# happens to be by the time the error is produced. + +--source include/not_embedded.inc +--source include/have_debug_sync.inc + +set @start_read_only= @@global.read_only; + +# read_only does not apply to users with the READ ONLY ADMIN privilege, +# so we use a user without it. SUPER is needed to set debug_sync and does +# not imply READ ONLY ADMIN. +create user test@localhost; +grant SUPER on *.* to test@localhost; +grant SELECT, INSERT, UPDATE, LOCK TABLES on test.* to test@localhost; + +create table t1 (a int); +insert into t1 values (1),(2); + +connect (con1,localhost,test,,test); +connection default; + +--echo # +--echo # read_only turned off between the statement check and the error +--echo # + +set global read_only='NO_LOCK'; + +connection con1; +set debug_sync='after_read_only_check SIGNAL parked WAIT_FOR go'; +--send update t1 set a= a + 1 + +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='OFF'; +set debug_sync='now SIGNAL go'; + +connection con1; +--error ER_OPTION_PREVENTS_STATEMENT +--reap +set debug_sync='RESET'; + +--echo # +--echo # read_only changed to another level between the statement check and +--echo # the error. The error must still name the level that was rejected on. +--echo # + +connection default; +set global read_only='NO_LOCK'; + +connection con1; +set debug_sync='after_read_only_check SIGNAL parked WAIT_FOR go'; +--send update t1 set a= a + 1 + +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='ON'; +set debug_sync='now SIGNAL go'; + +connection con1; +--error ER_OPTION_PREVENTS_STATEMENT +--reap +set debug_sync='RESET'; + +--echo # +--echo # read_only turned off between the lock check and the error +--echo # + +connection default; +set global read_only='NO_LOCK'; + +connection con1; +set debug_sync='after_lock_tables_read_only_check SIGNAL parked WAIT_FOR go'; +--send lock tables t1 read + +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='OFF'; +set debug_sync='now SIGNAL go'; + +connection con1; +--error ER_OPTION_PREVENTS_STATEMENT +--reap +unlock tables; +set debug_sync='RESET'; + +--echo # +--echo # read_only turned off between the start transaction check and the +--echo # error +--echo # + +connection default; +set global read_only='ON'; + +connection con1; +set debug_sync='after_trans_begin_read_only_check SIGNAL parked WAIT_FOR go'; +--send start transaction read write + +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='OFF'; +set debug_sync='now SIGNAL go'; + +connection con1; +--error ER_OPTION_PREVENTS_STATEMENT +--reap +set debug_sync='RESET'; + +--echo # +--echo # The rows must be unchanged +--echo # + +select * from t1; + +disconnect con1; +connection default; +set debug_sync='RESET'; +drop table t1; +drop user test@localhost; +set global read_only= @start_read_only; + +--echo # End of 13.1 tests diff --git a/sql/lock.cc b/sql/lock.cc index 79a3f98f5dda6..922d576d1d242 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -104,15 +104,19 @@ extern const char *read_only_mode_names[]; /* Give an error in case if users violates read only state + + @param read_only The opt_readonly value the caller made its decision on. + + The caller has to pass the value it tested, not opt_readonly, as a + concurrent SET GLOBAL read_only can change opt_readonly at any time. + Reading it again here could report a value that no one was denied for, + or, if read_only was just turned off, no value at all. */ -void mariadb_error_read_only() +void mariadb_error_read_only(ulong read_only) { char msg[60]; - int read_only= opt_readonly; DBUG_ASSERT(read_only); - if (unlikely(read_only == 0)) - read_only= 1; // If global readonly changed during call strxnmov(msg, sizeof(msg), "--read-only=", read_only_mode_names[read_only], @@ -200,12 +204,21 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) Prevent modifications to base tables if READ_ONLY is activated. In any case, read only does not apply to temporary tables or slave threads. + + Read opt_readonly once per table, so that this table is checked and + reported against one value even if SET GLOBAL read_only changes it + while we are here. It is read per table, not once for the whole lock + request, so that a read_only that is turned on while we are looping + still stops the remaining tables. */ - if (unlikely(opt_readonly) && + ulong read_only= opt_readonly; + + if (unlikely(read_only) && !(flags & MYSQL_LOCK_IGNORE_GLOBAL_READ_ONLY) && !t->s->tmp_table && !thd->slave_thread) { - switch (opt_readonly) + DEBUG_SYNC(thd, "after_lock_tables_read_only_check"); + switch (read_only) { case READONLY_OFF: // Impossible DBUG_ASSERT(0); @@ -214,7 +227,7 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) if (!(thd->security_ctx->master_access & PRIV_IGNORE_READ_ONLY) && t->reginfo.lock_type >= TL_FIRST_WRITE) { - mariadb_error_read_only(); + mariadb_error_read_only(read_only); DBUG_RETURN(1); } break; @@ -223,7 +236,7 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) (thd->lex->sql_command == SQLCOM_LOCK_TABLES || t->reginfo.lock_type >= TL_BLOCKS_READONLY)) { - mariadb_error_read_only(); + mariadb_error_read_only(read_only); DBUG_RETURN(1); } break; @@ -231,7 +244,7 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) if (thd->lex->sql_command == SQLCOM_LOCK_TABLES || t->reginfo.lock_type >= TL_BLOCKS_READONLY) { - mariadb_error_read_only(); + mariadb_error_read_only(read_only); DBUG_RETURN(1); } break; diff --git a/sql/sql_class.h b/sql/sql_class.h index 25ffcf49d3bab..5fda70c389f7d 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -258,7 +258,7 @@ extern "C" int thd_current_status(MYSQL_THD thd); extern "C" enum enum_server_command thd_current_command(MYSQL_THD thd); extern "C" int thd_double_innodb_cardinality(MYSQL_THD thd); -extern void mariadb_error_read_only(); +extern void mariadb_error_read_only(ulong read_only); /** @class CSET_STRING @@ -3821,11 +3821,12 @@ class THD: public THD_count, /* this must be first */ */ inline bool check_read_only_with_error() { - if (likely(!opt_readonly) || slave_thread || + ulong read_only= opt_readonly; // Read once, may be changed any time + if (likely(!read_only) || slave_thread || ((security_ctx->master_access & PRIV_IGNORE_READ_ONLY) && - opt_readonly != READONLY_NO_LOCK_NO_ADMIN)) + read_only != READONLY_NO_LOCK_NO_ADMIN)) return false; - mariadb_error_read_only(); + mariadb_error_read_only(read_only); return true; } diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 6a1f53f7ba102..1d6ddb2526e67 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -1482,25 +1482,29 @@ dispatch_command_return do_command(THD *thd, bool blocking) @note SQLCOM_MULTI_UPDATE is an exception and dealt with elsewhere. + @param read_only The opt_readonly value to check against. The caller reads + it once and also passes it to mariadb_error_read_only(), + so that the decision and the error message are based on + the same value even if SET GLOBAL read_only changes it. + @see mysql_execute_command @returns Status code @retval TRUE The statement should be denied. @retval FALSE The statement isn't updating any relevant tables. */ -static bool deny_updates_if_read_only_option(THD *thd, TABLE_LIST *all_tables) +static bool deny_updates_if_read_only_option(THD *thd, TABLE_LIST *all_tables, + ulong read_only) { DBUG_ENTER("deny_updates_if_read_only_option"); DBUG_ASSERT(!thd->slave_thread); // Checked by caller - - if (!opt_readonly) - DBUG_RETURN(FALSE); + DBUG_ASSERT(read_only); // Checked by caller LEX *lex= thd->lex; /* Super user is allowed to do changes in some cases */ if ((thd->security_ctx->master_access & PRIV_IGNORE_READ_ONLY) != NO_ACL && - opt_readonly < READONLY_NO_LOCK_NO_ADMIN) + read_only < READONLY_NO_LOCK_NO_ADMIN) DBUG_RETURN(FALSE); /* Check if command doesn't update anything */ @@ -3679,11 +3683,23 @@ mysql_execute_command(THD *thd, bool is_called_from_prepared_stmt) /* When option readonly is set deny operations which change non-temporary tables. Except for the replication thread and the 'super' users. + + opt_readonly is read once here and passed on, so that the check and + the error message use the same value even if SET GLOBAL read_only + changes it in between. The block keeps read_only out of the function + scope also when HAVE_REPLICATION is not defined and the enclosing + 'else' is compiled away. */ - if (deny_updates_if_read_only_option(thd, all_tables)) { - mariadb_error_read_only(); - DBUG_RETURN(-1); + ulong read_only= opt_readonly; + + if (unlikely(read_only) && + deny_updates_if_read_only_option(thd, all_tables, read_only)) + { + DEBUG_SYNC(thd, "after_read_only_check"); + mariadb_error_read_only(read_only); + DBUG_RETURN(-1); + } } #ifdef HAVE_REPLICATION } /* endif unlikely slave */ diff --git a/sql/transaction.cc b/sql/transaction.cc index 6aaa7140b59d2..3046507efd958 100644 --- a/sql/transaction.cc +++ b/sql/transaction.cc @@ -171,12 +171,14 @@ bool trans_begin(THD *thd, uint flags) Implicitly starting a RW transaction is allowed for backward compatibility. */ - if (opt_readonly) + ulong read_only= opt_readonly; // Read once, may be changed any time + if (read_only) { if (!(thd->security_ctx->master_access & PRIV_IGNORE_READ_ONLY) || - opt_readonly == READONLY_NO_LOCK_NO_ADMIN) + read_only == READONLY_NO_LOCK_NO_ADMIN) { - mariadb_error_read_only(); + DEBUG_SYNC(thd, "after_trans_begin_read_only_check"); + mariadb_error_read_only(read_only); DBUG_RETURN(true); } }