executor: kill tableReader for each connection correctly #18277

fzhedu · 2020-06-30T05:07:28Z

What problem does this PR solve?

Issue Number: close a part of #18057

Problem Summary:
The tableReader in the coprocessor may retry many times to read regions, druing which, it cannot be aware of the cancelation from client. It may take much of memory for a long time.

What is changed and how it works?

Proposal: detect Killed to stop the goroutine.

What's Changed: should not use CompareAndSwap to change the Killed before Close(), because many runninig goroutines should detect the Killed.

How it Works: the TableReader the detect the killed in every loop.
The query may not exit quickly, because the query do some close work.

Check List

Tests

Integration test
Manual test (add detailed scripts or steps below)
test on mocktikv:

step1. explain analyze select * from lineitem;
step2. kill tidb sessionID on another client;
to see the connection is cancelled.

similarly test on TiFlash is ok.

Release note

To speed up canceling a query

qw4990 · 2020-07-01T07:13:56Z

server/server.go

-	atomic.CompareAndSwapUint32(&sessVars.Killed, 0, 1)
+	atomic.StoreUint32(&sessVars.Killed, 1)


Is there some tricky reason that we use CAS instead of Store here at the begging? Please help us confirm this. @tiancaiamao

Exception handling is a hard.

If we use CAS, exception handling is the same as error handling:

one of the goroutine detect kill (CAS), it return an error

the error is throw to the main loop

the main loop handle worker's Close

This processing is exactly the same as the error handling, when one of the worker get an error.

Exception handling is much difficult than error handling.

The the worker and main loop does not know when the exception happen

Every worker has to handle the exception

The exception handle is tricky, should the worker kill itself or report error to the main loop and let it kill worker? Will the Close message repeat and Close multiple times?

tiancaiamao · 2020-07-02T04:47:14Z

store/tikv/region_request.go

@@ -217,7 +221,18 @@ func (s *RegionRequestSender) SendReqCtx(
 		if err != nil {
 			return nil, nil, errors.Trace(err)
 		}
+
+		// recheck whether the session/query is killed during the Next()
+		if bo.vars != nil && bo.vars.Killed != nil && atomic.LoadUint32(bo.vars.Killed) == 1 {


Maybe the only change necessary is to check CAS(killed) here?
And another question is, why backoff does not check it.

tiancaiamao · 2020-07-03T19:37:54Z

I've find the real cause.

tiancaiamao · 2020-07-06T08:43:59Z

The problem is that current implementation do not really support "cancel" something.
The cancel is implemented by by caller to set the Killed flag and let the callee to inspect it.
If the callee is block on network sending, it has no change to detect Killed.

The callee may block on the network for a long time, because in the rpc error scenario,
send request timeout is not accumulated into the backoff duration.

https://github.com/pingcap/tidb/pull/17135/files#diff-deb6c7bcc2ba593559c11d2e92f8d8dcR56

I do not have any better ideas to fix those issues eithor, so this commit LGTM

fzhedu · 2020-07-06T11:03:46Z

/run-unit-test

… should detect kill == 1

qw4990

LGTM

ti-srebot · 2020-07-07T11:35:11Z

@qw4990,Thanks for your review.

qw4990 · 2020-07-07T11:35:20Z

/merge

ti-srebot · 2020-07-07T11:35:22Z

Sorry @qw4990, you don't have permission to trigger auto merge event on this branch. The number of LGTM for this PR is 1 while it needs 2 at least

SunRunAway · 2020-07-08T13:10:05Z

/lgtm
/merge

SunRunAway · 2020-07-08T13:12:09Z

/merge

fzhedu · 2020-07-09T13:27:51Z

/label type/3.0-cherry-pick

fzhedu · 2020-07-09T13:28:05Z

/run-cherry-picker

fzhedu · 2020-07-13T08:24:58Z

/label needs-cherry-pick-4.0

fzhedu · 2020-07-13T08:25:12Z

/label needs-cherry-pick-3.0

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2020-07-13T08:29:16Z

cherry pick to release-4.0 in PR #18505

fzhedu · 2020-07-13T08:31:30Z

/label needs-cherry-pick-3.1

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2020-07-13T08:32:07Z

cherry pick to release-3.0 in PR #18506

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2020-07-13T08:34:46Z

cherry pick to release-3.1 in PR #18507

qw4990 · 2020-07-13T13:09:01Z

server/server.go

+	type vars struct {
+		killed *uint32
+	}


These lines are unnecessary, so how about removing them?

These lines are unnecessary, so how about removing them?

ok, I will remove in my next PR.

fzhedu · 2020-07-15T05:40:15Z

/unlabel type/4.0-cherry-pick

fzhedu · 2020-07-15T05:40:30Z

/unlabel type/3.0-cherry-pick

…8505)

…8507)

…8506) Signed-off-by: ti-srebot <[email protected]>

fzhedu requested a review from a team as a code owner June 30, 2020 07:54

fzhedu requested review from XuHuaiyu and removed request for a team June 30, 2020 07:54

qw4990 self-requested a review June 30, 2020 07:55

github-actions bot added the sig/execution SIG execution label Jun 30, 2020

fzhedu force-pushed the CloseTableReaderSlow branch from 3e3bf31 to ef97b2c Compare July 1, 2020 05:18

qw4990 reviewed Jul 1, 2020

View reviewed changes

fzhedu changed the title ~~executor: add cancel fun in tableReader for each connection to kill it quickly~~ executor: kill tableReader for each connection correctly Jul 1, 2020

tiancaiamao reviewed Jul 2, 2020

