*: pass sql, plan digest down to KV request by crazycs520 · Pull Request #24854 · pingcap/tidb

crazycs520 · 2021-05-24T03:23:12Z

Track issue: #24875

Related PR: #24380

What problem does this PR solve?

This PR use to implement the part of TopSQL feature.

For TiKV to collection resource by SQL statements, TiDB need pass the sql digest and plan digest down to TiKV.

What is changed and how it works?

Encode the sql digest and plan digest to []byte and the set on kv.Context.ResourceGroupTag.

Related PR

*: add resource group tag definition tipb#223 add resource group tag definition
*: Add Digest struct to get bytes of digest parser#1231 Add Digest struct to get bytes of digest

Check List

Tests

Unit test

Side effects

Performance regression
- Consumes more CPU
- Consumes more MEM
Breaking backward compatibility

Release note

No release note

Signed-off-by: crazycs <crazycs520@gmail.com>

MyonKeminta

I think it doesn't look so good that the pessimistic lock requests use a different way to set the resource group tag. I think there should be a better way, but it's ok for now since we don't have too much time.

config/config.go

Signed-off-by: crazycs <crazycs520@gmail.com>

tiancaiamao · 2021-05-24T08:21:08Z

executor/adapter.go

 	memMax := sessVars.StmtCtx.MemTracker.MaxConsumed()
 	diskMax := sessVars.StmtCtx.DiskTracker.MaxConsumed()
-	_, planDigest := getPlanDigest(a.Ctx, a.Plan)
+	planDigest := getPlanDigest(a.Ctx, a.Plan)


Duplicated with line 336?

+1 maybe make getPlanDigest be memoized, just calculate plan once in 336 and directly reuse result in here

Line 336 uses to initialize the plan digest, and it is already cache in StatementContext

executor/update.go

util/resourcegrouptag/resource_group_tag_test.go

lysu · 2021-05-24T11:50:25Z

executor/adapter.go

 	memMax := sessVars.StmtCtx.MemTracker.MaxConsumed()
 	diskMax := sessVars.StmtCtx.DiskTracker.MaxConsumed()
-	_, planDigest := getPlanDigest(a.Ctx, a.Plan)
+	planDigest := getPlanDigest(a.Ctx, a.Plan)


+1 maybe make getPlanDigest be memoized, just calculate plan once in 336 and directly reuse result in here

sessionctx/stmtctx/stmtctx.go

lysu · 2021-05-24T12:42:58Z

tidb/store/driver/txn/txn_driver.go

