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.

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

The meaning of max-memory in config has changed #17298

Closed
cfzjywxk opened this issue May 19, 2020 · 7 comments
Closed

The meaning of max-memory in config has changed #17298

cfzjywxk opened this issue May 19, 2020 · 7 comments
Assignees
Labels

Comments

@cfzjywxk
Copy link
Contributor

cfzjywxk commented May 19, 2020

Bug Report

Please answer these questions before submitting your issue. Thanks!

Since the GlobalMemoryUsageTracker is introduced, its size is decided by performance.max-memory configuration. But this configuration is still used by prepared plan cache to set the maximum plan cache size, better to remove this usage for prepare plan cache or change its name to avoid possible incorrect configurations, a single configuration should not have two usages.

1. Minimal reproduce step (Required)

2. What did you expect to see? (Required)

3. What did you see instead (Required)

4. Affected version (Required)

5. Root Cause Analysis

@cfzjywxk cfzjywxk added the type/bug The issue is confirmed as a bug. label May 19, 2020
@zz-jason zz-jason added the sig/execution SIG execution label May 20, 2020
@zz-jason
Copy link
Member

@SunRunAway PTAL

@SunRunAway SunRunAway self-assigned this May 21, 2020
@SunRunAway
Copy link
Contributor

SunRunAway commented May 21, 2020

Good catch. Thank you @cfzjywxk

The annotation in config file says,

# Max memory size to use, 0 use the total usable memory in the machine.

The configuration description clearly says that max-memory is the max memory to use (for tidb-server perhaps everyone would assume that).

But, The documentation says it's only used for plan cache.

The maximum memory limit for the Prepared Least Recently Used (LRU) caching.

@SunRunAway
Copy link
Contributor

SunRunAway commented May 21, 2020

IMO, for best compatibility for upgraded servers from old versions, we should,

  1. Fix the annotation in tidb.toml.example and tell the readers that max-memory is only for Plan Cache in tidb.toml.example in all release branches(v2.1, v3.0, v3.1, v4.0).
  2. max-memory should be changed back to the old meaning, and only be scoped in Plan Cache according to the documentation, then hide and deprecate in the future versions (v4.1, v5.0).
  3. For the memory limit of a whole server, introduce a new configuration named like memory-quota-server in the [performance] section.

@SunRunAway SunRunAway added this to the v4.0.0-ga milestone May 21, 2020
@SunRunAway SunRunAway added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label May 21, 2020
@SunRunAway
Copy link
Contributor

@Yisaer Would you like to take a look?

@SunRunAway SunRunAway removed the priority/release-blocker This issue blocks a release. Please solve it ASAP. label May 21, 2020
@SunRunAway SunRunAway removed this from the v4.0.0-ga milestone May 21, 2020
@SunRunAway
Copy link
Contributor

cc @fzhedu @XuHuaiyu

@Yisaer
Copy link
Contributor

Yisaer commented Dec 15, 2020

fixed in #17532

@ti-srebot
Copy link
Contributor

Please edit this comment or add a new comment to complete the following information

Not a bug

  1. Remove the 'type/bug' label
  2. Add notes to indicate why it is not a bug

Duplicate bug

  1. Add the 'type/duplicate' label
  2. Add the link to the original bug

Bug

Note: Make Sure that 'component', and 'severity' labels are added
Example for how to fill out the template: #20100

1. Root Cause Analysis (RCA) (optional)

2. Symptom (optional)

3. All Trigger Conditions (optional)

4. Workaround (optional)

5. Affected versions

6. Fixed versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants