Repository: tikv/client-go (go)

Issue:
NotLeader error may cause unnecessary backoff by causing pseudo region error
When a `NotLeader` error carrying a new leader info is received from TiKV, we update the leader info of the region by updating the struct `Region` 's `store` field which is in type `RegionStore`). As the `Region` is a type of shared data managed by `RegionCache`, the `store` is updated by replacing atomic pointers, and thus `RegionStore` is used in copy-on-write pattern.

https://github.com/tikv/client-go/blob/abda04c22acf06d9922fbd20c5dc82b1764b62d0/internal/locate/region_cache.go#L2451-L2454

Then, #264 introduces this which is executed every time attempting to send a request:

https://github.com/tikv/client-go/blob/abda04c22acf06d9922fbd20c5dc82b1764b62d0/internal/locate/region_request.go#L1023-L1026

This forces the replicaSelector to be rebuilt when retrying on `NotLeader` error, by return nil rpcCtx:

https://github.com/tikv/client-go/blob/abda04c22acf06d9922fbd20c5dc82b1764b62d0/internal/locate/region_request.go#L894-L897

and then triggering pseudo region error:

https://github.com/tikv/client-go/blob/abda04c22acf06d9922fbd20c5dc82b1764b62d0/internal/locate/region_request.go#L1424-L1435

This will cause backoff to the caller of `SendReqCtx`, which is unnecessary in a `NotLeader` case where the new leader is immediately known.

Pull request description:
Fix the issue that leader change with nea leader info cause invalidStore state and lead to unnecessary backoff
Fixes #1112

Patch:
diff --git a/internal/locate/region_request.go b/internal/locate/region_request.go
index f56ae32..d149598 100644
--- a/internal/locate/region_request.go
+++ b/internal/locate/region_request.go
@@ -45,6 +45,7 @@ import (
 	"sync"
 	"sync/atomic"
 	"time"
+	"unsafe"
 
 	"go.uber.org/zap"
 	"google.golang.org/grpc/codes"
@@ -1008,6 +1009,12 @@ func (s *replicaSelector) proxyReplica() *replica {
 	return nil
 }
 
+// sliceIdentical checks whether two slices are referencing the same block of memory. Two `nil`s are also considered
+// the same.
+func sliceIdentical[T any](a, b []T) bool {
+	return len(a) == len(b) && unsafe.SliceData(a) == unsafe.SliceData(b)
+}
+
 func (s *replicaSelector) refreshRegionStore() {
 	oldRegionStore := s.regionStore
 	newRegionStore := s.region.getStore()
@@ -1020,7 +1027,7 @@ func (s *replicaSelector) refreshRegionStore() {
 	// So we just compare the address here.
 	// When stores change, we mark this replicaSelector as invalid to let the caller
 	// recreate a new replicaSelector.
-	if &oldRegionStore.stores != &newRegionStore.stores {
+	if !sliceIdentical(oldRegionStore.stores, newRegionStore.stores) {
 		s.state = &invalidStore{}
 		return
 	}


Test patch:
diff --git a/internal/locate/region_request3_test.go b/internal/locate/region_request3_test.go
index b5486bc..9e5ce29 100644
--- a/internal/locate/region_request3_test.go
+++ b/internal/locate/region_request3_test.go
@@ -137,6 +137,81 @@ func (s *testRegionRequestToThreeStoresSuite) TestSwitchPeerWhenNoLeader() {
 	resp, _, err := s.regionRequestSender.SendReq(bo, req, loc.Region, time.Second)
 	s.Nil(err)
 	s.NotNil(resp)
+	s.Nil(resp.GetRegionError())
+}
+
+func (s *testRegionRequestToThreeStoresSuite) TestSwitchPeerWhenNoLeaderErrorWithNewLeaderInfo() {
+	cnt := 0
+	var location *KeyLocation
+	cli := &fnClient{fn: func(ctx context.Context, addr string, req *tikvrpc.Request, timeout time.Duration) (response *tikvrpc.Response, err error) {
+		cnt++
+		switch cnt {
+		case 1:
+			region := s.cache.GetCachedRegionWithRLock(location.Region)
+			s.NotNil(region)
+			leaderPeerIdx := int(region.getStore().workTiKVIdx)
+			peers := region.meta.Peers
+			// return no leader with new leader info
+			response = &tikvrpc.Response{Resp: &kvrpcpb.GetResponse{
+				RegionError: &errorpb.Error{NotLeader: &errorpb.NotLeader{
+					RegionId: req.RegionId,
+					Leader:   peers[(leaderPeerIdx+1)%len(peers)],
+				}},
+			}}
+		case 2:
+			response = &tikvrpc.Response{Resp: &kvrpcpb.GetResponse{
+				Value: []byte("a"),
+			}}
+		default:
+			return nil, fmt.Errorf("unexpected request")
+		}
+		return response, err
+	}}
+
+	req := tikvrpc.NewRequest(tikvrpc.CmdGet, &kvrpcpb.GetRequest{Key: []byte("a")}, kvrpcpb.Context{})
+	req.ReplicaReadType = kv.ReplicaReadLeader
+	var err error
+	location, err = s.cache.LocateKey(s.bo, []byte("a"))
+	s.Nil(err)
+	s.NotNil(location)
+	bo := retry.NewBackoffer(context.Background(), 1000)
+	resp, _, _, err := NewRegionRequestSender(s.cache, cli).SendReqCtx(bo, req, location.Region, time.Second, tikvrpc.TiKV)
+	s.Nil(err)
+	s.NotNil(resp)
+	regionErr, err := resp.GetRegionError()
+	s.Nil(err)
+	s.Nil(regionErr)
+	// It's unreasoneable to retry in upper layer, such as cop request, the upper layer will need to rebuild cop request and retry, there are some unnecessary overhead.
+	s.Equal(cnt, 2)
+	r := s.cache.GetCachedRegionWithRLock(location.Region)
+	s.True(r.isValid())
+}
+
+func (s *testRegionRequestToThreeStoresSuite) TestSliceIdentical() {
+	a := make([]int, 0)
+	b := a
+	s.True(sliceIdentical(a, b))
+	b = make([]int, 0)
+	s.False(sliceIdentical(a, b))
+
+	a = append(a, 1, 2, 3)
+	b = a
+	s.True(sliceIdentical(a, b))
+	b = a[:2]
+	s.False(sliceIdentical(a, b))
+	b = a[1:]
+	s.False(sliceIdentical(a, b))
+	a = a[1:]
+	s.True(sliceIdentical(a, b))
+
+	a = nil
+	b = nil
+
+	s.True(sliceIdentical(a, b))
+	a = make([]int, 0)
+	s.False(sliceIdentical(a, b))
+	a = append(a, 1)
+	s.False(sliceIdentical(a, b))
 }
 
 func (s *testRegionRequestToThreeStoresSuite) loadAndGetLeaderStore() (*Store, string) {