From a41b62f8139d7b80656379ddac8e2bf9fc287502 Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Mon, 29 Apr 2019 10:58:21 +0800 Subject: [PATCH 1/2] executor: fix IsPointGet judgment condition (#10278) index lookup should not use the max ts optimization --- executor/adapter.go | 16 ++++++++-------- executor/executor_test.go | 34 +++++++++++++++++++++++++++++++++- executor/point_get.go | 16 ++++++++++++++++ go.mod | 1 + go.sum | 2 ++ util/testkit/testkit.go | 7 ++++++- 6 files changed, 66 insertions(+), 10 deletions(-) diff --git a/executor/adapter.go b/executor/adapter.go index bcad6e4a9101d..41bac81170b75 100644 --- a/executor/adapter.go +++ b/executor/adapter.go @@ -309,11 +309,11 @@ func (a *ExecStmt) buildExecutor(ctx sessionctx.Context) (Executor, error) { if _, ok := a.Plan.(*plannercore.Execute); !ok { // Do not sync transaction for Execute statement, because the real optimization work is done in // "ExecuteExec.Build". - isPointGet, err := IsPointGetWithPKOrUniqueKeyByAutoCommit(ctx, a.Plan) + useMaxTS, err := IsPointGetWithPKOrUniqueKeyByAutoCommit(ctx, a.Plan) if err != nil { return nil, errors.Trace(err) } - if isPointGet { + if useMaxTS { logutil.Logger(context.Background()).Debug("init txnStartTS with MaxUint64", zap.Uint64("conn", ctx.GetSessionVars().ConnectionID), zap.String("text", a.Text)) err = ctx.InitTxnWithStartTS(math.MaxUint64) } @@ -324,7 +324,7 @@ func (a *ExecStmt) buildExecutor(ctx sessionctx.Context) (Executor, error) { stmtCtx := ctx.GetSessionVars().StmtCtx if stmtPri := stmtCtx.Priority; stmtPri == mysql.NoPriority { switch { - case isPointGet: + case useMaxTS: stmtCtx.Priority = kv.PriorityHigh case a.Expensive: stmtCtx.Priority = kv.PriorityLow @@ -461,7 +461,7 @@ func (a *ExecStmt) getStatsInfo() map[string]uint64 { // IsPointGetWithPKOrUniqueKeyByAutoCommit returns true when meets following conditions: // 1. ctx is auto commit tagged // 2. txn is not valid -// 2. plan is point get by pk or unique key +// 2. plan is point get by pk, or point get by unique index (no double read) func IsPointGetWithPKOrUniqueKeyByAutoCommit(ctx sessionctx.Context, p plannercore.Plan) (bool, error) { // check auto commit if !ctx.GetSessionVars().IsAutocommit() { @@ -489,14 +489,14 @@ func IsPointGetWithPKOrUniqueKeyByAutoCommit(ctx sessionctx.Context, p plannerco case *plannercore.PhysicalIndexReader: indexScan := v.IndexPlans[0].(*plannercore.PhysicalIndexScan) return indexScan.IsPointGetByUniqueKey(ctx.GetSessionVars().StmtCtx), nil - case *plannercore.PhysicalIndexLookUpReader: - indexScan := v.IndexPlans[0].(*plannercore.PhysicalIndexScan) - return indexScan.IsPointGetByUniqueKey(ctx.GetSessionVars().StmtCtx), nil case *plannercore.PhysicalTableReader: tableScan := v.TablePlans[0].(*plannercore.PhysicalTableScan) return len(tableScan.Ranges) == 1 && tableScan.Ranges[0].IsPoint(ctx.GetSessionVars().StmtCtx), nil case *plannercore.PointGetPlan: - return true, nil + // If the PointGetPlan needs to read data using unique index (double read), we + // can't use max uint64, because using math.MaxUint64 can't guarantee repeatable-read + // and the data and index would be inconsistent! + return v.IndexInfo == nil, nil default: return false, nil } diff --git a/executor/executor_test.go b/executor/executor_test.go index 91465619b4e18..9cb29ada42929 100644 --- a/executor/executor_test.go +++ b/executor/executor_test.go @@ -64,6 +64,7 @@ import ( "github.com/pingcap/tidb/util/testutil" "github.com/pingcap/tidb/util/timeutil" "github.com/pingcap/tipb/go-tipb" + "github.com/tiancaiamao/debugger" "go.uber.org/zap" "go.uber.org/zap/zapcore" "golang.org/x/net/context" @@ -1850,7 +1851,8 @@ func (s *testSuite) TestIsPointGet(c *C) { tk.MustExec("use mysql") ctx := tk.Se.(sessionctx.Context) tests := map[string]bool{ - "select * from help_topic where name='aaa'": true, + "select * from help_topic where name='aaa'": false, + "select 1 from help_topic where name='aaa'": true, "select * from help_topic where help_topic_id=1": true, "select * from help_topic where help_category_id=1": false, } @@ -1869,6 +1871,36 @@ func (s *testSuite) TestIsPointGet(c *C) { } } +func (s *testSuite) TestPointGetRepeatableRead(c *C) { + gofail.Enable("github.com/pingcap/tidb/executor/pointGetRepeatableReadTest1", `return(true)`) + gofail.Enable("github.com/pingcap/tidb/executor/pointGetRepeatableReadTest2", `return(true)`) + defer gofail.Disable("github.com/pingcap/tidb/executor/pointGetRepeatableReadTest1") + defer gofail.Disable("github.com/pingcap/tidb/executor/pointGetRepeatableReadTest2") + + tk1 := testkit.NewTestKit(c, s.store) + tk1.MustExec("use test") + tk1.MustExec(`create table point_get (a int, b int, c int, + primary key k_a(a), + unique key k_b(b))`) + tk1.MustExec("insert into point_get values (1, 1, 1)") + tk2 := testkit.NewTestKit(c, s.store) + tk2.MustExec("use test") + + go func() { + ctx := context.WithValue(context.Background(), "pointGetRepeatableReadTest", true) + rs, err := tk1.Se.Execute(ctx, "select c from point_get where b = 1") + c.Assert(err, IsNil) + result := tk1.ResultSetToResultWithCtx(ctx, rs[0], Commentf("execute sql fail")) + result.Check(testkit.Rows("1")) + }() + + label := debugger.Bind("point-get-g2") + debugger.Continue("point-get-g1") + debugger.Breakpoint(label) + tk2.MustExec("update point_get set b = 2, c = 2 where a = 1") + debugger.Continue("point-get-g1") +} + func (s *testSuite) TestRow(c *C) { tk := testkit.NewTestKit(c, s.store) tk.MustExec("use test") diff --git a/executor/point_get.go b/executor/point_get.go index ac1a796d335b9..673bd4555bcf6 100644 --- a/executor/point_get.go +++ b/executor/point_get.go @@ -27,6 +27,7 @@ import ( "github.com/pingcap/tidb/types" "github.com/pingcap/tidb/util/chunk" "github.com/pingcap/tidb/util/codec" + "github.com/tiancaiamao/debugger" "golang.org/x/net/context" ) @@ -88,6 +89,13 @@ func (e *PointGetExecutor) Next(ctx context.Context, chk *chunk.Chunk) error { if err1 != nil { return errors.Trace(err1) } + + // gofail: var pointGetRepeatableReadTest1 bool + // if pointGetRepeatableReadTest1 && ctx.Value("pointGetRepeatableReadTest") != nil { + // label := debugger.Bind("point-get-g1") + // debugger.Breakpoint(label) + // } + handleVal, err1 := e.get(idxKey) if err1 != nil && !kv.ErrNotExist.Equal(err1) { return errors.Trace(err1) @@ -99,7 +107,15 @@ func (e *PointGetExecutor) Next(ctx context.Context, chk *chunk.Chunk) error { if err1 != nil { return errors.Trace(err1) } + + // gofail: var pointGetRepeatableReadTest2 bool + // if pointGetRepeatableReadTest2 && ctx.Value("pointGetRepeatableReadTest") != nil { + // label := debugger.Bind("point-get-g1") + // debugger.Continue("point-get-g2") + // debugger.Breakpoint(label) + // } } + key := tablecodec.EncodeRowKeyWithHandle(e.tblInfo.ID, e.handle) val, err := e.get(key) if err != nil && !kv.ErrNotExist.Equal(err) { diff --git a/go.mod b/go.mod index f6bb042b8baba..15f312b33c6ba 100644 --- a/go.mod +++ b/go.mod @@ -59,6 +59,7 @@ require ( github.com/prometheus/procfs v0.0.0-20180408092902-8b1c2da0d56d // indirect github.com/sirupsen/logrus v0.0.0-20170323161349-3bcb09397d6d github.com/spaolacci/murmur3 v0.0.0-20150829172844-0d12bf811670 + github.com/tiancaiamao/debugger v0.0.0-20190428065433-3a10ffa41d22 github.com/twinj/uuid v0.0.0-20150629100731-70cac2bcd273 github.com/uber/jaeger-client-go v2.8.0+incompatible github.com/uber/jaeger-lib v1.1.0 // indirect diff --git a/go.sum b/go.sum index 0b226545ecf24..baf5eb047b03d 100644 --- a/go.sum +++ b/go.sum @@ -123,6 +123,8 @@ github.com/spaolacci/murmur3 v0.0.0-20150829172844-0d12bf811670/go.mod h1:JwIasO github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= +github.com/tiancaiamao/debugger v0.0.0-20190428065433-3a10ffa41d22 h1:P4sgavMKEdqNOws2VfR2c/Bye9nYFgV8gHyiW1wpQhE= +github.com/tiancaiamao/debugger v0.0.0-20190428065433-3a10ffa41d22/go.mod h1:qaShs3uDBYnvaQZJAJ6PjPg8kuAHR9zUJ8ilSLK1y/w= github.com/twinj/uuid v0.0.0-20150629100731-70cac2bcd273 h1:YqFyfcgqxQqjpRr0SEG0Z555J/3kPqDL/xmRyeAaX/0= github.com/twinj/uuid v0.0.0-20150629100731-70cac2bcd273/go.mod h1:mMgcE1RHFUFqe5AfiwlINXisXfDGro23fWdPUfOMjRY= github.com/uber/jaeger-client-go v2.8.0+incompatible h1:7DGH8Hqk6PirD+GE+bvCf0cLnspLuae7N1NcwMeQcyg= diff --git a/util/testkit/testkit.go b/util/testkit/testkit.go index cdb76a65f14b3..ca12e8a83cd3f 100644 --- a/util/testkit/testkit.go +++ b/util/testkit/testkit.go @@ -183,7 +183,12 @@ func (tk *TestKit) MustQuery(sql string, args ...interface{}) *Result { // ResultSetToResult converts sqlexec.RecordSet to testkit.Result. // It is used to check results of execute statement in binary mode. func (tk *TestKit) ResultSetToResult(rs sqlexec.RecordSet, comment check.CommentInterface) *Result { - rows, err := session.GetRows4Test(context.Background(), tk.Se, rs) + return tk.ResultSetToResultWithCtx(context.Background(), rs, comment) +} + +// ResultSetToResultWithCtx converts sqlexec.RecordSet to testkit.Result. +func (tk *TestKit) ResultSetToResultWithCtx(ctx context.Context, rs sqlexec.RecordSet, comment check.CommentInterface) *Result { + rows, err := session.GetRows4Test(ctx, tk.Se, rs) tk.c.Assert(errors.ErrorStack(err), check.Equals, "", comment) err = rs.Close() tk.c.Assert(errors.ErrorStack(err), check.Equals, "", comment) From ed8b73076468068ec0e13fdd0ec0eb0c99a5c88a Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Mon, 29 Apr 2019 11:08:21 +0800 Subject: [PATCH 2/2] make golint happy --- executor/point_get.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/executor/point_get.go b/executor/point_get.go index 673bd4555bcf6..1f906341ebf2b 100644 --- a/executor/point_get.go +++ b/executor/point_get.go @@ -72,6 +72,13 @@ func (e *PointGetExecutor) Close() error { return nil } +func dummyUseDebuggerPackageToMakeGoLintHappy() { + // debugger is import and used in gofail only + // We need to **use** it, otherwise 'make check' would complain: + // imported and not used: "github.com/tiancaiamao/debugger" + debugger.Bind("xx") +} + // Next implements the Executor interface. func (e *PointGetExecutor) Next(ctx context.Context, chk *chunk.Chunk) error { chk.Reset()