View reviewed changes

tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 6, 2020

fzhedu added 9 commits July 6, 2020 19:15

add cancel fun in tableReader for each connection

1256a77

delete a comment

638f3b6

add check the cancle signal after Next()

ca4f730

use kill to cancel

c0eb783

we should reset kill 1 to 0 when in parallel, because other goroutine…

c6d4f6a

… should detect kill == 1

solve panic bug

4a01ac4

solve panic bug

004620a

solve test errors

2894642

add test

bfbdf42

fzhedu force-pushed the CloseTableReaderSlow branch from ab6e0a4 to bfbdf42 Compare July 6, 2020 13:49

qw4990 approved these changes Jul 7, 2020

View reviewed changes

qw4990 added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 7, 2020

ti-srebot added the type/4.0-cherry-pick label Jul 9, 2020

ti-srebot added the type/3.0-cherry-pick label Jul 9, 2020

fzhedu mentioned this pull request Jul 13, 2020

fix Test failed case from PR/18277 #18504

Merged

ti-srebot added the needs-cherry-pick-4.0 label Jul 13, 2020

ti-srebot added the needs-cherry-pick-3.0 label Jul 13, 2020

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 13, 2020

cherry pick pingcap#18277 to release-4.0

d06ec79

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Jul 13, 2020

executor: kill tableReader for each connection correctly (#18277) #18505

Merged

ti-srebot added the needs-cherry-pick-3.1 label Jul 13, 2020

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 13, 2020

cherry pick pingcap#18277 to release-3.0

9818861

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Jul 13, 2020

executor: kill tableReader for each connection correctly (#18277) #18506

Merged

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 13, 2020

cherry pick pingcap#18277 to release-3.1

68c1069

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Jul 13, 2020

executor: kill tableReader for each connection correctly (#18277) #18507

Merged

qw4990 reviewed Jul 13, 2020

View reviewed changes

ti-srebot removed the type/4.0-cherry-pick label Jul 15, 2020

ti-srebot removed the type/3.0-cherry-pick label Jul 15, 2020

SunRunAway pushed a commit that referenced this pull request Jul 15, 2020

executor: kill tableReader for each connection correctly (#18277) (#1…

fa9dd0a

…8505)

ti-srebot mentioned this pull request Jul 15, 2020

tikv: support cancel before cop response return (#18445) #18580

Merged

SunRunAway pushed a commit that referenced this pull request Jul 17, 2020

executor: kill tableReader for each connection correctly (#18277) (#1…

5e2a251

…8507)

ti-srebot added a commit that referenced this pull request Jul 20, 2020

executor: kill tableReader for each connection correctly (#18277) (#1…

75a6168

…8506) Signed-off-by: ti-srebot <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: kill tableReader for each connection correctly #18277

executor: kill tableReader for each connection correctly #18277

fzhedu commented Jun 30, 2020 •

edited

Loading

qw4990 Jul 1, 2020 •

edited

Loading

tiancaiamao Jul 2, 2020 •

edited

Loading

tiancaiamao Jul 2, 2020

tiancaiamao commented Jul 3, 2020

tiancaiamao commented Jul 6, 2020

fzhedu commented Jul 6, 2020

qw4990 left a comment

ti-srebot commented Jul 7, 2020

qw4990 commented Jul 7, 2020

ti-srebot commented Jul 7, 2020

SunRunAway commented Jul 8, 2020

SunRunAway commented Jul 8, 2020

fzhedu commented Jul 9, 2020

fzhedu commented Jul 9, 2020

fzhedu commented Jul 13, 2020

fzhedu commented Jul 13, 2020

ti-srebot commented Jul 13, 2020

fzhedu commented Jul 13, 2020

ti-srebot commented Jul 13, 2020

ti-srebot commented Jul 13, 2020

qw4990 Jul 13, 2020

fzhedu Jul 13, 2020

fzhedu commented Jul 15, 2020

fzhedu commented Jul 15, 2020

		atomic.CompareAndSwapUint32(&sessVars.Killed, 0, 1)
		atomic.StoreUint32(&sessVars.Killed, 1)

executor: kill tableReader for each connection correctly #18277

executor: kill tableReader for each connection correctly #18277

Conversation

fzhedu commented Jun 30, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

qw4990 Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

tiancaiamao Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

tiancaiamao Jul 2, 2020

Choose a reason for hiding this comment

tiancaiamao commented Jul 3, 2020

tiancaiamao commented Jul 6, 2020

fzhedu commented Jul 6, 2020

qw4990 left a comment

Choose a reason for hiding this comment

ti-srebot commented Jul 7, 2020

qw4990 commented Jul 7, 2020

ti-srebot commented Jul 7, 2020

SunRunAway commented Jul 8, 2020

SunRunAway commented Jul 8, 2020

fzhedu commented Jul 9, 2020

fzhedu commented Jul 9, 2020

fzhedu commented Jul 13, 2020

fzhedu commented Jul 13, 2020

ti-srebot commented Jul 13, 2020

fzhedu commented Jul 13, 2020

ti-srebot commented Jul 13, 2020

ti-srebot commented Jul 13, 2020

qw4990 Jul 13, 2020

Choose a reason for hiding this comment

fzhedu Jul 13, 2020

Choose a reason for hiding this comment

fzhedu commented Jul 15, 2020

fzhedu commented Jul 15, 2020

fzhedu commented Jun 30, 2020 •

edited

Loading

qw4990 Jul 1, 2020 •

edited

Loading

tiancaiamao Jul 2, 2020 •

edited

Loading