*: Change stmt summary and capture plan baselines to GLOBAL only

[morgo]

What problem does this PR solve?

Issue Number: part of #30557

Problem Summary:

In #30557 it is proposed that we implement INSTANCE scope for system variables using a simplified proposal (instance and global are mutually exclusive). This addresses all the pre-requisite behavior changes, which are related to stmt summary and capture plan baseslines (the features are related).

Merging this PR is predicated on the proposal for simplified INSTANCE scope being approved. If we don't approve the proposal, this PR does not make sense.

Yes, this is a breaking change! It has been approved by @easonn7

What is changed and how it works?

The change is unfortunately quite large, this is because:

• If it is no longer instance scope, the tie-in to config is obsolete.
• stmt summary predated sysvar validation (and performed its own validation). When refactoring, it made sense in making the changes that it would switch to using sysvar validation and use the correct types in the internal API.

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
  I manually tested that the starting of sysvar cache loop correctly sets the global variablse for stmt summary. If you set global tidb_enable_stmt_summary = 0 and restart the tidb server, it will be off.
• No code

Side effects

• Performance regression: Consumes more CPU
• Performance regression: Consumes more Memory
• Breaking backward compatibility

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

Configuration for Statement Summary and Capture Plan Baselines is now only available on a GLOBAL basis. This simplifies configuration from earlier versions of TiDB where it was configurable on an INSTANCE of GLOBAL basis.

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has been approved by:

• Reminiscent
• djshow832

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

[morgo]

/run-check_dev_2

[Reminiscent]

The capture plan baselines part LGTM.

[Reminiscent]

Another question is if we execute the set tidb_capture_plan_baselines = 1;. The default scope should be transferred from SESSION to GLOBAL. We should update the doc to remind the user.

[morgo]

Another question is if we execute the set tidb_capture_plan_baselines = 1;. The default scope should be transferred from SESSION to GLOBAL. We should update the doc to remind the user.

Sorry, just to clarify: did you mean that (a) we should allow session to be specified for compatibiliity, or (b) update the docs so that it correctly lists scope?

For (b) I have a script that auto-generates the correct docs which I then use to create PRs. See pingcap/docs#6838 for an example where I detected changes and updated the docs (I typically run this script every 2 weeks, so it will be caught before GA).

[Reminiscent]

Another question is if we execute the set tidb_capture_plan_baselines = 1;. The default scope should be transferred from SESSION to GLOBAL. We should update the doc to remind the user.

Sorry, just to clarify: did you mean that (a) we should allow session to be specified for compatibiliity, or (b) update the docs so that it correctly lists scope?

For (b) I have a script that auto-generates the correct docs which I then use to create PRs. See pingcap/docs#6838 for an example where I detected changes and updated the docs (I typically run this script every 2 weeks, so it will be caught before GA).

I mean plan b, thanks for the answer. In fact, what I mean is that when we do not explicitly specify the scope of the query. For example, the original set tidb_capture_plan_baselines = 1; is equivalent to set session tidb_capture_plan_baselines = 1;. When tidb_capture_plan_baselines only supports global scope, set tidb_capture_plan_baselines = 1; is equivalent to set global tidb_capture_plan_baselines = 1;. We need to remind users of this behavior change in the document. So I think plan B is OK:)

[morgo]

When tidb_capture_plan_baselines only supports global scope, set tidb_capture_plan_baselines = 1; is equivalent to set global tidb_capture_plan_baselines = 1;.

Unfortunately this is not the case. SET has a default scope of session, so the user will get an error.

I am hessitant to allow SET SESSION to redirect to global scope in this context, because if a user did not intend for this to execute globally it could cause problems. At least they will get an error message and can check the manual, and it is not typically application sessions which will be executing this query but DBAs.

(In a different context in #30558 I proposed we allow both for backward compatibility. But this is slightly different because it very strictly ensures the previous behavior and it does not propagate to peers).

We need to remind users of this behavior change in the document. So I think plan B is OK:)

Yes, the docs team will also catch it from the compatibility-breaker label. There is a section in each .0 release which covers defaults and variable changes.

[Reminiscent]

When tidb_capture_plan_baselines only supports global scope, set tidb_capture_plan_baselines = 1; is equivalent to set global tidb_capture_plan_baselines = 1;.

Unfortunately this is not the case. SET has a default scope of session, so the user will get an error.

I am hessitant to allow SET SESSION to redirect to global scope in this context, because if a user did not intend for this to execute globally it could cause problems. At least they will get an error message and can check the manual, and it is not typically application sessions which will be eecuting this query but DBAs.

(In a different context in #30558 I proposed we allow both for backward compatibility. But this is slightly different because it very strictly ensures the previous behavior and it does not propagate to peers).

We need to remind users of this behavior change in the document. So I think plan B is OK:)

Yes, the docs team will also catch it from the compatibility-breaker label. There is a section in each .0 release which covers defaults and variable changes.

@qw4990 PTAL.
I think is reasonable to report the error. But I'm not sure it's reasonable for others. So it may need more discussion.

[sre-bot]

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/c25fb114b1e9f72a910f9b1f2f66b4fcbc87c928

[morgo]

/merge

[ti-chi-bot]

This pull request has been accepted and is ready to merge.

Commit hash: d8f420f

[ti-chi-bot]

@morgo: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.