Hi!! On Tue, Apr 1, 2025 at 9:47 PM Sergei Golubchik <serg@mariadb.org> wrote: <cut>
MDEV-36425 Extend read_only to also block share locks and super user
<cut>
diff --git a/mysql-test/main/read_only.result b/mysql-test/main/read_only.result index 65cc12ffce9..89390ad65d5 100644 --- a/mysql-test/main/read_only.result +++ b/mysql-test/main/read_only.result @@ -69,11 +69,14 @@ set global read_only=1; connection con1; select @@global.read_only; @@global.read_only -0 +OFF unlock tables ; +Timeout in wait_condition.inc for SELECT @@global.read_only= 1 +Id User Host db Command Time State Info Progress +5 test localhost test Query 0 starting show full processlist 0.000
Forgot to fix? It should wait for @@global.read_only='READ_ONLY' now
Yes, I had missed that. I somehow thought that comparing the value with 1 would work, similar to one allowing to set the value to 1. Fixed.
--- a/sql/lock.cc +++ b/sql/lock.cc @@ -179,19 +198,48 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags)
<cut>
- if (t->reginfo.lock_type >= TL_FIRST_WRITE && - !ignore_read_only && opt_readonly && !thd->slave_thread) + switch (opt_readonly) // Impossible
you have "Impossible" comment on a wrong line
fixed
{ - my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only"); - DBUG_RETURN(1); + case READONLY_OFF: + DBUG_ASSERT(0); + break; + case READONLY_ON: // Compatibility + if (!(thd->security_ctx->master_access & PRIV_IGNORE_READ_ONLY) && + t->reginfo.lock_type >= TL_FIRST_WRITE) + { + mariadb_error_read_only(); + DBUG_RETURN(1);
I'd make mariadb_error_read_only() to return 1. Then you can write DBUG_RETURN(mariadb_error_read_only()); here and also in all other places, check it out, it's always followed by return 1.
I thought about that but not sure if it makes any sense. I do not like to have a function that only returns one value. It makes more sense to have the function as void. The end result from the compiler should be the same.
+ } + break; + case READONLY_NO_LOCK: + if (!(thd->security_ctx->master_access & PRIV_IGNORE_READ_ONLY) && + (thd->lex->sql_command == SQLCOM_LOCK_TABLES || + t->reginfo.lock_type >= TL_FIRST_WRITE || + t->reginfo.lock_type == TL_READ_WITH_SHARED_LOCKS))
may be t->reginfo.lock_type >= TL_READ_WITH_SHARED_LOCKS ?
No, it has to be TL_READ_WITH_SHARED_LOCKS as we have higher locks like TL_READ_SKIP_LOCKED that should not be part of this. I did not want to change order of lock enum's in this patch. <cut>
index d65cec37154..34ee8391009 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3448,9 +3452,12 @@ class THD: public THD_count, /* this must be first */ */ inline bool is_read_only_ctx()
rename to, for example, check_read_only_with_errror()
good idea, will do.
diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc index 0247afd91a6..545b4674da1 100644 --- a/sql/sys_vars.cc +++ b/sql/sys_vars.cc <cut>
Sys_readonly( "read_only", - "Make all non-temporary tables read-only, with the exception for " - "replication (slave) threads and users with the 'READ ONLY ADMIN' " - "privilege", - GLOBAL_VAR(read_only), CMD_LINE(OPT_ARG), DEFAULT(FALSE), - NO_MUTEX_GUARD, NOT_IN_BINLOG, + "Do not allow changes to non-temporary tables. Options are:" + "OFF changes allowed. " + "READ_ONLY blocks changes for users without the 'READ ONLY ADMIN' "
"Disallow changes for users without the READ ONLY ADMIN privilege"
+ "privilege. " + "READ_ONLY_NO_LOCK blocks in addition LOCK TABLES and " + "SELECT IN SHARE MODE for not ADMIN users. "
"Additionally disallows LOCK TABLES and SELECT IN SHARE MODE"
+ "READ_ONLY_NO_LOCK_NO_ADMIN blocks in addition 'ADMIN' users. "
"Disallows also for users with READ_ONLY ADMIN privilege"
Fixed. The only thing left is the naming of the options. Will call you about that. Regards, Monty