New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

distsql,store/tikv: fix analyze table OOM #4399

Merged
merged 8 commits into from Sep 5, 2017

Conversation

Projects
None yet
8 participants
@tiancaiamao
Contributor

tiancaiamao commented Sep 1, 2017

Analyze table on a huge table would make TiDB OOM.
The real reason is that coprocessor response data (each of size 70M~100M as I observed) is referenced somewhere and can't be garbage collected.

Two simple changes in this PR:

  1. copy the row data to avoid deep reference to the raw data (fix memory leak)
  2. make coprocessor task channel unbuffered (reduce memory use)

@shenli @lamxTyler

Show outdated Hide outdated distsql/distsql.go
@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Sep 1, 2017

Contributor

Now I just change analyze table. @shenli

Contributor

tiancaiamao commented Sep 1, 2017

Now I just change analyze table. @shenli

@lamxTyler

This comment has been minimized.

Show comment
Hide comment
@lamxTyler

lamxTyler Sep 1, 2017

Member

@tiancaiamao Should we copy it when analyzing index?

Member

lamxTyler commented Sep 1, 2017

@tiancaiamao Should we copy it when analyzing index?

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Sep 1, 2017

Contributor

It seems they're already copied in statistics.BuildIndex @lamxTyler

Contributor

tiancaiamao commented Sep 1, 2017

It seems they're already copied in statistics.BuildIndex @lamxTyler

Show outdated Hide outdated util/types/datum.go
@@ -388,9 +387,9 @@ func (it *copIterator) run(ctx goctx.Context) {
}()
}
func (it *copIterator) sendToTaskCh(ctx goctx.Context, t *copTask) (finished bool, canceled bool) {
func (it *copIterator) sendToTaskCh(ctx goctx.Context, t *copTask, taskCh chan<- *copTask) (finished bool, canceled bool) {

This comment has been minimized.

@shenli

shenli Sep 1, 2017

Member

Why change the arg list?

@shenli

shenli Sep 1, 2017

Member

Why change the arg list?

This comment has been minimized.

@tiancaiamao

tiancaiamao Sep 4, 2017

Contributor

I move taskCh out of copIterator, so the extra data is passed as argument.

@tiancaiamao

tiancaiamao Sep 4, 2017

Contributor

I move taskCh out of copIterator, so the extra data is passed as argument.

Show outdated Hide outdated store/tikv/coprocessor.go
Show outdated Hide outdated executor/analyze.go

tiancaiamao added some commits Sep 4, 2017

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 5, 2017

Member

LGTM

Member

coocood commented Sep 5, 2017

LGTM

@zimulala

This comment has been minimized.

Show comment
Hide comment
@zimulala

zimulala Sep 5, 2017

Member

LGTM

Member

zimulala commented Sep 5, 2017

LGTM

@zimulala

This comment has been minimized.

Show comment
Hide comment
@zimulala

zimulala Sep 5, 2017

Member

/run-all-test

Member

zimulala commented Sep 5, 2017

/run-all-test

@zimulala zimulala added status/LGT2 and removed status/LGT1 labels Sep 5, 2017

@iamxy

This comment has been minimized.

Show comment
Hide comment
@iamxy

iamxy Sep 5, 2017

Member

/run-integration-common-test
/run-integration-ddl-test

Member

iamxy commented Sep 5, 2017

/run-integration-common-test
/run-integration-ddl-test

@coocood coocood merged commit e7e4019 into master Sep 5, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@coocood coocood deleted the tiancaiamao/oom branch Sep 5, 2017

mccxj added a commit to mccxj/tidb that referenced this pull request Sep 7, 2017

tiancaiamao added a commit that referenced this pull request Sep 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment