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.

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

variables last_plan_from_cache & last_plan_from_binding should be read-only #21943

Closed
SunRunAway opened this issue Dec 22, 2020 · 2 comments · Fixed by #21953
Closed

variables last_plan_from_cache & last_plan_from_binding should be read-only #21943

SunRunAway opened this issue Dec 22, 2020 · 2 comments · Fixed by #21953
Assignees
Labels
component/config good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. severity/moderate sig/sql-infra SIG: SQL Infra type/bug The issue is confirmed as a bug.

Comments

@SunRunAway
Copy link
Contributor

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

mysql> set @@last_plan_from_binding='123';
Query OK, 0 rows affected, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+-----------------------------------------------------------------+
| Level   | Code | Message                                                         |
+---------+------+-----------------------------------------------------------------+
| Warning | 1105 | Set operation for 'last_plan_from_binding' will not take effect |
+---------+------+-----------------------------------------------------------------+
1 row in set (0.00 sec)

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

mysql> set @@last_plan_from_binding='123';
ERROR 1105 (HY000): Variable 'version_comment' is a read only variable

3. What did you see instead (Required)

4. What is your TiDB version? (Required)

@SunRunAway SunRunAway added type/bug The issue is confirmed as a bug. component/config labels Dec 22, 2020
@SunRunAway SunRunAway added the sig/sql-infra SIG: SQL Infra label Dec 22, 2020
@morgo morgo added the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Dec 22, 2020
@morgo
Copy link
Contributor

morgo commented Dec 22, 2020

I think this is easy for new contributors. The SysVar struct supports the ability to set a sysvar as read-only:

diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go
index eb6710392..00d3034c3 100644
--- a/sessionctx/variable/sysvar.go
+++ b/sessionctx/variable/sysvar.go
@@ -1131,8 +1131,8 @@ var defaultSysVars = []*SysVar{
        {Scope: ScopeSession, Name: TiDBEnableSlowLog, Value: BoolToOnOff(logutil.DefaultTiDBEnableSlowLog), Type: TypeBool},
        {Scope: ScopeSession, Name: TiDBQueryLogMaxLen, Value: strconv.Itoa(logutil.DefaultQueryLogMaxLen), Type: TypeInt, MinValue: -1, MaxValue: math.MaxInt64},
        {Scope: ScopeSession, Name: TiDBCheckMb4ValueInUTF8, Value: BoolToOnOff(config.GetGlobalConfig().CheckMb4ValueInUTF8), Type: TypeBool},
-       {Scope: ScopeSession, Name: TiDBFoundInPlanCache, Value: BoolToOnOff(DefTiDBFoundInPlanCache), Type: TypeBool},
-       {Scope: ScopeSession, Name: TiDBFoundInBinding, Value: BoolToOnOff(DefTiDBFoundInBinding), Type: TypeBool},
+       {Scope: ScopeSession, Name: TiDBFoundInPlanCache, Value: BoolToOnOff(DefTiDBFoundInPlanCache), Type: TypeBool, ReadOnly: true},
+       {Scope: ScopeSession, Name: TiDBFoundInBinding, Value: BoolToOnOff(DefTiDBFoundInBinding), Type: TypeBool, ReadOnly: true},
        {Scope: ScopeSession, Name: TiDBEnableCollectExecutionInfo, Value: BoolToOnOff(DefTiDBEnableCollectExecutionInfo), Type: TypeBool},
        {Scope: ScopeGlobal | ScopeSession, Name: TiDBAllowAutoRandExplicitInsert, Value: BoolToOnOff(DefTiDBAllowAutoRandExplicitInsert), Type: TypeBool},
        {Scope: ScopeGlobal | ScopeSession, Name: TiDBEnableClusteredIndex, Value: BoolToOnOff(DefTiDBEnableClusteredIndex), Type: TypeBool},

This is checked when changing a global variable, but not yet a sessionvar. It is also an easy fix:

diff --git a/executor/set.go b/executor/set.go
index f1944bc1e..41a6c06db 100644
--- a/executor/set.go
+++ b/executor/set.go
@@ -130,7 +130,7 @@ func (e *SetExecutor) setSysVariable(name string, v *expression.VarAssignment) e
        if sysVar == nil {
                return variable.ErrUnknownSystemVar.GenWithStackByArgs(name)
        }
-       if sysVar.Scope == variable.ScopeNone {
+       if sysVar.Scope == variable.ScopeNone || sysVar.ReadOnly {
                return errors.Errorf("Variable '%s' is a read only variable", name)
        }
        var valStr string

The code that sets the current warnings can also be removed.

@ti-srebot
Copy link
Contributor

Please edit this comment or add a new comment to complete the following information

Not a bug

  1. Remove the 'type/bug' label
  2. Add notes to indicate why it is not a bug

Duplicate bug

  1. Add the 'type/duplicate' label
  2. Add the link to the original bug

Bug

Note: Make Sure that 'component', and 'severity' labels are added
Example for how to fill out the template: #20100

1. Root Cause Analysis (RCA) (optional)

2. Symptom (optional)

3. All Trigger Conditions (optional)

4. Workaround (optional)

5. Affected versions

6. Fixed versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/config good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. severity/moderate sig/sql-infra SIG: SQL Infra type/bug The issue is confirmed as a bug.
Projects
None yet
6 participants