Repository: tikv/client-go
Pull request #1607 (2025-03-12 08:16:06)

Validate ts only for stale read
ref https://github.com/pingcap/tidb/issues/59402
Only validate read ts for stale read and those who set `tidb_snapshot`

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/1607#pullrequestreview-2677503306" 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-12 08:18:01.235272653 +0000 UTC m=+343836.840801582`: :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/1607#pullrequestreview-2677503306" title="Approved">cfzjywxk</a>*, *<a href="https://github.com/tikv/client-go/pull/1607#pullrequestreview-2677527643" title="Approved">MyonKeminta</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)~~ [MyonKeminta,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-12 08:18:01.235272653 +0000 UTC m=+343836.840801582`: :ballot_box_with_check: agreed by [cfzjywxk](https://github.com/cfzjywxk).
- `2025-03-12 08:27:36.68504348 +0000 UTC m=+344412.290572408`: :ballot_box_with_check: agreed by [MyonKeminta](https://github.com/MyonKeminta).

Patch:
diff --git a/oracle/oracles/local.go b/oracle/oracles/local.go
index a980bbd..e5aa21f 100644
--- a/oracle/oracles/local.go
+++ b/oracle/oracles/local.go
@@ -36,11 +36,9 @@ package oracles
 
 import (
 	"context"
-	"math"
 	"sync"
 	"time"
 
-	"github.com/pingcap/errors"
 	"github.com/tikv/client-go/v2/oracle"
 )
 
@@ -152,22 +150,6 @@ func (l *localOracle) GetExternalTimestamp(ctx context.Context) (uint64, error)
 }
 
 func (l *localOracle) ValidateReadTS(ctx context.Context, readTS uint64, isStaleRead bool, opt *oracle.Option) error {
-	if readTS == math.MaxUint64 {
-		if isStaleRead {
-			return oracle.ErrLatestStaleRead{}
-		}
-		return nil
-	}
-
-	currentTS, err := l.GetTimestamp(ctx, opt)
-	if err != nil {
-		return errors.Errorf("fail to validate read timestamp: %v", err)
-	}
-	if currentTS < readTS {
-		return oracle.ErrFutureTSRead{
-			ReadTS:    readTS,
-			CurrentTS: currentTS,
-		}
-	}
+	// local oracle is not supposed to be used
 	return nil
 }
diff --git a/oracle/oracles/mock.go b/oracle/oracles/mock.go
index 757ce31..5ed8d75 100644
--- a/oracle/oracles/mock.go
+++ b/oracle/oracles/mock.go
@@ -36,7 +36,6 @@ package oracles
 
 import (
 	"context"
-	"math"
 	"sync"
 	"time"
 
@@ -139,23 +138,6 @@ func (o *MockOracle) SetLowResolutionTimestampUpdateInterval(time.Duration) erro
 }
 
 func (o *MockOracle) ValidateReadTS(ctx context.Context, readTS uint64, isStaleRead bool, opt *oracle.Option) error {
-	if readTS == math.MaxUint64 {
-		if isStaleRead {
-			return oracle.ErrLatestStaleRead{}
-		}
-		return nil
-	}
-
-	currentTS, err := o.GetTimestamp(ctx, opt)
-	if err != nil {
-		return errors.Errorf("fail to validate read timestamp: %v", err)
-	}
-	if currentTS < readTS {
-		return oracle.ErrFutureTSRead{
-			ReadTS:    readTS,
-			CurrentTS: currentTS,
-		}
-	}
 	return nil
 }
 
diff --git a/oracle/oracles/pd.go b/oracle/oracles/pd.go
index c2171c0..98813ad 100644
--- a/oracle/oracles/pd.go
+++ b/oracle/oracles/pd.go
@@ -662,7 +662,22 @@ func (o *pdOracle) getCurrentTSForValidation(ctx context.Context, opt *oracle.Op
 	}
 }
 
+// ValidateReadTSForTidbSnapshot is a flag in context, indicating whether the read ts is for tidb_snapshot.
+// This is a special approach for release branches to minimize code changes to reduce risks.
+type ValidateReadTSForTidbSnapshot struct{}
+
 func (o *pdOracle) ValidateReadTS(ctx context.Context, readTS uint64, isStaleRead bool, opt *oracle.Option) error {
+	// 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 0719c2c..46e067e 100644
--- a/internal/locate/region_request_test.go
+++ b/internal/locate/region_request_test.go
@@ -962,7 +962,8 @@ 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)
-	testImpl(func() uint64 { return addTS(getTS(), +time.Minute) }, false, oracle.ErrFutureTSRead{})
+	// check is skipped for normal read
+	testImpl(func() uint64 { return addTS(getTS(), +time.Minute) }, false, nil)
 	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 2114710..b4c4d62 100644
--- a/oracle/oracles/pd_test.go
+++ b/oracle/oracles/pd_test.go
@@ -403,7 +403,6 @@ func TestValidateReadTS(t *testing.T) {
 	}
 
 	testImpl(true)
-	testImpl(false)
 }
 
 type MockPDClientWithPause struct {
@@ -572,8 +571,9 @@ func TestValidateReadTSForNormalReadDoNotAffectUpdateInterval(t *testing.T) {
 	mustNoNotify()
 
 	// 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.Error(t, err)
+	assert.NoError(t, err)
 	mustNoNotify()
 
 	// Do the check again. It loads `ts + 5` from the mock PD, and the check passes.
@@ -618,6 +618,9 @@ func TestSetLastTSAlwaysPushTS(t *testing.T) {
 }
 
 func TestValidateReadTSFromDifferentSource(t *testing.T) {
+	// Updated: 2025-03-12, the non-stale read check is temporarily skipped.
+	t.Skip()
+
 	// 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()