distsql: clean the memory usage of MemTracker when a query ends (#10893) #10898

SunRunAway · 2019-06-21T03:58:47Z

What problem does this PR solve?

closes #10893

What is changed and how it works?

Correctly release memory consuming in distsql.

Check List

Tests

Unit test
Manual test (add detailed scripts or steps below)

run a select query like select * from t, ta

mysql> SELECT * FROM INFORMATION_SCHEMA.PROCESSLIST;
+------+------+-----------+------+---------+------+-------+----------------------------------------------+------+
| ID   | USER | HOST      | DB   | COMMAND | TIME | STATE | INFO                                         | MEM  |
+------+------+-----------+------+---------+------+-------+----------------------------------------------+------+
|    1 | root | 127.0.0.1 | test | Sleep   |    3 | 2     |                                              |    0 |
|    2 | root | 127.0.0.1 | test | Query   |    0 | 2     | SELECT * FROM INFORMATION_SCHEMA.PROCESSLIST |    0 |
+------+------+-----------+------+---------+------+-------+----------------------------------------------+------+
2 rows in set (0.00 sec)

Code changes

None

Side effects

Increased code complexity

Related changes

None

SunRunAway · 2019-06-21T05:39:34Z

/run-all-tests

SunRunAway · 2019-06-21T06:01:54Z

/run-unit-test

SunRunAway · 2019-06-21T06:03:20Z

/run-all-tests

codecov · 2019-06-21T06:46:33Z

Codecov Report

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

@@             Coverage Diff             @@
##             master     #10898   +/-   ##
===========================================
  Coverage   80.9333%   80.9333%           
===========================================
  Files           418        418           
  Lines         89245      89245           
===========================================
  Hits          72229      72229           
  Misses        11786      11786           
  Partials       5230       5230

SunRunAway · 2019-06-21T07:20:29Z

/run-all-tests

SunRunAway · 2019-06-21T08:34:15Z

/run-all-tests

SunRunAway · 2019-06-21T08:40:07Z

/rebuild

crazycs520 · 2019-06-24T14:46:53Z

distsql/select_result.go

@@ -106,17 +106,21 @@ func (r *selectResult) fetch(ctx context.Context) {
 			return


Should we check in here?

Yes, we should. Per https://godoc.org/github.com/pingcap/tidb/kv#Response, resultSubset may be nil even if no error take place.

// Next returns a resultSubset from a single storage unit. // When full result set is returned, nil is returned. Next(ctx context.Context) (resultSubset ResultSubset, err error)

I will add a comment here.

crazycs520 · 2019-06-24T14:49:53Z

distsql/select_result.go

@@ -231,16 +234,31 @@ func (r *selectResult) readRowsData(chk *chunk.Chunk) (err error) {
 		}
 	}
 	r.selectResp.Chunks[r.respChkIdx].RowsData = rowsData
+	r.memConsume(-int64(originMemory - r.selectResp.Size()))


r.memConsume(int64(r.selectResp.Size() - originMemory))

I prefer to directly to use -, it indicates a memory releasing.

By the way, Maybe we should not reduce the used memory size. because the underlying buffer is still referenced by rowsData, the memory is actually not changed.

You're right. I'll try to implement it in another way.

distsql/distsql_test.go

zz-jason · 2019-06-26T01:16:26Z

distsql/select_result.go

 		}

 		select {
 		case r.results <- result:
 		case <-r.closed:
 			// If selectResult called Close() already, make fetch goroutine exit.
+			if resultSubset != nil {


no need to check nil again

if line 102 occurs error, resultSubset would be nil.

zz-jason · 2019-06-26T01:19:14Z

distsql/select_result.go

 		r.selectResp = new(tipb.SelectResponse)
 		err := r.selectResp.Unmarshal(re.result.GetData())
 		if err != nil {
 			return errors.Trace(err)
 		}
-		if r.memTracker != nil && r.selectResp != nil {
-			r.memTracker.Consume(int64(r.selectResp.Size()))
+		if r.selectResp != nil {


it's guaranteed to be not nil in line 176.

zz-jason · 2019-06-26T01:27:46Z

distsql/select_result.go

@@ -231,16 +234,31 @@ func (r *selectResult) readRowsData(chk *chunk.Chunk) (err error) {
 		}
 	}
 	r.selectResp.Chunks[r.respChkIdx].RowsData = rowsData
+	r.memConsume(-int64(originMemory - r.selectResp.Size()))


By the way, Maybe we should not reduce the used memory size. because the underlying buffer is still referenced by rowsData, the memory is actually not changed.

CLAassistant · 2019-06-26T06:35:29Z

All committers have signed the CLA.

…cap#10893)

SunRunAway · 2019-06-26T06:51:37Z

/run-all-tests

distsql/select_result.go

SunRunAway · 2019-06-26T07:05:33Z

@zz-jason @crazycs520 PTAL again. Thanks.

qw4990

LGTM

zz-jason

LGTM

…cap#10898)

…) (#10971)

SunRunAway added sig/execution SIG execution type/bugfix This PR fixes a bug. labels Jun 21, 2019

SunRunAway requested review from qw4990 and zz-jason June 21, 2019 04:06

SunRunAway marked this pull request as ready for review June 21, 2019 04:06

qw4990 requested a review from XuHuaiyu June 21, 2019 05:59

SunRunAway force-pushed the memory branch from 4d045ac to 3bf45cc Compare June 21, 2019 06:46

crazycs520 reviewed Jun 24, 2019

View reviewed changes

distsql/distsql_test.go Outdated Show resolved Hide resolved

zz-jason reviewed Jun 26, 2019

View reviewed changes

SunRunAway force-pushed the memory branch from db4eca6 to dbc02eb Compare June 26, 2019 06:42

SunRunAway added 3 commits June 26, 2019 14:47

distsql: clean the memory usage of MemTracker when a query ends (ping…

1c812f9

…cap#10893)

ddl: fix data race

311d1bc

fix

d13a063

SunRunAway force-pushed the memory branch from dbc02eb to 096e2ac Compare June 26, 2019 06:48

address code review

7d963fb

SunRunAway force-pushed the memory branch from 096e2ac to 7d963fb Compare June 26, 2019 06:50

qw4990 reviewed Jun 26, 2019

View reviewed changes

distsql/select_result.go Show resolved Hide resolved

SunRunAway requested review from zz-jason and crazycs520 June 26, 2019 07:01

qw4990 reviewed Jun 26, 2019

View reviewed changes

qw4990 added status/all tests passed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 26, 2019

Merge branch 'master' into memory

b2b869d

zz-jason approved these changes Jun 27, 2019

View reviewed changes

zz-jason merged commit fc6baf2 into pingcap:master Jun 27, 2019

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 27, 2019

SunRunAway added a commit to SunRunAway/tidb that referenced this pull request Jun 27, 2019

distsql: clean the memory usage of MemTracker when a query end… (ping…

69dd7a1

…cap#10898)

SunRunAway mentioned this pull request Jun 27, 2019

distsql: clean the memory usage of MemTracker when a query ends (#10898) #10970

Merged

SunRunAway added a commit to SunRunAway/tidb that referenced this pull request Jun 27, 2019

distsql: clean the memory usage of MemTracker when a query end… (ping…

4e28bb5

…cap#10898)

SunRunAway mentioned this pull request Jun 27, 2019

distsql: clean the memory usage of MemTracker when a query end… (#10898) #10971

Merged

SunRunAway deleted the memory branch June 27, 2019 12:54

jackysp pushed a commit that referenced this pull request Jun 28, 2019

distsql: clean the memory usage of MemTracker when a query end… (#10898…

b65f81f

…) (#10971)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

distsql: clean the memory usage of MemTracker when a query ends (#10893) #10898

distsql: clean the memory usage of MemTracker when a query ends (#10893) #10898

SunRunAway commented Jun 21, 2019

SunRunAway commented Jun 21, 2019

SunRunAway commented Jun 21, 2019

SunRunAway commented Jun 21, 2019

codecov bot commented Jun 21, 2019 •

edited

Loading

SunRunAway commented Jun 21, 2019

SunRunAway commented Jun 21, 2019

SunRunAway commented Jun 21, 2019

crazycs520 Jun 24, 2019

SunRunAway Jun 26, 2019 •

edited

Loading

crazycs520 Jun 24, 2019 •

edited

Loading

zz-jason Jun 26, 2019

zz-jason Jun 26, 2019

SunRunAway Jun 26, 2019 •

edited

Loading

zz-jason Jun 26, 2019

SunRunAway Jun 26, 2019

zz-jason Jun 26, 2019

zz-jason Jun 26, 2019

CLAassistant commented Jun 26, 2019 •

edited

Loading

SunRunAway commented Jun 26, 2019

SunRunAway commented Jun 26, 2019

qw4990 left a comment

zz-jason left a comment

		@@ -106,17 +106,21 @@ func (r *selectResult) fetch(ctx context.Context) {
		return

distsql: clean the memory usage of MemTracker when a query ends (#10893) #10898

distsql: clean the memory usage of MemTracker when a query ends (#10893) #10898

Conversation

SunRunAway commented Jun 21, 2019

What problem does this PR solve?

What is changed and how it works?

Check List

SunRunAway commented Jun 21, 2019

SunRunAway commented Jun 21, 2019

SunRunAway commented Jun 21, 2019

codecov bot commented Jun 21, 2019 • edited Loading

Codecov Report

SunRunAway commented Jun 21, 2019

SunRunAway commented Jun 21, 2019

SunRunAway commented Jun 21, 2019

crazycs520 Jun 24, 2019

Choose a reason for hiding this comment

SunRunAway Jun 26, 2019 • edited Loading

Choose a reason for hiding this comment

crazycs520 Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

zz-jason Jun 26, 2019

Choose a reason for hiding this comment

zz-jason Jun 26, 2019

Choose a reason for hiding this comment

SunRunAway Jun 26, 2019 • edited Loading

Choose a reason for hiding this comment

zz-jason Jun 26, 2019

Choose a reason for hiding this comment

SunRunAway Jun 26, 2019

Choose a reason for hiding this comment

zz-jason Jun 26, 2019

Choose a reason for hiding this comment

zz-jason Jun 26, 2019

Choose a reason for hiding this comment

CLAassistant commented Jun 26, 2019 • edited Loading

SunRunAway commented Jun 26, 2019

SunRunAway commented Jun 26, 2019

qw4990 left a comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 21, 2019 •

edited

Loading

SunRunAway Jun 26, 2019 •

edited

Loading

crazycs520 Jun 24, 2019 •

edited

Loading

SunRunAway Jun 26, 2019 •

edited

Loading

CLAassistant commented Jun 26, 2019 •

edited

Loading