DATA RACE in the internal/unionstore.(*MemDB)

[hawkingrei]

==================
WARNING: DATA RACE
Write at 0x00c01d457a18 by goroutine 269048:
  github.com/tikv/client-go/v2/internal/unionstore.(*MemDB).allocNode()
      external/com_github_tikv_client_go_v2/internal/unionstore/memdb.go:784 +0x6b
  github.com/tikv/client-go/v2/internal/unionstore.(*MemDB).traverse()
      external/com_github_tikv_client_go_v2/internal/unionstore/memdb.go:407 +0x369
  github.com/tikv/client-go/v2/internal/unionstore.(*MemDB).set()
      external/com_github_tikv_client_go_v2/internal/unionstore/memdb.go:332 +0x1ae
  github.com/tikv/client-go/v2/internal/unionstore.(*MemDB).UpdateFlags()
      external/com_github_tikv_client_go_v2/internal/unionstore/memdb.go:247 +0x1ba6
  github.com/tikv/client-go/v2/txnkv/transaction.(*KVTxn).LockKeys()
      external/com_github_tikv_client_go_v2/txnkv/transaction/txn.go:805 +0x1ac7
  github.com/pingcap/tidb/store/driver/txn.(*tikvTxn).LockKeys()
      store/driver/txn/txn_driver.go:75 +0x105
  github.com/pingcap/tidb/session.(*LazyTxn).LockKeys()
      session/txn.go:429 +0x22e
  github.com/pingcap/tidb/executor.doLockKeys()
      executor/executor.go:1258 +0x30d
  github.com/pingcap/tidb/executor.(*SelectLockExec).Next()
      executor/executor.go:1190 +0x77e
  github.com/pingcap/tidb/executor.Next()
      executor/executor.go:324 +0x5c3
  github.com/pingcap/tidb/executor.(*UnionExec).resultPuller()
      executor/executor.go:1837 +0x8e9
  github.com/pingcap/tidb/executor.(*UnionExec).initialize.func1()
      executor/executor.go:1790 +0x64
Previous read at 0x00c01d457a18 by goroutine 269047:
  github.com/tikv/client-go/v2/internal/unionstore.(*MemDB).Size()
      external/com_github_tikv_client_go_v2/internal/unionstore/memdb.go:303 +0x97
  github.com/tikv/client-go/v2/txnkv/transaction.(*KVTxn).Size()
      external/com_github_tikv_client_go_v2/txnkv/transaction/txn.go:915 +0x52
  github.com/pingcap/tidb/store/driver/txn.(*tikvTxn).Size()
      <autogenerated>:1 +0x29
  github.com/pingcap/tidb/session.(*LazyTxn).LockKeys()
      session/txn.go:436 +0x32e
  github.com/pingcap/tidb/executor.doLockKeys()
      executor/executor.go:1258 +0x30d
  github.com/pingcap/tidb/executor.(*SelectLockExec).Next()
      executor/executor.go:1190 +0x77e
  github.com/pingcap/tidb/executor.Next()
      executor/executor.go:324 +0x5c3
  github.com/pingcap/tidb/executor.(*UnionExec).resultPuller()
      executor/executor.go:1837 +0x8e9
  github.com/pingcap/tidb/executor.(*UnionExec).initialize.func1()
      executor/executor.go:1790 +0x64
Goroutine 269048 (running) created at:
  github.com/pingcap/tidb/executor.(*UnionExec).initialize()
      executor/executor.go:1790 +0x4cf
  github.com/pingcap/tidb/executor.(*UnionExec).Next()
      executor/executor.go:1863 +0xca
  github.com/pingcap/tidb/executor.Next()
      executor/executor.go:324 +0x5c3
  github.com/pingcap/tidb/executor.(*HashAggExec).fetchChildData()
      executor/aggregate.go:813 +0x325
  github.com/pingcap/tidb/executor.(*HashAggExec).prepare4ParallelExec.func3()
      executor/aggregate.go:850 +0x64
Goroutine 269047 (finished) created at:
  github.com/pingcap/tidb/executor.(*UnionExec).initialize()
      executor/executor.go:1790 +0x4cf
  github.com/pingcap/tidb/executor.(*UnionExec).Next()
      executor/executor.go:1863 +0xca
  github.com/pingcap/tidb/executor.Next()
      executor/executor.go:324 +0x5c3
  github.com/pingcap/tidb/executor.(*HashAggExec).fetchChildData()
      executor/aggregate.go:813 +0x325
  github.com/pingcap/tidb/executor.(*HashAggExec).prepare4ParallelExec.func3()
      executor/aggregate.go:850 +0x64
================== 

[disksing]

@hawkingrei I think MemDB is not designed to allow accessing by multiple goroutines. So tidb may need some lock to avoid data race.

[sticnarf]

I think #585 should have resolved similar races that cause real problems.

I have been aware of the race in (*MemDB).Size(). I intentionally let off this race because it is only used in lock view. This race does not cause real problems and it is a bit complex to resolve it. :(

[sticnarf]

And sadly, Go doesn't provide atomic load/store in relaxed order. It will bring small performance regression if we insist on removing this unimportant race by using an atomic int.

[disksing]

@hawkingrei could you confirm if the tidb you are using contains the patch #585 ?

[hawkingrei]

@disksing I think it is used in tidb. Let me check our team's CI to find this problem again.

[hawkingrei]

@disksing This problem is still in existed.
https://prow.pingcap.net/view/gs/pingcapprow/logs/tidb_race/1587453059418558464

[disksing]

@sticnarf was the previous patch fix incomplete? I doubt if it should be fixed within client-go.

[sticnarf]

@sticnarf was the previous patch fix incomplete? I doubt if it should be fixed within client-go.

I cannot guarantee it is complete, but it should have covered the known cases.

The race reported by this issue is not a real problem and should be ignored. (Although it's truly a race)