*: move config file option tidb_enable_auto_analyze to sysvar #34643

Alkaagr81 · 2022-05-13T16:43:06Z

What problem does this PR solve?

Issue Number: ref #33769

Problem Summary:

The option tidb_enable_auto_analyze has historically been a config option. But based on requirements from cloud & PM it should instead be a sysvar scope Global.

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

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

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

The option `tidb_enable_auto_analyze `  has historically been a config option. But based on requirements from cloud & PM it should instead be a sysvar scope Global.

ti-chi-bot · 2022-05-13T16:43:08Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

bb7133
morgo

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.

Alkaagr81 · 2022-05-13T16:45:56Z

cc @morgo

sre-bot · 2022-05-13T16:52:43Z

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

session/bootstrap.go

config/config.go

session/bootstrap.go

sessionctx/variable/sysvar.go

morgo

LGTM, just one minor nit about skip-init.

sessionctx/variable/sysvar_test.go

bb7133 · 2022-05-17T10:19:31Z

domain/domain.go

@@ -1328,7 +1325,7 @@ func (do *Domain) UpdateTableStatsLoop(ctx sessionctx.Context) error {
 	}
 	do.SetStatsUpdating(true)
 	do.wg.Run(func() { do.updateStatsWorker(ctx, owner) })
-	if RunAutoAnalyze {
+	if variable.RunAutoAnalyze.Load() {


Changing the code here does not take effect actually, because UpdateTableStatsLoop is only called on the bootstrap stage of TiDB if you check the code in BootstrapSession().

This is fine before because when it is changed in the configuration file, TiDB always needs to restart and the BootstrapSession() is called, but making it a sysvar means it the value can be changed dynamically, so:

When it was FALSE and set to TRUE, the auto-analyze worker will not be invoked.

When it was TRUE and set to FALSE, the auto-analyze worker will still be there.

You can easily verify it by, for example, adding a log in HandleAutoAnalyze().

So my suggestion is removing the if here:

// always runs the loop do.wg.Run(func() { do.autoAnalyzeWorker(owner) })

And moving if to here like:

select { case <-analyzeTicker.C: if owner.IsOwner() && variable.RunAutoAnalyze.Load() { statsHandle.HandleAutoAnalyze(do.InfoSchema()) } case <-do.exit: return }

So that the sysvar takes effect really.

@chrysan Please let me know if you have any comment on it, thanks!

bb7133 · 2022-05-17T10:20:13Z

statistics/integration_test.go

@@ -499,7 +498,6 @@ func TestAnalyzeLongString(t *testing.T) {
 }

 func TestOutdatedStatsCheck(t *testing.T) {
-	domain.RunAutoAnalyze = false


Since the default value of RunAutoAnalyze is true, I think simply removing it may cause some trouble. @chrysan PTAL.

It's false in the test suite. See the change in session/bootstrap.go:2012.

Prefer to keep the variable set in ut or at least an assertion to make the ut safe no matter what has been changed in variable module.

Agreed, an explicit update for UT is more solid. @Alkaagr81 could you update this?

The problem with setting it in the test is:

If you update the atomic directly, it can get overwritten when the sysvar cache is updated. This is because the sysvar cache calls the SetGlobal func with the value from the mysql.global_variables.

To safely change it, tk.MustExec("set global sysvar=x") must be used. But tk is not available in all of the tests. We could add it, but it's quite a bit more code to set up each time.

This is why it uses the bootstrap to disable auto-analyze instead.

We can however add an assertion that runautoanalyze is false. Reading the atomic value is no problem.

I see, an alternative way is to create it as another 'enhancement' issue as a backlog, then we could make this PR delivered with the next version(v6.1)

Forked to #34792

bb7133 · 2022-05-18T15:52:11Z

@Alkaagr81 Please resolve the conflict.

bb7133

LGTM

morgo · 2022-05-18T17:35:42Z

/merge

ti-chi-bot · 2022-05-18T17:35:46Z

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

Commit hash: 70d5032

morgo · 2022-05-18T18:48:27Z

As discussed, we will handle the upgrade step for this in #34711

sre-bot · 2022-05-18T19:04:08Z

TiDB MergeCI notify

🔴 Bad News! New failing [1] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test	🟥 failed 2, success 9, total 11	7 min 27 sec	New failing
idc-jenkins-ci/integration-cdc-test	🟢 all 34 tests passed	24 min	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	6 min 51 sec	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 12 tests passed	6 min 11 sec	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	6 min 3 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	5 min 44 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	5 min 44 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 32 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	5 min 17 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

Alkaagr81 added 3 commits May 12, 2022 15:37

*: move config file option tidb_enable_auto_analyze to sysvar

0c50bed

update

c29fef8

Changes done

8a93079

Alkaagr81 requested a review from a team as a code owner May 13, 2022 16:43

ti-chi-bot added release-note size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 13, 2022

Changes done for upgrade

b70f443

morgo reviewed May 13, 2022

View reviewed changes

session/bootstrap.go Outdated Show resolved Hide resolved

config/config.go Show resolved Hide resolved

session/bootstrap.go Outdated Show resolved Hide resolved

Changes done for upgrade

fbf4ae2

morgo reviewed May 13, 2022

View reviewed changes

sessionctx/variable/sysvar.go Outdated Show resolved Hide resolved

morgo reviewed May 13, 2022

View reviewed changes

Alkaagr81 added 3 commits May 13, 2022 13:59

Conflict solved

642f1b8

Merge

59febfc

Changes done for skipinit

e80eb8d

morgo mentioned this pull request May 16, 2022

Convert several config options to Global sysvars #33769

Open

morgo approved these changes May 16, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 16, 2022

morgo reviewed May 17, 2022

View reviewed changes

sessionctx/variable/sysvar_test.go Outdated Show resolved Hide resolved

removed testcase according to the review

ebb9112

bb7133 reviewed May 17, 2022

View reviewed changes

morgo mentioned this pull request May 17, 2022

session: improve bootstrap code #34755

Merged

12 tasks

Alkaagr81 added 2 commits May 17, 2022 12:12

Change done as per the review

c3b6d1a

merge

0dea10f

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2022

bb7133 approved these changes May 18, 2022

View reviewed changes

ti-chi-bot removed the status/LGT1 Indicates that a PR has LGTM 1. label May 18, 2022

ti-chi-bot added the status/LGT2 Indicates that a PR has LGTM 2. label May 18, 2022

morgo mentioned this pull request May 18, 2022

Tests requiring autoAnalyze disabled should assert it is disabled #34792

Open

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2022

removed upgrade changes and resolved conflict

70d5032

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 18, 2022

Merge branch 'master' into tidb_enable_auto_analyze

113b961

morgo added the compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. label May 18, 2022

Merge branch 'master' into tidb_enable_auto_analyze

45f814d

ti-chi-bot merged commit eae2ae8 into pingcap:master May 18, 2022

hi-rustin mentioned this pull request Mar 7, 2024

Do not allow set tidb_auto_analyze_ratio to 0 #51582

Closed

hi-rustin mentioned this pull request Mar 28, 2024

sysvar: Do not allow set tidb_auto_analyze_ratio to 0 #52190

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: move config file option tidb_enable_auto_analyze to sysvar #34643

*: move config file option tidb_enable_auto_analyze to sysvar #34643

Alkaagr81 commented May 13, 2022

ti-chi-bot commented May 13, 2022 •

edited

Alkaagr81 commented May 13, 2022

sre-bot commented May 13, 2022 •

edited

morgo left a comment

bb7133 May 17, 2022

chrysan May 18, 2022

bb7133 May 17, 2022

morgo May 17, 2022

chrysan May 18, 2022

bb7133 May 18, 2022

morgo May 18, 2022

morgo May 18, 2022

bb7133 May 18, 2022

morgo May 18, 2022

bb7133 commented May 18, 2022

bb7133 left a comment

morgo commented May 18, 2022

ti-chi-bot commented May 18, 2022

morgo commented May 18, 2022

sre-bot commented May 18, 2022

*: move config file option tidb_enable_auto_analyze to sysvar #34643

*: move config file option tidb_enable_auto_analyze to sysvar #34643

Conversation

Alkaagr81 commented May 13, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented May 13, 2022 • edited

Alkaagr81 commented May 13, 2022

sre-bot commented May 13, 2022 • edited

morgo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bb7133 commented May 18, 2022

bb7133 left a comment

Choose a reason for hiding this comment

morgo commented May 18, 2022

ti-chi-bot commented May 18, 2022

morgo commented May 18, 2022

sre-bot commented May 18, 2022

TiDB MergeCI notify

ti-chi-bot commented May 13, 2022 •

edited

sre-bot commented May 13, 2022 •

edited