regression in tidb_restricted_read_only - can not disable after enabling

[morgo]

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

SET GLOBAL tidb_restricted_read_only=1;
SET GLOBAL tidb_restricted_read_only=0;
CREATE TABLE test.t1 (a int);

2. What did you expect to see? (Required)

The create table statement should succeed.

This is a regression bisected to #31746

3. What did you see instead (Required)

mysql> CREATE TABLE test.t1 (a int);
ERROR 1836 (HY000): Running in read-only mode

4. What is your TiDB version? (Required)

tidb> SELECT tidb_version()\G
*************************** 1. row ***************************
tidb_version(): Release Version: v6.0.0-alpha-56-g9a4ca3ca6
Edition: Community
Git Commit Hash: 9a4ca3ca6919699fb4f0da72edd7151c56f84edd
Git Branch: master
UTC Build Time: 2022-03-10 20:12:59
GoVersion: go1.16.15
Race Enabled: false
TiKV Min Version: v3.0.0-60965b006877ca7234adaced7890d7b029ed1306
Check Table Before Drop: false
1 row in set (0.00 sec)

[morgo]

@ichn-hu PTAL :-)

[ichn-hu]

I suppose this is the by-design behavior of tidb_restricted_read_only, as the transmissibility requirements in #31746 (review) states.

[ichn-hu]

we need to update the docs though

[morgo]

I suppose this is the by-design behavior of tidb_restricted_read_only, as the transmissibility requirements in #31746 (review) states.

The design has a usability problem if it allows the value to be changed, but not effective. If this is the intended case, then attempting to set it to 0 again should return an error saying that another value must be modified first.

[ichn-hu]

I think this might be against the original design requirements, as it is for cloud replication scenario, where the cloud service DBA turns on the restricted read-only variable and implicitly turned on super read-only before the replication started, and when it is done, she would turn off the restricted read-only and it is upon to the user to turn off super read-only.

cc @SunRunAway please take a look as this is your design.

[SunRunAway]

Another reference is that the behavior is similar like MySQL’s super_read_only and read_only.

[morgo]

Another reference is that the behavior is similar like MySQL’s super_read_only and read_only.

See the test-case above. I'm not touching the value of super_read_only: I'm just toggling tidb_restricted_read_only on and then off. If it's not supported, there should be an error?

[SunRunAway]

When I change super_read_only from on to off, the read_only will continue to be kept as on.

tidb_restricted_read_only and tidb_super_read_only follow this behaviour.

[morgo]

Here is my suggestion:

morgo@ubuntu:~/go/src/github.com/morgo/tidb$ git diff
diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go
index bdc9150f5..227172955 100644
--- a/sessionctx/variable/sysvar.go
+++ b/sessionctx/variable/sysvar.go
@@ -1063,13 +1063,29 @@ var defaultSysVars = []*SysVar{
        {Scope: ScopeGlobal, Name: TiDBRestrictedReadOnly, Value: BoolToOnOff(DefTiDBRestrictedReadOnly), Type: TypeBool, SetGlobal: func(s *SessionVars, val string) error {
                on := TiDBOptOn(val)
                if on {
-                       err := s.GlobalVarsAccessor.SetGlobalSysVar(TiDBSuperReadOnly, "ON")
+                       // Set SuperReadOnly to ON as well
+                       err := s.GlobalVarsAccessor.SetGlobalSysVarOnly(TiDBSuperReadOnly, "ON")
                        if err != nil {
                                return err
                        }
                }
                RestrictedReadOnly.Store(on)
                return nil
+       }, Validation: func(vars *SessionVars, normalizedValue string, _ string, _ ScopeFlag) (string, error) {
+               // Because setting ON also enables SuperReadOnly, we need to at least WARN
+               // that SETTING ON->OFF won't take effect when SuperReadOnly is ON.
+               // Another option is that we could also toggle SuperReadOnly OFF,
+               // but we don't currently do this.
+               if !TiDBOptOn(normalizedValue) {
+                       result, err := vars.GlobalVarsAccessor.GetGlobalSysVar(TiDBSuperReadOnly)
+                       if err != nil {
+                               return normalizedValue, err
+                       }
+                       if TiDBOptOn(result) {
+                               vars.StmtCtx.AppendWarning(errors.New("you need to set super_read_only OFF too, otherwise this doesn't make sense."))
+                       }
+               }
+               return normalizedValue, nil
        }},
        {Scope: ScopeGlobal, Name: TiDBSuperReadOnly, Value: BoolToOnOff(DefTiDBSuperReadOnly), Type: TypeBool, Validation: func(vars *SessionVars, normalizedValue string, _ string, _ ScopeFlag) (string, error) {
                on := TiDBOptOn(normalizedValue)

[SunRunAway]

It does make sense.
cloudAdmin uses tidb_restricted_read_only while customer(root user) only uses tidb_super_read_only.

When cloudAdmin sets tidb_restricted_read_only to ON, it tells that customer can not write into the database, and any root user can not change this situation.
When cloudAdmin sets tidb_restricted_read_only to OFF, it tells that the customer now has right to change tidb_super_read_only to OFF to make their cluster writable.

[morgo]

Closing as not required. If users use tidb_super_read_only as documented, there is no problem.