From 41c9073521cebc5bff5a8a2fd74b613e6237701c Mon Sep 17 00:00:00 2001 From: Yu Shuaipeng Date: Thu, 14 Mar 2019 14:30:55 +0800 Subject: [PATCH 1/3] fix read only check for statement --- executor/adapter.go | 15 +++++++++------ executor/executor.go | 3 +++ session/session.go | 3 --- session/tidb.go | 6 +++++- util/sqlexec/restricted_sql_executor.go | 3 ++- 5 files changed, 19 insertions(+), 11 deletions(-) diff --git a/executor/adapter.go b/executor/adapter.go index 6a8443ddbd81d..331a4cbd58580 100644 --- a/executor/adapter.go +++ b/executor/adapter.go @@ -166,15 +166,18 @@ func (a *ExecStmt) IsPrepared() bool { } // IsReadOnly returns true if a statement is read only. -// It will update readOnlyCheckStmt if current ExecStmt can be conveted to +// It will update readOnlyCheckStmt if current ExecStmt can be converted to // a plannercore.Execute. Last step is using ast.IsReadOnly function to determine // a statement is read only or not. -func (a *ExecStmt) IsReadOnly() bool { - readOnlyCheckStmt := a.StmtNode - if checkPlan, ok := a.Plan.(*plannercore.Execute); ok { - readOnlyCheckStmt = checkPlan.Stmt +func (a *ExecStmt) IsReadOnly(vars *variable.SessionVars) (bool, error) { + if execStmt, ok := a.StmtNode.(*ast.ExecuteStmt); ok { + s, err := getPreparedStmt(execStmt, vars) + if err != nil { + return false, err + } + return ast.IsReadOnly(s), nil } - return ast.IsReadOnly(readOnlyCheckStmt) + return ast.IsReadOnly(a.StmtNode), nil } // RebuildPlan rebuilds current execute statement plan. diff --git a/executor/executor.go b/executor/executor.go index b630550351226..34f987a37a7df 100644 --- a/executor/executor.go +++ b/executor/executor.go @@ -1304,6 +1304,9 @@ func ResetContextOfStmt(ctx sessionctx.Context, s ast.StmtNode) (err error) { if execStmt, ok := s.(*ast.ExecuteStmt); ok { s, err = getPreparedStmt(execStmt, vars) + if err != nil { + return + } } // TODO: Many same bool variables here. // We should set only two variables ( diff --git a/session/session.go b/session/session.go index 489990bb6f2d9..6508e1c2d8384 100644 --- a/session/session.go +++ b/session/session.go @@ -565,9 +565,6 @@ func (s *session) retry(ctx context.Context, maxCnt uint) (err error) { s.sessionVars.RetryInfo.ResetOffset() for i, sr := range nh.history { st := sr.st - if st.IsReadOnly() { - continue - } s.sessionVars.StmtCtx = sr.stmtCtx s.sessionVars.StmtCtx.ResetForRetry() s.sessionVars.PreparedParams = s.sessionVars.PreparedParams[:0] diff --git a/session/tidb.go b/session/tidb.go index 7e7e93465fa0c..dbdaf0b4de4fc 100644 --- a/session/tidb.go +++ b/session/tidb.go @@ -214,7 +214,11 @@ func runStmt(ctx context.Context, sctx sessionctx.Context, s sqlexec.Statement) sessVars := se.GetSessionVars() // All the history should be added here. sessVars.TxnCtx.StatementCount++ - if !s.IsReadOnly() { + isReadOnly, err := s.IsReadOnly(sessVars) + if err != nil { + return nil, err + } + if !isReadOnly { if err == nil { GetHistory(sctx).Add(0, s, se.sessionVars.StmtCtx) } diff --git a/util/sqlexec/restricted_sql_executor.go b/util/sqlexec/restricted_sql_executor.go index 02ef84a95d89f..7b45173f3dba1 100644 --- a/util/sqlexec/restricted_sql_executor.go +++ b/util/sqlexec/restricted_sql_executor.go @@ -18,6 +18,7 @@ import ( "github.com/pingcap/parser/ast" "github.com/pingcap/tidb/sessionctx" + "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/util/chunk" ) @@ -74,7 +75,7 @@ type Statement interface { IsPrepared() bool // IsReadOnly returns if the statement is read only. For example: SelectStmt without lock. - IsReadOnly() bool + IsReadOnly(vars *variable.SessionVars) (bool, error) // RebuildPlan rebuilds the plan of the statement. RebuildPlan() (schemaVersion int64, err error) From 6f7d3815cdf5b4ee0dc0cbbe40b75429afa058b2 Mon Sep 17 00:00:00 2001 From: Yu Shuaipeng Date: Thu, 14 Mar 2019 14:37:56 +0800 Subject: [PATCH 2/3] tiny fix --- session/tidb.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/session/tidb.go b/session/tidb.go index dbdaf0b4de4fc..44d8797f494a1 100644 --- a/session/tidb.go +++ b/session/tidb.go @@ -214,9 +214,9 @@ func runStmt(ctx context.Context, sctx sessionctx.Context, s sqlexec.Statement) sessVars := se.GetSessionVars() // All the history should be added here. sessVars.TxnCtx.StatementCount++ - isReadOnly, err := s.IsReadOnly(sessVars) - if err != nil { - return nil, err + isReadOnly, errNotFound := s.IsReadOnly(sessVars) + if errNotFound != nil { + log.Error(errNotFound) } if !isReadOnly { if err == nil { From 9892114b9d7d971b7c231c30db4fb9abc85bc29f Mon Sep 17 00:00:00 2001 From: Yu Shuaipeng Date: Mon, 25 Mar 2019 19:49:31 +0800 Subject: [PATCH 3/3] add test case --- executor/adapter.go | 14 +++++++------- go.mod | 1 - go.sum | 2 -- session/session_test.go | 19 +++++++++++++++++++ session/tidb.go | 6 +----- util/sqlexec/restricted_sql_executor.go | 2 +- 6 files changed, 28 insertions(+), 16 deletions(-) diff --git a/executor/adapter.go b/executor/adapter.go index 331a4cbd58580..0ded0dd1dd5b0 100644 --- a/executor/adapter.go +++ b/executor/adapter.go @@ -166,18 +166,18 @@ func (a *ExecStmt) IsPrepared() bool { } // IsReadOnly returns true if a statement is read only. -// It will update readOnlyCheckStmt if current ExecStmt can be converted to -// a plannercore.Execute. Last step is using ast.IsReadOnly function to determine -// a statement is read only or not. -func (a *ExecStmt) IsReadOnly(vars *variable.SessionVars) (bool, error) { +// If current StmtNode is an ExecuteStmt, we can get its prepared stmt, +// then using ast.IsReadOnly function to determine a statement is read only or not. +func (a *ExecStmt) IsReadOnly(vars *variable.SessionVars) bool { if execStmt, ok := a.StmtNode.(*ast.ExecuteStmt); ok { s, err := getPreparedStmt(execStmt, vars) if err != nil { - return false, err + logutil.Logger(context.Background()).Error("getPreparedStmt failed", zap.Error(err)) + return false } - return ast.IsReadOnly(s), nil + return ast.IsReadOnly(s) } - return ast.IsReadOnly(a.StmtNode), nil + return ast.IsReadOnly(a.StmtNode) } // RebuildPlan rebuilds current execute statement plan. diff --git a/go.mod b/go.mod index f46ff3a30c1a2..f866418b4ad23 100644 --- a/go.mod +++ b/go.mod @@ -87,7 +87,6 @@ require ( google.golang.org/genproto v0.0.0-20190108161440-ae2f86662275 // indirect google.golang.org/grpc v1.17.0 gopkg.in/natefinch/lumberjack.v2 v2.0.0 - gopkg.in/stretchr/testify.v1 v1.2.2 // indirect sourcegraph.com/sourcegraph/appdash v0.0.0-20180531100431-4c381bd170b4 sourcegraph.com/sourcegraph/appdash-data v0.0.0-20151005221446-73f23eafcf67 ) diff --git a/go.sum b/go.sum index d483098e0b9d0..6136494fb52ed 100644 --- a/go.sum +++ b/go.sum @@ -229,8 +229,6 @@ gopkg.in/fsnotify.v1 v1.4.7 h1:xOHLXZwVvI9hhs+cLKq5+I5onOuwQLhQwiu63xxlHs4= gopkg.in/fsnotify.v1 v1.4.7/go.mod h1:Tz8NjZHkW78fSQdbUxIjBTcgA1z1m8ZHf0WmKUhAMys= gopkg.in/natefinch/lumberjack.v2 v2.0.0 h1:1Lc07Kr7qY4U2YPouBjpCLxpiyxIVoxqXgkXLknAOE8= gopkg.in/natefinch/lumberjack.v2 v2.0.0/go.mod h1:l0ndWWf7gzL7RNwBG7wST/UCcT4T24xpD6X8LsfU/+k= -gopkg.in/stretchr/testify.v1 v1.2.2 h1:yhQC6Uy5CqibAIlk1wlusa/MJ3iAN49/BsR/dCCKz3M= -gopkg.in/stretchr/testify.v1 v1.2.2/go.mod h1:QI5V/q6UbPmuhtm10CaFZxED9NreB8PnFYN9JcR6TxU= gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 h1:uRGJdciOHaEIrze2W8Q3AKkepLTh2hOroT7a+7czfdQ= gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7/go.mod h1:dt/ZhP58zS4L8KSrWDmTeBkI65Dw0HsyUHuEVlX15mw= gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= diff --git a/session/session_test.go b/session/session_test.go index b22aadc635a9b..aef60fbe039e5 100644 --- a/session/session_test.go +++ b/session/session_test.go @@ -968,6 +968,25 @@ func (s *testSessionSuite) TestAutoIncrementWithRetry(c *C) { c.Assert(lastInsertID+3, Equals, currLastInsertID) } +func (s *testSessionSuite) TestBinaryReadOnly(c *C) { + tk := testkit.NewTestKitWithInit(c, s.store) + tk.MustExec("create table t (i int key)") + id, _, _, err := tk.Se.PrepareStmt("select i from t where i = ?") + c.Assert(err, IsNil) + id2, _, _, err := tk.Se.PrepareStmt("insert into t values (?)") + c.Assert(err, IsNil) + tk.MustExec("set autocommit = 0") + _, err = tk.Se.ExecutePreparedStmt(context.Background(), id, 1) + c.Assert(err, IsNil) + c.Assert(session.GetHistory(tk.Se).Count(), Equals, 0) + tk.MustExec("insert into t values (1)") + c.Assert(session.GetHistory(tk.Se).Count(), Equals, 1) + _, err = tk.Se.ExecutePreparedStmt(context.Background(), id2, 2) + c.Assert(err, IsNil) + c.Assert(session.GetHistory(tk.Se).Count(), Equals, 2) + tk.MustExec("commit") +} + func (s *testSessionSuite) TestPrepare(c *C) { tk := testkit.NewTestKitWithInit(c, s.store) tk.MustExec("create table t(id TEXT)") diff --git a/session/tidb.go b/session/tidb.go index 44d8797f494a1..e3ed676d5b1c9 100644 --- a/session/tidb.go +++ b/session/tidb.go @@ -214,11 +214,7 @@ func runStmt(ctx context.Context, sctx sessionctx.Context, s sqlexec.Statement) sessVars := se.GetSessionVars() // All the history should be added here. sessVars.TxnCtx.StatementCount++ - isReadOnly, errNotFound := s.IsReadOnly(sessVars) - if errNotFound != nil { - log.Error(errNotFound) - } - if !isReadOnly { + if !s.IsReadOnly(sessVars) { if err == nil { GetHistory(sctx).Add(0, s, se.sessionVars.StmtCtx) } diff --git a/util/sqlexec/restricted_sql_executor.go b/util/sqlexec/restricted_sql_executor.go index 7b45173f3dba1..70923acc09288 100644 --- a/util/sqlexec/restricted_sql_executor.go +++ b/util/sqlexec/restricted_sql_executor.go @@ -75,7 +75,7 @@ type Statement interface { IsPrepared() bool // IsReadOnly returns if the statement is read only. For example: SelectStmt without lock. - IsReadOnly(vars *variable.SessionVars) (bool, error) + IsReadOnly(vars *variable.SessionVars) bool // RebuildPlan rebuilds the plan of the statement. RebuildPlan() (schemaVersion int64, err error)