From a8a433d2563a9243316ad79fda266058ead0af2b Mon Sep 17 00:00:00 2001 From: you06 Date: Tue, 7 Nov 2023 11:47:45 +0900 Subject: [PATCH 1/3] fix copr cache panic Signed-off-by: you06 update comment Signed-off-by: you06 --- pkg/store/copr/coprocessor.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/store/copr/coprocessor.go b/pkg/store/copr/coprocessor.go index 8f5f55444897c..3dcd818e79ad2 100644 --- a/pkg/store/copr/coprocessor.go +++ b/pkg/store/copr/coprocessor.go @@ -1685,7 +1685,13 @@ func (worker *copIteratorWorker) handleCopCache(task *copTask, resp *copResponse resp.pbResp.Range = nil } } - resp.detail.CoprCacheHit = true + // If the coprocessor cache key of the task is same whether `worker.enableCollectExecutionInfo` is true or false, + // the cache may be hit when `worker.enableCollectExecutionInfo` is false, but the `detail` is nil. + // Check `resp.detail` to avoid panic. + // Details: https://github.com/pingcap/tidb/issues/48212 + if resp.detail != nil { + resp.detail.CoprCacheHit = true + } return nil } copr_metrics.CoprCacheCounterMiss.Add(1) From f786a9be0c0f6428887fb517a4da585aa5faeb74 Mon Sep 17 00:00:00 2001 From: you06 Date: Thu, 9 Nov 2023 11:28:05 +0900 Subject: [PATCH 2/3] add test Signed-off-by: you06 --- pkg/executor/distsql_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/pkg/executor/distsql_test.go b/pkg/executor/distsql_test.go index b50d15e8ea060..e3681dcf9e5f5 100644 --- a/pkg/executor/distsql_test.go +++ b/pkg/executor/distsql_test.go @@ -569,3 +569,25 @@ func TestCoprocessorBatchByStore(t *testing.T) { } } } + +func TestCoprCacheWithoutExecutionInfo(t *testing.T) { + store := testkit.CreateMockStore(t) + tk := testkit.NewTestKit(t, store) + tk1 := testkit.NewTestKit(t, store) + tk.MustExec("use test") + tk.MustExec("drop table if exists t") + tk.MustExec("create table t(id int)") + tk.MustExec("insert into t values(1), (2), (3)") + + require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/pkg/store/mockstore/unistore/cophandler/mockCopCacheInUnistore", `return(123)`)) + defer func() { + require.NoError(t, failpoint.Disable("github.com/pingcap/tidb/pkg/store/mockstore/unistore/cophandler/mockCopCacheInUnistore")) + }() + + defer tk.MustExec("set @@tidb_enable_collect_execution_info=1") + ctx := context.WithValue(context.Background(), "CheckSelectRequestHook", func(_ *kv.Request) { + tk1.MustExec("set @@tidb_enable_collect_execution_info=0") + }) + tk.MustQuery("select * from t").Check(testkit.Rows("1", "2", "3")) + tk.MustQueryWithContext(ctx, "select * from t").Check(testkit.Rows("1", "2", "3")) +} From 858c32d7b8f032811f5fd006f1ec98df6133f58a Mon Sep 17 00:00:00 2001 From: you06 Date: Thu, 9 Nov 2023 13:19:52 +0900 Subject: [PATCH 3/3] update comment Signed-off-by: you06 --- pkg/store/copr/coprocessor.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/store/copr/coprocessor.go b/pkg/store/copr/coprocessor.go index 3dcd818e79ad2..f0764f57b72a4 100644 --- a/pkg/store/copr/coprocessor.go +++ b/pkg/store/copr/coprocessor.go @@ -1685,8 +1685,9 @@ func (worker *copIteratorWorker) handleCopCache(task *copTask, resp *copResponse resp.pbResp.Range = nil } } - // If the coprocessor cache key of the task is same whether `worker.enableCollectExecutionInfo` is true or false, - // the cache may be hit when `worker.enableCollectExecutionInfo` is false, but the `detail` is nil. + // `worker.enableCollectExecutionInfo` is loaded from the instance's config. Because it's not related to the request, + // the cache key can be same when `worker.enableCollectExecutionInfo` is true or false. + // When `worker.enableCollectExecutionInfo` is false, the `resp.detail` is nil, and hit cache is still possible. // Check `resp.detail` to avoid panic. // Details: https://github.com/pingcap/tidb/issues/48212 if resp.detail != nil {