Skip to content
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.

Sign up for GitHub

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

Jump to bottom

information: Fix issue of query information_schema.columns cost too much memory when there are lots of tables in TiDB #18362

Merged
merged 12 commits into from
Jul 8, 2020
Merged

information: Fix issue of query information_schema.columns cost too much memory when there are lots of tables in TiDB #18362

merged 12 commits into from
Jul 8, 2020

Conversation

crazycs520
Copy link
Contributor

What problem does this PR solve?

Fix issue of query information_schema.columns cost too many memory when there are lots of tables in TiDB.

What is changed and how it works?

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • PR to update pingcap/tidb-ansible:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Manual test (add detailed scripts or steps below)

Side effects

  • No

Release note

  • Fix issue of query information_schema.columns cost too many memory when there are lots of tables in TiDB.

@crazycs520
init
8e2d8e3 
Signed-off-by: crazycs <crazycs520@gmail.com>
@crazycs520
add cache
7f10d3d 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520 crazycs520 added type/enhancement The issue or PR belongs to an enhancement. component/infoschema labels Jul 3, 2020
@crazycs520 crazycs520 requested a review from a team as a code owner July 3, 2020 13:31
@crazycs520 crazycs520 requested review from SunRunAway and removed request for a team July 3, 2020 13:31
@crazycs520
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented Jul 3, 2020 •
edited
Loading

Codecov Report

Merging #18362 into master will increase coverage by 0.1487%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #18362        +/-   ##
================================================
+ Coverage   79.4194%   79.5681%   +0.1487%     
================================================
  Files           540        540                
  Lines        144748     145621       +873     
================================================
+ Hits         114958     115868       +910     
+ Misses        20504      20491        -13     
+ Partials       9286       9262        -24

@github-actions github-actions bot added the sig/execution SIG execution label Jul 3, 2020
@crazycs520
Copy link
Contributor Author

/run-integration-copr-test

wjhuang2016
wjhuang2016 reviewed Jul 6, 2020
View reviewed changes
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the PR description. What is changed and how it works?

wjhuang2016
wjhuang2016 reviewed Jul 6, 2020
View reviewed changes
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 6, 2020
ti-srebot
ti-srebot previously approved these changes Jul 6, 2020
View reviewed changes
Copy link
Contributor

@ti-srebot ti-srebot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot
Copy link
Contributor

@wjhuang2016,Thanks for your review.

tangenta
tangenta reviewed Jul 6, 2020
View reviewed changes
Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the major memory consumption exist in memtableRetriever.rows? What about is.AllSchemas()? Is there any benchmark to show the problem is eased?

executor/infoschema_reader.go Outdated Show resolved Hide resolved
@crazycs520
address comment
3c9e289 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
Merge branch 'master' into columns
e988446
djshow832
djshow832 reviewed Jul 7, 2020
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 7, 2020
ti-srebot
ti-srebot previously approved these changes Jul 7, 2020
View reviewed changes
@ti-srebot
Copy link
Contributor

@djshow832,Thanks for your review.

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

/rebuild

@crazycs520
Copy link
Contributor Author

/run-unit-test

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 8, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@crazycs520 merge failed.

@crazycs520
Copy link
Contributor Author

/run-unit-test

@crazycs520 crazycs520 merged commit ff94f2b into pingcap:master Jul 8, 2020
@crazycs520
Copy link
Contributor Author

/run-cherry-picker

@zimulala zimulala mentioned this pull request Jul 8, 2020
Merged
@ti-srebot ti-srebot mentioned this pull request Jul 8, 2020
Merged
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 8, 2020
@crazycs520 @ti-srebot
cherry pick pingcap#18362 to release-4.0
b691e2f 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #18436

SunRunAway pushed a commit that referenced this pull request Jul 15, 2020
@ti-srebot
information: Fix issue of query information_schema.columns cost too…
4795387 
… much memory when there are lots of tables in TiDB (#18362) (#18503) (#18436)
@ghost ghost mentioned this pull request Jul 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjhuang2016 wjhuang2016 wjhuang2016 left review comments

@tangenta tangenta tangenta left review comments

@djshow832 djshow832 djshow832 approved these changes

@ti-srebot ti-srebot ti-srebot left review comments

@SunRunAway SunRunAway Awaiting requested review from SunRunAway

Assignees
No one assigned
Labels
component/infoschema sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@crazycs520 @ti-srebot @wjhuang2016 @tangenta @djshow832