session, tikv: allocate task IDs for distsql requests #16520

sticnarf · 2020-04-17T08:09:24Z

Depends on add task_id to kv context kvproto#604

What problem does this PR solve?

Problem Summary:

Currently TiDB provides no information for TiKV that some group of requests belong to the same statement (task). So TiKV's unified read pool uses the start TS of the transaction as the task identifier . It's not suitable in all cases to use start ts as the task id for the unified read pool. For example, in the same transaction, a big query is executed and the following point selects are also downgraded.

What is changed and how it works?

What's Changed:

Currently in this PR, we only set task ids for distsql requests that can be run in transactions. For other distsql requests (like DDL reorg, analyze and checksum), the task id is not provided and TiKV still uses start_ts as task id. And TiKV uses random numbers as the task ids for point gets commands so this PR also does not set task ids for these requests.

Related changes

Need to cherry-pick to the release branch

Check List

Tests

Unit test

Side effects

Release note

Allocate task IDs for distsql requests to help TiKV schedule them better.

Signed-off-by: Yilin Chen <[email protected]>

breezewish · 2020-04-17T12:56:22Z

Should it be called SqlStatementId instead?

Signed-off-by: Jay Lee <[email protected]>

breezewish

LGTM

Signed-off-by: Jay Lee <[email protected]>

codecov · 2020-04-28T14:46:12Z

Codecov Report

Merging #16520 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #16520   +/-   ##
===========================================
  Coverage   80.0905%   80.0905%           
===========================================
  Files           510        510           
  Lines        139597     139597           
===========================================
  Hits         111804     111804           
  Misses        18823      18823           
  Partials       8970       8970

zz-jason · 2020-04-28T14:47:58Z

sessionctx/stmtctx/stmtctx.go

