Repository: tikv/client-go
Pull request #1619 (2025-03-24 08:40:55)

enable ts validation for normal read
Enable the check on the master branch

ti-chi-bot[bot]: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please ask for approval from [ekexium](https://github.com/ekexium), **ensuring that each of them provides their approval before proceeding**. For more information see [the Code Review Process](https://book.prow.tidb.net/#/workflows/pr).

The full list of commands accepted by this bot can be found [here](https://prow.tidb.net/command-help?repo=tikv%2Fclient-go).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/tikv/client-go/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["ekexium"]} -->
ti-chi-bot[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/tikv/client-go/pull/1619#pullrequestreview-2712945901" title="Approved">cfzjywxk</a>*

The full list of commands accepted by this bot can be found [here](https://prow.tidb.net/command-help?repo=tikv%2Fclient-go).

The pull request process is described [here](https://book.prow.tidb.net/#/workflows/pr)

<details >
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
</details>
<!-- META={"approvers":[]} -->
ti-chi-bot[bot]: [LGTM Timeline notifier]
---
Timeline:

- `2025-03-25 09:04:15.127900615 +0000 UTC m=+951148.812136713`: :ballot_box_with_check: agreed by [cfzjywxk](https://github.com/cfzjywxk).
cfzjywxk: > Should we introduce an "escape route" in case users encounter related issues after upgrading to the new version?

Currently, it is enabled experimentally on the master branch, and it may be reverted if any issues arise. 
Based on previous lessons, it is crucial to avoid false positive in production environments, as even with workarounds, they have already caused service unavailability.

The challendge is it's difficult to verify no false positive would happen.
zyguan: > > Should we introduce an "escape route" in case users encounter related issues after upgrading to the new version?
> 
> Currently, it is enabled experimentally on the master branch, and it may be reverted if any issues arise. Based on previous lessons, it is crucial to avoid false positive in production environments, as even with workarounds, they have already caused service unavailability.
> 
> The challendge is it's difficult to verify no false positive would happen.

I known it's difficult to verify no false positive would happen. What if there is an unfound false positive which causes service unavailability in the customer‘s production environment?
- if there is no "escape route", we have to urgently release a version with the bug fix.
- if there is a switch to disable the validation, we only need to inform the customer to disable this feature.

IMO, an even better approach is to keep this feature disabled by default for upgraded clusters.
cfzjywxk: Makes sense,  it is still needed to introduce the relevant configuration option or system variable, and as @zyguan  mentioned, it should only take effect for newly created clusters. Perhaps this is the safest way to enable the check.
ekexium: If we add a switch to this, what should be the behvaior when the switch is off? Skip all checks or still check stale reads?
you06: > If we add a switch to this, what should be the behvaior when the switch is off? Skip all checks or still check stale reads?

I prefer skipping checks than logging the violations. Because the logs may be ignored, and too many logs can be another serious issue.
cfzjywxk: > > If we add a switch to this, what should be the behvaior when the switch is off? Skip all checks or still check stale reads?
> 
> I prefer skipping checks than logging the violations. Because the logs may be ignored, and too many logs can be another serious issue.

The `stale read` check is already enabled in the LTS version with error logging, the behaviour of the new switch needs some considerations. Disabling for all is a more common approach.
ti-chi-bot[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/tikv/client-go/pull/1619#pullrequestreview-2712945901" title="Approved">cfzjywxk</a>*, *<a href="https://github.com/tikv/client-go/pull/1619#pullrequestreview-2776006335" title="Approved">zyguan</a>*

The full list of commands accepted by this bot can be found [here](https://prow.tidb.net/command-help?repo=tikv%2Fclient-go).

The pull request process is described [here](https://book.prow.tidb.net/#/workflows/pr)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/tikv/client-go/blob/master/OWNERS)~~ [cfzjywxk,zyguan]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
ti-chi-bot[bot]: [LGTM Timeline notifier]
---
Timeline:

- `2025-03-25 09:04:15.127900615 +0000 UTC m=+951148.812136713`: :ballot_box_with_check: agreed by [cfzjywxk](https://github.com/cfzjywxk).
- `2025-04-17 14:35:14.951396572 +0000 UTC m=+2958208.635632668`: :ballot_box_with_check: agreed by [zyguan](https://github.com/zyguan).
ti-chi-bot[bot]: New changes are detected. LGTM label has been removed.
ti-chi-bot[bot]: [LGTM Timeline notifier]
---
Timeline:

- `2025-03-25 09:04:15.127900615 +0000 UTC m=+951148.812136713`: :ballot_box_with_check: agreed by [cfzjywxk](https://github.com/cfzjywxk).
- `2025-04-17 14:35:14.951396572 +0000 UTC m=+2958208.635632668`: :ballot_box_with_check: agreed by [zyguan](https://github.com/zyguan).
- `2025-04-25 03:59:15.945923527 +0000 UTC m=+587299.757713910`: :heavy_multiplication_x::repeat: reset by [ekexium](https://github.com/ekexium).
ti-chi-bot[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/tikv/client-go/pull/1619#pullrequestreview-2793117661" title="Approved">cfzjywxk</a>*, *<a href="https://github.com/tikv/client-go/pull/1619#pullrequestreview-2776006335" title="Approved">zyguan</a>*

The full list of commands accepted by this bot can be found [here](https://prow.tidb.net/command-help?repo=tikv%2Fclient-go).

The pull request process is described [here](https://book.prow.tidb.net/#/workflows/pr)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/tikv/client-go/blob/master/OWNERS)~~ [cfzjywxk,zyguan]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
ti-chi-bot[bot]: [LGTM Timeline notifier]
---
Timeline:

- `2025-03-25 09:04:15.127900615 +0000 UTC m=+951148.812136713`: :ballot_box_with_check: agreed by [cfzjywxk](https://github.com/cfzjywxk).
- `2025-04-17 14:35:14.951396572 +0000 UTC m=+2958208.635632668`: :ballot_box_with_check: agreed by [zyguan](https://github.com/zyguan).
- `2025-04-25 03:59:15.945923527 +0000 UTC m=+587299.757713910`: :heavy_multiplication_x::repeat: reset by [ekexium](https://github.com/ekexium).
- `2025-04-25 06:29:39.716934822 +0000 UTC m=+596323.528725198`: :ballot_box_with_check: agreed by [cfzjywxk](https://github.com/cfzjywxk).
ti-chi-bot[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/tikv/client-go/pull/1619#pullrequestreview-2793117661" title="Approved">cfzjywxk</a>*, *<a href="https://github.com/tikv/client-go/pull/1619#pullrequestreview-2793437499" title="Approved">zyguan</a>*

The full list of commands accepted by this bot can be found [here](https://prow.tidb.net/command-help?repo=tikv%2Fclient-go).

The pull request process is described [here](https://book.prow.tidb.net/#/workflows/pr)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/tikv/client-go/blob/master/OWNERS)~~ [cfzjywxk,zyguan]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
ti-chi-bot[bot]: [LGTM Timeline notifier]
---
Timeline:

- `2025-03-25 09:04:15.127900615 +0000 UTC m=+951148.812136713`: :ballot_box_with_check: agreed by [cfzjywxk](https://github.com/cfzjywxk).
- `2025-04-17 14:35:14.951396572 +0000 UTC m=+2958208.635632668`: :ballot_box_with_check: agreed by [zyguan](https://github.com/zyguan).
- `2025-04-25 03:59:15.945923527 +0000 UTC m=+587299.757713910`: :heavy_multiplication_x::repeat: reset by [ekexium](https://github.com/ekexium).
- `2025-04-25 06:29:39.716934822 +0000 UTC m=+596323.528725198`: :ballot_box_with_check: agreed by [cfzjywxk](https://github.com/cfzjywxk).
- `2025-04-25 08:52:33.481919747 +0000 UTC m=+604897.293710129`: :ballot_box_with_check: agreed by [zyguan](https://github.com/zyguan).

Patch:
diff --git a/oracle/oracles/pd.go b/oracle/oracles/pd.go
index 98813ad..1671c4d 100644
--- a/oracle/oracles/pd.go
+++ b/oracle/oracles/pd.go
@@ -60,6 +60,8 @@ const slowDist = 30 * time.Millisecond
 
 type adaptiveUpdateTSIntervalState int
 
+var EnableTSValidation atomic.Bool
+
 const (
 	adaptiveUpdateTSIntervalStateNone adaptiveUpdateTSIntervalState = iota
 	// adaptiveUpdateTSIntervalStateNormal represents the state that the adaptive update ts interval is synced with the
@@ -667,17 +669,15 @@ func (o *pdOracle) getCurrentTSForValidation(ctx context.Context, opt *oracle.Op
 type ValidateReadTSForTidbSnapshot struct{}
 
 func (o *pdOracle) ValidateReadTS(ctx context.Context, readTS uint64, isStaleRead bool, opt *oracle.Option) error {
+	if !EnableTSValidation.Load() {
+		return nil
+	}
+
 	// For a mistake we've seen
 	if readTS >= math.MaxInt64 && readTS < math.MaxUint64 {
 		return errors.Errorf("MaxInt64 <= readTS < MaxUint64, readTS=%v", readTS)
 	}
 
-	// only check stale reads and reads using `tidb_snapshot`
-	forTidbSnapshot := ctx.Value(ValidateReadTSForTidbSnapshot{}) != nil
-	if !forTidbSnapshot && !isStaleRead {
-		return nil
-	}
-
 	if readTS == math.MaxUint64 {
 		if isStaleRead {
 			return oracle.ErrLatestStaleRead{}


Test patch:
diff --git a/internal/locate/region_request_test.go b/internal/locate/region_request_test.go
index 46e067e..117ff49 100644
--- a/internal/locate/region_request_test.go
+++ b/internal/locate/region_request_test.go
@@ -920,6 +920,8 @@ func (s *testRegionRequestToSingleStoreSuite) TestRegionRequestStats() {
 }
 
 func (s *testRegionRequestToSingleStoreSuite) TestRegionRequestValidateReadTS() {
+	oracles.EnableTSValidation.Store(true)
+	defer oracles.EnableTSValidation.Store(false)
 	o, err := oracles.NewPdOracle(s.pdCli, &oracles.PDOracleOptions{
 		UpdateInterval: time.Second * 2,
 	})
@@ -962,8 +964,7 @@ func (s *testRegionRequestToSingleStoreSuite) TestRegionRequestValidateReadTS()
 	testImpl(getTS, true, nil)
 	testImpl(func() uint64 { return addTS(getTS(), -time.Minute) }, false, nil)
 	testImpl(func() uint64 { return addTS(getTS(), -time.Minute) }, true, nil)
-	// check is skipped for normal read
-	testImpl(func() uint64 { return addTS(getTS(), +time.Minute) }, false, nil)
+	testImpl(func() uint64 { return addTS(getTS(), +time.Minute) }, false, oracle.ErrFutureTSRead{})
 	testImpl(func() uint64 { return addTS(getTS(), +time.Minute) }, true, oracle.ErrFutureTSRead{})
 	testImpl(func() uint64 { return math.MaxUint64 }, false, nil)
 	testImpl(func() uint64 { return math.MaxUint64 }, true, oracle.ErrLatestStaleRead{})
diff --git a/oracle/oracles/pd_test.go b/oracle/oracles/pd_test.go
index b4c4d62..6ae6aad 100644
--- a/oracle/oracles/pd_test.go
+++ b/oracle/oracles/pd_test.go
@@ -352,6 +352,8 @@ func TestAdaptiveUpdateTSInterval(t *testing.T) {
 }
 
 func TestValidateReadTS(t *testing.T) {
+	EnableTSValidation.Store(true)
+	defer EnableTSValidation.Store(false)
 	testImpl := func(staleRead bool) {
 		pdClient := MockPdClient{}
 		o, err := NewPdOracle(&pdClient, &PDOracleOptions{
@@ -429,6 +431,8 @@ func (c *MockPDClientWithPause) WithCallerComponent(component caller.Component)
 }
 
 func TestValidateReadTSForStaleReadReusingGetTSResult(t *testing.T) {
+	EnableTSValidation.Store(true)
+	defer EnableTSValidation.Store(false)
 	util.EnableFailpoints()
 
 	pdClient := &MockPDClientWithPause{}
@@ -536,6 +540,8 @@ func TestValidateReadTSForStaleReadReusingGetTSResult(t *testing.T) {
 }
 
 func TestValidateReadTSForNormalReadDoNotAffectUpdateInterval(t *testing.T) {
+	EnableTSValidation.Store(true)
+	defer EnableTSValidation.Store(false)
 	oracleInterface, err := NewPdOracle(&MockPdClient{}, &PDOracleOptions{
 		UpdateInterval: time.Second * 2,
 		NoUpdateTS:     true,
@@ -573,7 +579,7 @@ func TestValidateReadTSForNormalReadDoNotAffectUpdateInterval(t *testing.T) {
 	// It loads `ts + 3` and `ts + 4` from the mock PD, and the check cannot pass.
 	// Updated: 2025-03-12, the non-stale read check is temporarily skipped.
 	err = o.ValidateReadTS(ctx, ts+5, false, opt)
-	assert.NoError(t, err)
+	assert.Error(t, err)
 	mustNoNotify()
 
 	// Do the check again. It loads `ts + 5` from the mock PD, and the check passes.
@@ -618,9 +624,8 @@ func TestSetLastTSAlwaysPushTS(t *testing.T) {
 }
 
 func TestValidateReadTSFromDifferentSource(t *testing.T) {
-	// Updated: 2025-03-12, the non-stale read check is temporarily skipped.
-	t.Skip()
-
+	EnableTSValidation.Store(true)
+	defer EnableTSValidation.Store(false)
 	// If a ts is fetched from a different client to the same cluster, the ts might not be cached by the low resolution
 	// ts. In this case, the validation should not be false positive.
 	util.EnableFailpoints()