search

tikv / client-go

Go client for TiKV
Apache License 2.0
282 stars 222 forks source link

membuffer: fix data race when `IsReadOnly` and Set may be concurrently invoked #1479

Open you06 opened 1 month ago

you06 commented 1 month ago

ref pingcap/tidb#56178

TiDB concurrently call IsReadOnly and some write operations in union statement, so we need to avoid data race by mutex.

ti-chi-bot[bot] commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/tikv/client-go/blob/master/OWNERS)~~ [cfzjywxk] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
ti-chi-bot[bot] commented 1 month ago

[LGTM Timeline notifier]

Timeline:

ekexium commented 1 month ago

I understand it fixes the data race. I'm just wondering whether it means we are using it correctly. What is the expected behavior when dirty is accessed concurrently?

I haven't checked the case yet but a first rough thought might be like:

Thread-1 gets dirty = false
Thread-2 writes dirty = true
Thread-1 does something that depends on dirty = false? // this can be problematic?
you06 commented 1 month ago

Is there a test case in tidb to verify the fix?

There is a test case in the issue.

you06 commented 1 month ago

I understand it fixes the data race. I'm just wondering whether it means we are using it correctly. What is the expected behavior when dirty is accessed concurrently?

I haven't checked the case yet but a first rough thought might be like:

Thread-1 gets dirty = false
Thread-2 writes dirty = true
Thread-1 does something that depends on dirty = false? // this can be problematic?

For this question, we're just avoiding the race between UpdateFlags and IsDirty, if IsDirty() == true, the result from membuffer will be not-exist error and it finally read from the store.

This is a large topic because TiDB used to pretend it's executor is running in serial mode. So there may be some hidden issues with high possibility.