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

support "SHOW PROCESSLIST MEMINFO" #10199

Closed
zz-jason opened this issue Apr 19, 2019 · 8 comments · Fixed by #10837
Closed

support "SHOW PROCESSLIST MEMINFO" #10199

zz-jason opened this issue Apr 19, 2019 · 8 comments · Fixed by #10837
Assignees
@SunRunAway
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. sig/execution SIG execution type/new-feature

Comments

@zz-jason
Copy link
Member

Feature Request

Is your feature request related to a problem? Please describe:

In #8543, we removed the memory info from the result of show processlist. As discussed in #8543 (comment), we decide to add a new command like show processlist meminfo to show the process lists + meminfo

Describe the feature you'd like:

use show processlist meminfo to show the result of process lists and meminfo

Describe alternatives you've considered:

No

Teachability, Documentation, Adoption, Migration Strategy:

No
@zz-jason zz-jason added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. sig/execution SIG execution type/new-feature labels Apr 19, 2019
@zz-jason zz-jason assigned zz-jason and unassigned zz-jason Apr 19, 2019
@ngaut
Copy link
Member

ngaut commented Apr 22, 2019

Can we add a new column to the output of "show processlist"? It should be more convenient.
What do you guys think? @morgo @kolbe

@morgo
Copy link
Contributor

morgo commented Apr 22, 2019

@ngaut It actually was on show processlist, the previous PR removed it. The memory feature does not work exactly as described - it only records memory of some parts of execution, which makes it confusing for new users (and me:-)).

@ngaut
Copy link
Member

ngaut commented Apr 22, 2019

Great, let's make it work properly.

@zz-jason
Copy link
Member Author

@ngaut It actually was on show processlist, the previous PR removed it. The memory feature does not work exactly as described - it only records memory of some parts of execution, which makes it confusing for new users (and me:-)).

We can trace memory usage in distsql now. It's much precise than before. However the memory info for some simple statements can still be zero. Because they are not memory intensive queries.

I think we can optimize it in these ways:

  1. Display the memory info in show processlist. Don'y display zero memory usage for simple queries. We can display a message like "little".
  2. Display the memory info in show processlist meminfo. Display the exact memory usage we tracked.

@kolbe
Copy link
Contributor

kolbe commented Apr 22, 2019 •
edited
Loading

Why don't we just add a column to information_schema.processlist instead of fooling around w/ extra SHOW PROCESSLIST syntax?

I worry that adding columns to SHOW PROCESSLIST output might break some tools, while I think it's probably safer to add things to information_schema.processlist.

@morgo
Copy link
Contributor

morgo commented Apr 23, 2019

+1 to @kolbe's suggestion. It is easier to extend i_s because applications can write SQL to customize it. For SHOW commands it is better to remain compatible.

@SunRunAway SunRunAway self-assigned this Jun 17, 2019
@zz-jason
Copy link
Member Author

@morgo, @kolbe To summary, the refinements we should do are:

  1. add a column named mem for table information_schema.processlist
  2. add a new command show processlist meminfo to display the result of show processlist plus a mem column.

Am I right? If so, @SunRunAway is going to address this issue.

@kolbe
Copy link
Contributor

kolbe commented Jun 17, 2019

I think we should only do #1 (add a column to information_schema.processlist). There is no great benefit to extending show processlist with additional syntax.

SunRunAway added a commit to SunRunAway/tidb that referenced this issue Jun 18, 2019
@SunRunAway
*: add a column describing memory usage for table information_schema.…
75dd1c6 
…processlist

Closes pingcap#10199
@SunRunAway SunRunAway mentioned this issue Jun 18, 2019
Merged
SunRunAway added a commit that referenced this issue Jun 20, 2019
@SunRunAway
*: add a column describing memory usage for table information_schema.…
75b9470 
…processlist (#10837)

*: add a column describing memory usage for table information_schema.processlist

Closes #10199
SunRunAway added a commit to SunRunAway/tidb that referenced this issue Jun 20, 2019
@SunRunAway
*: add a column describing memory usage for table information_schema…
a5e6cb0 
….processlist (pingcap#10837)

     *: add a column describing memory usage for table information_schema.processlist

     Closes pingcap#10199

      Conflicts:
             executor/show.go
             infoschema/tables.go
             infoschema/tables_test.go
             util/misc_test.go
             util/processinfo.go
SunRunAway added a commit to SunRunAway/tidb that referenced this issue Jun 20, 2019
@SunRunAway
*: add a column describing memory usage for table information_schema.…
db621e9 
…processlist (pingcap#10837)

*: add a column describing memory usage for table information_schema.processlist

Closes pingcap#10199
Conflicts:
     executor/show.go
     infoschema/tables.go
     infoschema/tables_test.go
     util/misc_test.go
     util/processinfo.go
SunRunAway added a commit to SunRunAway/tidb that referenced this issue Oct 17, 2019
@SunRunAway
*: add a column describing memory usage for table information_schema.…
686977e 
…processlist

Closes pingcap#10199
@SunRunAway SunRunAway mentioned this issue Oct 17, 2019
Merged
SunRunAway added a commit to SunRunAway/tidb that referenced this issue Oct 18, 2019
@SunRunAway
*: add a column describing memory usage for table information_schema.…
e1374ff 
…processlist

Closes pingcap#10199
@SunRunAway SunRunAway mentioned this issue Oct 18, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@SunRunAway SunRunAway

Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. sig/execution SIG execution type/new-feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

*: add a column describing memory usage for table information_schema.processlist SunRunAway/tidb
5 participants
@morgo @SunRunAway @ngaut @kolbe @zz-jason