From 75946ff972adc1fb8629048b85d9c3a381b79717 Mon Sep 17 00:00:00 2001 From: "Zhuomin(Charming) Liu" Date: Wed, 23 Oct 2019 13:57:30 +0800 Subject: [PATCH] executor: fix data race in `GetDirtyTable()` (#12767) --- executor/builder.go | 5 +++-- executor/union_scan.go | 7 +++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/executor/builder.go b/executor/builder.go index 35e0b969fd229..039038ab86f4d 100644 --- a/executor/builder.go +++ b/executor/builder.go @@ -848,8 +848,9 @@ func (b *executorBuilder) buildUnionScanFromReader(reader Executor, v *plannerco // GetDirtyDB() is safe here. If this table has been modified in the transaction, non-nil DirtyTable // can be found in DirtyDB now, so GetDirtyTable is safe; if this table has not been modified in the // transaction, empty DirtyTable would be inserted into DirtyDB, it does not matter when multiple - // goroutines write empty DirtyTable to DirtyDB for this table concurrently. Thus we don't use lock - // to synchronize here. + // goroutines write empty DirtyTable to DirtyDB for this table concurrently. Although the DirtyDB looks + // safe for data race in all the cases, the map of golang will throw panic when it's accessed in parallel. + // So we lock it when getting dirty table. physicalTableID := getPhysicalTableID(x.table) us.dirty = GetDirtyDB(b.ctx).GetDirtyTable(physicalTableID) us.conditions = v.Conditions diff --git a/executor/union_scan.go b/executor/union_scan.go index d9e9bf8d5d0be..0098fe99797bd 100644 --- a/executor/union_scan.go +++ b/executor/union_scan.go @@ -16,6 +16,7 @@ package executor import ( "context" "sort" + "sync" "time" "github.com/opentracing/opentracing-go" @@ -32,12 +33,17 @@ import ( // DirtyDB stores uncommitted write operations for a transaction. // It is stored and retrieved by context.Value and context.SetValue method. type DirtyDB struct { + sync.Mutex + // tables is a map whose key is tableID. tables map[int64]*DirtyTable } // GetDirtyTable gets the DirtyTable by id from the DirtyDB. func (udb *DirtyDB) GetDirtyTable(tid int64) *DirtyTable { + // The index join access the tables map parallelly. + // But the map throws panic in this case. So it's locked. + udb.Lock() dt, ok := udb.tables[tid] if !ok { dt = &DirtyTable{ @@ -47,6 +53,7 @@ func (udb *DirtyDB) GetDirtyTable(tid int64) *DirtyTable { } udb.tables[tid] = dt } + udb.Unlock() return dt }