Line 96 in 3f0c2a3

    
           func (txn *tikvTxn) BatchGet(ctx context.Context, keys []kv.Key) (map[string][]byte, error) {

will be used in insert/replace code path (prefetchUniqueIndices..) , maybe also need setResourceGroupTagForTxn(stmtCtx, snapshot), because it's new snapshot

Signed-off-by: crazycs <crazycs520@gmail.com>

crazycs520 · 2021-05-24T13:31:43Z

tidb/store/driver/txn/txn_driver.go

Line 96 in 3f0c2a3

func (txn *tikvTxn) BatchGet(ctx context.Context, keys []kv.Key) (map[string][]byte, error) {

will be used in insert/replace code path (prefetchUniqueIndices..) , maybe also need setResourceGroupTagForTxn(stmtCtx, snapshot), because it's new snapshot

@lysu , Already call setResourceGroupTagForTxn for replace SQL in replace.go#227

Signed-off-by: crazycs <crazycs520@gmail.com>

lysu · 2021-05-25T02:50:03Z

/lgtm

tiancaiamao · 2021-05-25T03:04:46Z

/LGTM

…e-tag

Signed-off-by: crazycs <crazycs520@gmail.com>

crazycs520 · 2021-05-25T03:41:10Z

/run-all-tests

Signed-off-by: crazycs <crazycs520@gmail.com>

crazycs520 · 2021-05-25T05:07:23Z

/run-all-tests

crazycs520 · 2021-05-25T05:07:58Z

/run-all-tests

crazycs520 · 2021-05-25T05:11:27Z

/run-e2e-test

Signed-off-by: crazycs <crazycs520@gmail.com>

…o resource-tag

crazycs520 · 2021-05-25T06:24:03Z

/run-all-tests

crazycs520 · 2021-05-25T07:00:55Z

/run-unit-test

lysu · 2021-05-25T07:15:00Z

/merge

ti-chi-bot · 2021-05-25T07:15:04Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: 0b35aba

ti-chi-bot · 2021-05-25T07:23:43Z

@crazycs520: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

crazycs520 · 2021-05-25T07:41:57Z

/run-sqllogic-test-1
/run-integration-copr-test

breezewish · 2021-05-28T09:07:57Z

sessionctx/stmtctx/stmtctx.go

+	normalized, sqlDigest := sc.SQLDigest()
+	if len(normalized) == 0 {
+		return nil
+	}
+	tag = resourcegrouptag.EncodeResourceGroupTag(sqlDigest, sc.planDigest)
+	sc.resourceGroupTag.Store(tag)


What about concurrently calling this function?

Concurrently calling doesn't affect correctness.

breezewish · 2021-05-28T09:09:58Z

util/deadlockhistory/deadlock_history.go

 		waitChain = append(waitChain, WaitChainItem{
 			TryLockTxn:     rawItem.Txn,
-			SQLDigest:      sqlDigest,
+			SQLDigest:      hex.EncodeToString(sqlDigest),


Is it better to be NewDigest(sqlDigest).String()?

Yes, use NewDigest(sqlDigest).String() is better.

crazycs520 added 8 commits May 24, 2021 09:51

add benchmark test

cb0f300

Signed-off-by: crazycs <crazycs520@gmail.com>

init

e434f6d

Signed-off-by: crazycs <crazycs520@gmail.com>

init

83b7ccd

Signed-off-by: crazycs <crazycs520@gmail.com>

set resource group task for each kv request

d63cc22

Signed-off-by: crazycs <crazycs520@gmail.com>

refine code and fix test

5769b9b

Signed-off-by: crazycs <crazycs520@gmail.com>

remove useless benchmark test

5624eb8

Signed-off-by: crazycs <crazycs520@gmail.com>

refine code

addd0ef

Signed-off-by: crazycs <crazycs520@gmail.com>

add resource tag for txn commit rpc request

3f0c2a3

Signed-off-by: crazycs <crazycs520@gmail.com>

crazycs520 requested review from a team as code owners May 24, 2021 03:23

crazycs520 requested review from lzmhhh123 and removed request for a team May 24, 2021 03:23

ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 24, 2021

github-actions bot added component/config sig/execution SIG execution sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra labels May 24, 2021

MyonKeminta reviewed May 24, 2021

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

add test and fix some bug

c6b926c

Signed-off-by: crazycs <crazycs520@gmail.com>

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 24, 2021

lysu self-requested a review May 24, 2021 07:46

tiancaiamao reviewed May 24, 2021

View reviewed changes

lysu reviewed May 24, 2021

View reviewed changes

add replace test

81eda6e

Signed-off-by: crazycs <crazycs520@gmail.com>

fix test

14124b7

Signed-off-by: crazycs <crazycs520@gmail.com>

ti-chi-bot added the status/LGT2 Indicates that a PR has LGTM 2. label May 25, 2021

crazycs520 added 2 commits May 25, 2021 11:33

Merge branch 'master' of https://github.com/pingcap/tidb into resourc…

010a556

…e-tag

update go.mod for parser, tipb

7a68984

Signed-off-by: crazycs <crazycs520@gmail.com>

crazycs520 added 3 commits May 25, 2021 12:53

fix test

aa98e5a

Signed-off-by: crazycs <crazycs520@gmail.com>

fix lint and test

dbecfaf

Signed-off-by: crazycs <crazycs520@gmail.com>

Merge branch 'master' into resource-tag

0bde137

Merge branch 'master' into resource-tag

3693e6b

crazycs520 added 3 commits May 25, 2021 14:23

fix race

d5fd72f

Signed-off-by: crazycs <crazycs520@gmail.com>

Merge branch 'resource-tag' of https://github.com/crazycs520/tidb int…

07a41a1

…o resource-tag

Merge branch 'master' into resource-tag

cf4aaad

Merge branch 'master' into resource-tag

0b35aba

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 25, 2021

Merge branch 'master' into resource-tag

8eb8bef

ti-chi-bot merged commit 2580240 into pingcap:master May 25, 2021

breezewish reviewed May 28, 2021

View reviewed changes

crazycs520 mentioned this pull request Jun 21, 2021

*: fix wrong resource tag of transaction commit statement #25616

Merged

ti-srebot mentioned this pull request Jun 23, 2021

*: fix wrong resource tag of transaction commit statement (#25616) #25706

Merged

jackysp mentioned this pull request May 30, 2022

ERROR 1054 (42S22): Unknown column '1' in 'where clause' #35031

Closed

nolouch mentioned this pull request Dec 12, 2023

coprocessor: add query digest tracing in tikv slow query tikv/tikv#16172

Merged

1 task

Conversation

crazycs520 commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Related PR

Check List

Release note

Uh oh!

MyonKeminta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lysu commented May 24, 2021

Uh oh!

crazycs520 commented May 24, 2021

Uh oh!

lysu commented May 25, 2021

Uh oh!

tiancaiamao commented May 25, 2021

Uh oh!

crazycs520 commented May 25, 2021

Uh oh!

crazycs520 commented May 25, 2021

Uh oh!

crazycs520 commented May 25, 2021

Uh oh!

crazycs520 commented May 25, 2021

Uh oh!

crazycs520 commented May 25, 2021

Uh oh!

crazycs520 commented May 25, 2021

Uh oh!

lysu commented May 25, 2021

Uh oh!

ti-chi-bot commented May 25, 2021

Uh oh!

ti-chi-bot commented May 25, 2021

Uh oh!

crazycs520 commented May 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

crazycs520 commented May 24, 2021 •

edited

Loading