+
+// AllocateTaskID allocates a new unique ID for a statement execution
+func AllocateTaskID() uint64 {
+	return atomic.AddUint64(&taskIDAlloc, 1)


There is a chance that different tidb-server use the same task ID.

TiKV will combine both start_ts and task_id into one.

See https://github.com/tikv/tikv/pull/7521/files#diff-d674d3a094c25d9060532e4d2ff339d1R158-R178

Got it, how about moving this calculation logic into the request sender side?

TiDB's logic is too complicated, I'm not sure if start ts exists when task id is assigned.

Can we use the statement idx, starts from 0 as the task id in the transaction?

The original author also suggests to use the ID for tracing. I'm not sure if allocating as statement index will affect the purpose.

XuHuaiyu · 2020-04-29T02:31:15Z

sessionctx/stmtctx/stmtctx.go

@@ -38,6 +39,13 @@ const (
 	WarnLevelNote = "Note"
 )

+var taskIDAlloc uint64


taskIDAlloc is a server-level variable here.
Can we define it as a session-level variable to reduce the contention on it?

XuHuaiyu · 2020-04-29T06:14:25Z

/bench + sysbench

XuHuaiyu · 2020-04-29T06:20:12Z

/bench + tpcc

XuHuaiyu · 2020-04-29T07:00:18Z

/bench + tpcc

sre-bot · 2020-04-29T08:30:24Z

Benchmark Report

Run TPC-C Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: cd362cae723a1699cb6d3f77ed5b3600ac910c40
+++ tidb: 7aa06145f5eaa760c9e13d8caf0957e5df7caaac
tikv: cdca278ba7c2f631d5b3b5db08f1335dc7a89108
pd: 5a8ccb5b53e0a048146c019bfb48fd99c696cc6c
================================================================================
Measured tpmC (NewOrders): 7111.71 ± 1.41% (std=67.58), delta: 0.31% (p=0.756)

XuHuaiyu

LGTM

zz-jason

@sticnarf please resolve conflict.

Signed-off-by: Yilin Chen <[email protected]>

sticnarf · 2020-05-13T02:52:31Z

/run-all-tests

sticnarf · 2020-05-13T02:57:31Z

/run-unit-test

sticnarf · 2020-05-13T03:02:15Z

PTAL again @XuHuaiyu @zz-jason
Thanks!

zz-jason

LGTM

sre-bot · 2020-05-13T03:07:07Z

/run-all-tests

sre-bot · 2020-05-13T03:10:05Z

@sticnarf merge failed.

sre-bot · 2020-05-13T03:21:04Z

/run-all-tests

sre-bot · 2020-05-13T03:25:05Z

@sticnarf merge failed.

sre-bot · 2020-05-13T03:42:34Z

/run-all-tests

Signed-off-by: sre-bot <[email protected]>

sre-bot · 2020-05-13T03:48:29Z

cherry pick to release-4.0 in PR #17155

Signed-off-by: Yilin Chen <[email protected]>

SunRunAway · 2020-05-26T11:30:29Z

Does this PR need a release note? It seems not to affect users

sticnarf · 2020-05-26T11:34:01Z

Does this PR need a release note? It seems not to affect users

Currently the only effect is to help TiKV schedule requests better. I think you can decide whether to include the release note as you please.

sticnarf added 3 commits April 17, 2020 14:08

add task id to distsql requests

2d98e9c

Signed-off-by: Yilin Chen <[email protected]>

add blanket implementation to new kv interfaces

8a993a6

Signed-off-by: Yilin Chen <[email protected]>

add unit test for allocating task id

70d3ea1

Signed-off-by: Yilin Chen <[email protected]>

sticnarf added status/DNM component/tikv component/session labels Apr 17, 2020

sticnarf requested a review from a team as a code owner April 17, 2020 08:09

ghost requested review from XuHuaiyu and removed request for a team April 17, 2020 08:09

set task id for point select requests

728b663

Signed-off-by: Yilin Chen <[email protected]>

github-actions bot added the sig/execution SIG execution label Apr 17, 2020

BusyJay added 3 commits April 28, 2020 22:04

update dependency

57b2050

Signed-off-by: Jay Lee <[email protected]>

remove unused checksum

016ac01

Signed-off-by: Jay Lee <[email protected]>

Merge branch 'master' into task-id

208e07d

Signed-off-by: Jay Lee <[email protected]>

breezewish reviewed Apr 28, 2020

View reviewed changes

fix tests

7aa0614

Signed-off-by: Jay Lee <[email protected]>

zz-jason reviewed Apr 28, 2020

View reviewed changes

XuHuaiyu reviewed Apr 29, 2020

View reviewed changes

BusyJay removed the status/DNM label Apr 29, 2020

zz-jason reviewed May 6, 2020

View reviewed changes

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label May 6, 2020

Merge remote-tracking branch 'upstream/master' into task-id

7765e2a

Signed-off-by: Yilin Chen <[email protected]>

zz-jason approved these changes May 13, 2020

View reviewed changes

zz-jason added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 13, 2020

Merge branch 'master' into task-id

b15a326

sticnarf added the needs-cherry-pick-4.0 label May 13, 2020

sticnarf added status/can-merge Indicates a PR has been approved by a committer. and removed status/can-merge Indicates a PR has been approved by a committer. labels May 13, 2020

sre-bot merged commit 526a711 into pingcap:master May 13, 2020

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request May 13, 2020

cherry pick pingcap#16520 to release-4.0

65253e3

Signed-off-by: sre-bot <[email protected]>

sre-bot mentioned this pull request May 13, 2020

session, tikv: allocate task IDs for distsql requests (#16520) #17155

Merged

1 task

sticnarf added a commit to sticnarf/tidb that referenced this pull request May 13, 2020

session, tikv: allocate task IDs for distsql requests (pingcap#16520)

8e6411e

Signed-off-by: Yilin Chen <[email protected]>

sre-bot added a commit that referenced this pull request May 14, 2020

session, tikv: allocate task IDs for distsql requests (#16520) (#17155)

f71cdf8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

session, tikv: allocate task IDs for distsql requests #16520

session, tikv: allocate task IDs for distsql requests #16520

sticnarf commented Apr 17, 2020 •

edited by BusyJay

Loading

breezewish commented Apr 17, 2020

breezewish left a comment

codecov bot commented Apr 28, 2020 •

edited

Loading

zz-jason Apr 28, 2020

BusyJay Apr 28, 2020

zz-jason Apr 29, 2020

BusyJay Apr 29, 2020

coocood Apr 29, 2020

BusyJay Apr 29, 2020

XuHuaiyu Apr 29, 2020

XuHuaiyu commented Apr 29, 2020

XuHuaiyu commented Apr 29, 2020

XuHuaiyu commented Apr 29, 2020

sre-bot commented Apr 29, 2020

XuHuaiyu left a comment

zz-jason left a comment

sticnarf commented May 13, 2020

sticnarf commented May 13, 2020

sticnarf commented May 13, 2020

zz-jason left a comment

sre-bot commented May 13, 2020

sre-bot commented May 13, 2020

sre-bot commented May 13, 2020

sre-bot commented May 13, 2020

sre-bot commented May 13, 2020

sre-bot commented May 13, 2020

SunRunAway commented May 26, 2020

sticnarf commented May 26, 2020

session, tikv: allocate task IDs for distsql requests #16520

session, tikv: allocate task IDs for distsql requests #16520

Conversation

sticnarf commented Apr 17, 2020 • edited by BusyJay Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

breezewish commented Apr 17, 2020

breezewish left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 28, 2020 • edited Loading

Codecov Report

zz-jason Apr 28, 2020

Choose a reason for hiding this comment

BusyJay Apr 28, 2020

Choose a reason for hiding this comment

zz-jason Apr 29, 2020

Choose a reason for hiding this comment

BusyJay Apr 29, 2020

Choose a reason for hiding this comment

coocood Apr 29, 2020

Choose a reason for hiding this comment

BusyJay Apr 29, 2020

Choose a reason for hiding this comment

XuHuaiyu Apr 29, 2020

Choose a reason for hiding this comment

XuHuaiyu commented Apr 29, 2020

XuHuaiyu commented Apr 29, 2020

XuHuaiyu commented Apr 29, 2020

sre-bot commented Apr 29, 2020

Benchmark Report

XuHuaiyu left a comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

sticnarf commented May 13, 2020

sticnarf commented May 13, 2020

sticnarf commented May 13, 2020

zz-jason left a comment

Choose a reason for hiding this comment

sre-bot commented May 13, 2020

sre-bot commented May 13, 2020

sre-bot commented May 13, 2020

sre-bot commented May 13, 2020

sre-bot commented May 13, 2020

sre-bot commented May 13, 2020

SunRunAway commented May 26, 2020

sticnarf commented May 26, 2020

sticnarf commented Apr 17, 2020 •

edited by BusyJay

Loading

codecov bot commented Apr 28, 2020 •

edited

Loading