pingcap / tidb

TiDB - the open-source, cloud-native, distributed SQL database designed for modern applications.
https://pingcap.com
Apache License 2.0
37.28k stars 5.84k forks source link

Some read requests do not honor the store labels #45693

Open zyguan opened 1 year ago

zyguan commented 1 year ago

Bug Report

TiDB only set MatchStoreLabels on executorBuilder.getSnapshot, however some of executors may use txn.{Get,BatchGet} directly. These reads do not honor the store labels thus may access replicas on the slow tikv.

1. Minimal reproduce step (Required)

Deploy a cluster with 3 AZs and evict-slow-store-scheduler, run a stale read workload, and then slowdown a tikv in one AZ.

2. What did you expect to see? (Required)

The other two AZs should not be affected after leaders have been evicted.

3. What did you see instead (Required)

All AZs had perf regression during the fault injection.

4. What is your TiDB version? (Required)

release-6.5

MyonKeminta commented 1 year ago

There are many calls to txn.{Get,BatchGet} that spreads everywhere. Including:

  1. DML / select for update executors, for example: https://github.com/pingcap/tidb/blob/57d778f0b1b6daebe0f330af2baa54e9e5310ea4/executor/replace.go#L76
  2. Temp table / temp index, for example: https://github.com/pingcap/tidb/blob/57d778f0b1b6daebe0f330af2baa54e9e5310ea4/ddl/index_merge_tmp.go#L93
  3. Write operations, for example: https://github.com/pingcap/tidb/blob/57d778f0b1b6daebe0f330af2baa54e9e5310ea4/table/tables/index.go#L287
  4. Foreign key checks, for example: https://github.com/pingcap/tidb/blob/57d778f0b1b6daebe0f330af2baa54e9e5310ea4/executor/foreign_key.go#L336

It seems that they are all in write operations. Those readonly operations doesn't seem to be affected.


To fix the problem, we have multiple choices:

  1. The first solution we considered is to disable replica read for read operations in write statements. However, since the current implementation has been released in several versions, changing the behavior may affect users that already benefits from replica read in write statements. We need to confirm the impact.
  2. Make all those reads affected by replica read and MatchStoreLabels. This makes all these read able to benefit from replica read, however it makes the write path more complicated, increasing the difficulty to diagnose performance issues. Also, we used to meet such issue that replica read in write statements actually slows the statement down when there are faults in the cluster. As the related code spreads everywhere as we listed above, the difficulty to change the code is also high.
  3. Add a variable/configuration to control whether replica read in write statements are allowed. This is most complicated to implementation, and we need PMs' opinion about whether adding this configuration is acceptable.
crazycs520 commented 1 year ago

Configurable KV Timeout also has this issue too. Such as tidb_kv_read_timeout variable doesn't take effect on insert ignore statement when doing unique check.