From 26512472f7b8704eefbdd97b69903685a20b55fe Mon Sep 17 00:00:00 2001 From: lzmhhh123 Date: Wed, 16 Oct 2019 20:33:59 +0800 Subject: [PATCH 1/5] fix bug index join causes build union scan data race --- executor/union_scan.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/executor/union_scan.go b/executor/union_scan.go index 72f7dcf33c386..42af57732ddbb 100644 --- a/executor/union_scan.go +++ b/executor/union_scan.go @@ -15,6 +15,7 @@ package executor import ( "context" + "sync" "github.com/pingcap/errors" "github.com/pingcap/parser/model" @@ -28,12 +29,15 @@ 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 { + udb.Lock() dt, ok := udb.tables[tid] if !ok { dt = &DirtyTable{ @@ -43,6 +47,7 @@ func (udb *DirtyDB) GetDirtyTable(tid int64) *DirtyTable { } udb.tables[tid] = dt } + udb.Unlock() return dt } From 4d46e7127c7d02a55c956e2de06aebe66fe6f9f1 Mon Sep 17 00:00:00 2001 From: lzmhhh123 Date: Wed, 16 Oct 2019 20:39:01 +0800 Subject: [PATCH 2/5] fix format --- executor/union_scan.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/executor/union_scan.go b/executor/union_scan.go index 42af57732ddbb..b62a58ab8432e 100644 --- a/executor/union_scan.go +++ b/executor/union_scan.go @@ -30,7 +30,7 @@ import ( // 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 } From 9f083583ba71ab953c3e43a88277e8c15b54a7e1 Mon Sep 17 00:00:00 2001 From: lzmhhh123 Date: Tue, 22 Oct 2019 19:41:29 +0800 Subject: [PATCH 3/5] add comment --- executor/union_scan.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/executor/union_scan.go b/executor/union_scan.go index b62a58ab8432e..0fe3234ee7ac6 100644 --- a/executor/union_scan.go +++ b/executor/union_scan.go @@ -37,6 +37,8 @@ type DirtyDB struct { // GetDirtyTable gets the DirtyTable by id from the DirtyDB. func (udb *DirtyDB) GetDirtyTable(tid int64) *DirtyTable { + // The index join access the tables map paralelly. + // But the map throws panic in this case. So it's locked. udb.Lock() dt, ok := udb.tables[tid] if !ok { From ec16e14cbf1bce19bd319a65e1d1e47c067993f6 Mon Sep 17 00:00:00 2001 From: lzmhhh123 Date: Wed, 23 Oct 2019 10:44:37 +0800 Subject: [PATCH 4/5] fix fmt --- executor/union_scan.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/executor/union_scan.go b/executor/union_scan.go index 0fe3234ee7ac6..93fd7311930b3 100644 --- a/executor/union_scan.go +++ b/executor/union_scan.go @@ -37,7 +37,7 @@ type DirtyDB struct { // GetDirtyTable gets the DirtyTable by id from the DirtyDB. func (udb *DirtyDB) GetDirtyTable(tid int64) *DirtyTable { - // The index join access the tables map paralelly. + // The index join access the tables map paralelly. // But the map throws panic in this case. So it's locked. udb.Lock() dt, ok := udb.tables[tid] From 7135aa9b3eb152c90874418b9b76f2f711470186 Mon Sep 17 00:00:00 2001 From: lzmhhh123 Date: Wed, 23 Oct 2019 12:41:40 +0800 Subject: [PATCH 5/5] change comments in build union scan --- executor/builder.go | 5 +++-- executor/union_scan.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/executor/builder.go b/executor/builder.go index cf9669028858f..acd6e52def3e7 100644 --- a/executor/builder.go +++ b/executor/builder.go @@ -865,8 +865,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 93fd7311930b3..fe548f78d359d 100644 --- a/executor/union_scan.go +++ b/executor/union_scan.go @@ -37,7 +37,7 @@ type DirtyDB struct { // GetDirtyTable gets the DirtyTable by id from the DirtyDB. func (udb *DirtyDB) GetDirtyTable(tid int64) *DirtyTable { - // The index join access the tables map paralelly. + // 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]