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

*: move config file option tidb_enable_auto_analyze to sysvar #34643

Merged
merged 14 commits into from
May 18, 2022
Merged

*: move config file option tidb_enable_auto_analyze to sysvar #34643

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0c50bed
*: move config file option tidb_enable_auto_analyze to sysvar
Alkaagr81 May 12, 2022
c29fef8
update
Alkaagr81 May 13, 2022
8a93079
Changes done
Alkaagr81 May 13, 2022
b70f443
Changes done for upgrade
Alkaagr81 May 13, 2022
fbf4ae2
Changes done for upgrade
Alkaagr81 May 13, 2022
642f1b8
Conflict solved
Alkaagr81 May 13, 2022
59febfc
Merge
Alkaagr81 May 15, 2022
e80eb8d
Changes done for skipinit
Alkaagr81 May 15, 2022
ebb9112
removed testcase according to the review
Alkaagr81 May 17, 2022
c3b6d1a
Change done as per the review
Alkaagr81 May 17, 2022
0dea10f
merge
Alkaagr81 May 17, 2022
70d5032
removed upgrade changes and resolved conflict
Alkaagr81 May 18, 2022
113b961
Merge branch 'master' into tidb_enable_auto_analyze
ti-chi-bot May 18, 2022
45f814d
Merge branch 'master' into tidb_enable_auto_analyze
ti-chi-bot May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,9 @@ type Config struct {

// EnableBatchDML, unused since bootstrap v90
EnableBatchDML bool `toml:"enable-batch-dml" json:"enable-batch-dml"`

// RunAutoAnalyze, unused since bootstrap v92
RunAutoAnalyze bool `toml:"run-auto-analyze" json:"run-auto-analyze"`
}

// UpdateTempStoragePath is to update the `TempStoragePath` if port/statusPort was changed
Expand Down Expand Up @@ -597,7 +600,6 @@ type Performance struct {
TCPKeepAlive bool `toml:"tcp-keep-alive" json:"tcp-keep-alive"`
TCPNoDelay bool `toml:"tcp-no-delay" json:"tcp-no-delay"`
CrossJoin bool `toml:"cross-join" json:"cross-join"`
RunAutoAnalyze bool `toml:"run-auto-analyze" json:"run-auto-analyze"`
DistinctAggPushDown bool `toml:"distinct-agg-push-down" json:"distinct-agg-push-down"`
// Whether enable projection push down for coprocessors (both tikv & tiflash), default false.
ProjectionPushDown bool `toml:"projection-push-down" json:"projection-push-down"`
Expand Down Expand Up @@ -814,7 +816,6 @@ var defaultConf = Config{
TCPNoDelay: true,
CrossJoin: true,
StatsLease: "3s",
RunAutoAnalyze: true,
StmtCountLimit: 5000,
FeedbackProbability: 0.0,
QueryFeedbackLimit: 512,
Expand Down Expand Up @@ -938,6 +939,7 @@ var deprecatedConfig = map[string]struct{}{
"enable-batch-dml": {}, // use tidb_enable_batch_dml
"mem-quota-query": {},
"query-log-max-len": {},
"performance.run-auto-analyze": {}, //use tidb_enable_auto_analyze
}

func isAllDeprecatedConfigItems(items []string) bool {
Expand Down
3 changes: 0 additions & 3 deletions config/config.toml.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,9 +232,6 @@ cross-join = true
# Stats lease duration, which influences the time of analyze and stats load.
stats-lease = "3s"

# Run auto analyze worker on this tidb-server.
run-auto-analyze = true

# Probability to use the query feedback to update stats, 0.0 or 1.0 for always false/true.
feedback-probability = 0.0

Expand Down
5 changes: 1 addition & 4 deletions domain/domain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1291,9 +1291,6 @@ func (do *Domain) SetStatsUpdating(val bool) {
}
}

// RunAutoAnalyze indicates if this TiDB server starts auto analyze worker and can run auto analyze job.
var RunAutoAnalyze = true

// LoadAndUpdateStatsLoop loads and updates stats info.
func (do *Domain) LoadAndUpdateStatsLoop(ctxs []sessionctx.Context) error {
if err := do.UpdateTableStatsLoop(ctxs[0]); err != nil {
Expand Down Expand Up @@ -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() {
Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

do.wg.Run(func() { do.autoAnalyzeWorker(owner) })
}
do.wg.Run(func() { do.gcAnalyzeHistory(owner) })
Expand Down
2 changes: 0 additions & 2 deletions executor/oomtest/oom_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (

"github.com/pingcap/log"
"github.com/pingcap/tidb/config"
"github.com/pingcap/tidb/domain"
"github.com/pingcap/tidb/testkit"
"github.com/pingcap/tidb/util/testbridge"
"github.com/stretchr/testify/require"
Expand All @@ -36,7 +35,6 @@ import (
func TestMain(m *testing.M) {
testbridge.SetupForCommonTest()
registerHook()
domain.RunAutoAnalyze = false
config.UpdateGlobal(func(conf *config.Config) {
conf.OOMAction = config.OOMActionLog
})
Expand Down
17 changes: 16 additions & 1 deletion session/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,11 +617,13 @@ const (
version89 = 89
// version90 converts enable-batch-dml to a sysvar
version90 = 90
// version92 converts run-auto-analyze to a sysvar
version92 = 92
)

// currentBootstrapVersion is defined as a variable, so we can modify its value for testing.
// please make sure this is the largest version
var currentBootstrapVersion int64 = version90
var currentBootstrapVersion int64 = version92

var (
bootstrapVersion = []func(Session, int64){
Expand Down Expand Up @@ -715,6 +717,7 @@ var (
upgradeToVer88,
upgradeToVer89,
upgradeToVer90,
upgradeToVer92,
}
)

Expand Down Expand Up @@ -1847,6 +1850,13 @@ func upgradeToVer90(s Session, ver int64) {
valStr := variable.BoolToOnOff(config.GetGlobalConfig().EnableBatchDML)
importConfigOption(s, "enable-batch-dml", variable.TiDBEnableBatchDML, valStr)
}
func upgradeToVer92(s Session, ver int64) {
if ver >= version92 {
return
}
valStr := variable.BoolToOnOff(config.GetGlobalConfig().RunAutoAnalyze)
importConfigOption(s, "run-auto-analyze", variable.TiDBEnableAutoAnalyze, valStr)
}

func writeOOMAction(s Session) {
comment := "oom-action is `log` by default in v3.0.x, `cancel` by default in v4.0.11+"
Expand Down Expand Up @@ -1999,6 +2009,11 @@ func doDMLWorks(s Session) {
if v.Name == variable.TiDBEnableMutationChecker {
vVal = variable.On
}
if v.Name == variable.TiDBEnableAutoAnalyze {
if flag.Lookup("test.v") != nil || flag.Lookup("check.v") != nil {
vVal = variable.Off
}
}
if v.Name == variable.TiDBTxnAssertionLevel {
vVal = variable.AssertionFastStr
}
Expand Down
9 changes: 9 additions & 0 deletions sessionctx/variable/sysvar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,15 @@ var defaultSysVars = []*SysVar{
return nil
},
},
{Scope: ScopeGlobal, Name: TiDBEnableAutoAnalyze, Value: BoolToOnOff(DefTiDBEnableAutoAnalyze), skipInit: true, Type: TypeBool,
GetGlobal: func(s *SessionVars) (string, error) {
return BoolToOnOff(RunAutoAnalyze.Load()), nil
},
SetGlobal: func(s *SessionVars, val string) error {
RunAutoAnalyze.Store(TiDBOptOn(val))
return nil
},
},
{Scope: ScopeGlobal, Name: TiDBEnableColumnTracking, Value: BoolToOnOff(DefTiDBEnableColumnTracking), skipInit: true, Type: TypeBool, GetGlobal: func(s *SessionVars) (string, error) {
return BoolToOnOff(EnableColumnTracking.Load()), nil
}, SetGlobal: func(s *SessionVars, val string) error {
Expand Down
2 changes: 1 addition & 1 deletion sessionctx/variable/sysvar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ func TestSettersandGetters(t *testing.T) {
case TiDBEnableChangeMultiSchema, TiDBDDLReorgBatchSize,
TiDBMaxDeltaSchemaCount, InitConnect, MaxPreparedStmtCount,
TiDBDDLReorgWorkerCount, TiDBDDLErrorCountLimit, TiDBRowFormatVersion,
TiDBEnableTelemetry, TiDBEnablePointGetCache:
TiDBEnableTelemetry, TiDBEnablePointGetCache, TiDBEnableAutoAnalyze:
continue
}
require.Nil(t, sv.SetSession)
Expand Down
4 changes: 4 additions & 0 deletions sessionctx/variable/tidb_vars.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,8 @@ const (
TiDBStatsCacheMemQuota = "tidb_stats_cache_mem_quota"
// TiDBMemQuotaAnalyze indicates the memory quota for all analyze jobs.
TiDBMemQuotaAnalyze = "tidb_mem_quota_analyze"
// TiDBEnableAutoAnalyze determines whether TiDB executes automatic analysis.
TiDBEnableAutoAnalyze = "tidb_enable_auto_analyze"
)

// TiDB intentional limits
Expand Down Expand Up @@ -857,11 +859,13 @@ const (
DefTiDBQueryLogMaxLen = 4096
DefTiDBBatchDMLIgnoreError = false
DefTiDBMemQuotaAnalyze = -1
DefTiDBEnableAutoAnalyze = true
)

// Process global variables.
var (
ProcessGeneralLog = atomic.NewBool(false)
RunAutoAnalyze = atomic.NewBool(DefTiDBEnableAutoAnalyze)
GlobalLogMaxDays = atomic.NewInt32(int32(config.GetGlobalConfig().Log.File.MaxDays))
QueryLogMaxLen = atomic.NewInt32(DefTiDBQueryLogMaxLen)
EnablePProfSQLCPU = atomic.NewBool(false)
Expand Down
2 changes: 0 additions & 2 deletions statistics/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"testing"

"github.com/pingcap/failpoint"
"github.com/pingcap/tidb/domain"
"github.com/pingcap/tidb/parser/model"
"github.com/pingcap/tidb/statistics"
"github.com/pingcap/tidb/statistics/handle"
Expand Down Expand Up @@ -499,7 +498,6 @@ func TestAnalyzeLongString(t *testing.T) {
}

func TestOutdatedStatsCheck(t *testing.T) {
domain.RunAutoAnalyze = false
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Forked to #34792

store, dom, clean := testkit.CreateMockStoreAndDomain(t)
defer clean()
tk := testkit.NewTestKit(t, store)
Expand Down
17 changes: 0 additions & 17 deletions statistics/selectivity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ import (
)

func TestCollationColumnEstimate(t *testing.T) {
domain.RunAutoAnalyze = false
store, dom, clean := testkit.CreateMockStoreAndDomain(t)
defer clean()
tk := testkit.NewTestKit(t, store)
Expand Down Expand Up @@ -74,7 +73,6 @@ func TestCollationColumnEstimate(t *testing.T) {
}

func BenchmarkSelectivity(b *testing.B) {
domain.RunAutoAnalyze = false
store, dom, clean := testkit.CreateMockStoreAndDomain(b)
defer clean()
testKit := testkit.NewTestKit(b, store)
Expand Down Expand Up @@ -113,7 +111,6 @@ func BenchmarkSelectivity(b *testing.B) {
}

func TestOutOfRangeEstimation(t *testing.T) {
domain.RunAutoAnalyze = false
store, dom, clean := testkit.CreateMockStoreAndDomain(t)
defer clean()
testKit := testkit.NewTestKit(t, store)
Expand Down Expand Up @@ -164,7 +161,6 @@ func TestOutOfRangeEstimation(t *testing.T) {
}

func TestEstimationForUnknownValues(t *testing.T) {
domain.RunAutoAnalyze = false
store, dom, clean := testkit.CreateMockStoreAndDomain(t)
defer clean()
testKit := testkit.NewTestKit(t, store)
Expand Down Expand Up @@ -243,7 +239,6 @@ func TestEstimationForUnknownValues(t *testing.T) {
}

func TestEstimationUniqueKeyEqualConds(t *testing.T) {
domain.RunAutoAnalyze = false
store, dom, clean := testkit.CreateMockStoreAndDomain(t)
defer clean()
testKit := testkit.NewTestKit(t, store)
Expand Down Expand Up @@ -277,7 +272,6 @@ func TestEstimationUniqueKeyEqualConds(t *testing.T) {
}

func TestPrimaryKeySelectivity(t *testing.T) {
domain.RunAutoAnalyze = false
store, clean := testkit.CreateMockStore(t)
defer clean()
testKit := testkit.NewTestKit(t, store)
Expand Down Expand Up @@ -306,7 +300,6 @@ func TestPrimaryKeySelectivity(t *testing.T) {
}

func TestStatsVer2(t *testing.T) {
domain.RunAutoAnalyze = false
store, clean := testkit.CreateMockStore(t)
defer clean()
testKit := testkit.NewTestKit(t, store)
Expand Down Expand Up @@ -375,7 +368,6 @@ func TestStatsVer2(t *testing.T) {
}

func TestTopNOutOfHist(t *testing.T) {
domain.RunAutoAnalyze = false
store, clean := testkit.CreateMockStore(t)
defer clean()
testKit := testkit.NewTestKit(t, store)
Expand Down Expand Up @@ -414,7 +406,6 @@ func TestTopNOutOfHist(t *testing.T) {
}

func TestColumnIndexNullEstimation(t *testing.T) {
domain.RunAutoAnalyze = false
store, dom, clean := testkit.CreateMockStoreAndDomain(t)
defer clean()
testKit := testkit.NewTestKit(t, store)
Expand Down Expand Up @@ -449,7 +440,6 @@ func TestColumnIndexNullEstimation(t *testing.T) {
}

func TestUniqCompEqualEst(t *testing.T) {
domain.RunAutoAnalyze = false
store, dom, clean := testkit.CreateMockStoreAndDomain(t)
defer clean()
testKit := testkit.NewTestKit(t, store)
Expand All @@ -476,7 +466,6 @@ func TestUniqCompEqualEst(t *testing.T) {
}

func TestSelectivity(t *testing.T) {
domain.RunAutoAnalyze = false
store, dom, clean := testkit.CreateMockStoreAndDomain(t)
defer clean()
testKit := testkit.NewTestKit(t, store)
Expand Down Expand Up @@ -571,7 +560,6 @@ func TestSelectivity(t *testing.T) {
// TestDiscreteDistribution tests the estimation for discrete data distribution. This is more common when the index
// consists several columns, and the first column has small NDV.
func TestDiscreteDistribution(t *testing.T) {
domain.RunAutoAnalyze = false
store, clean := testkit.CreateMockStore(t)
defer clean()
testKit := testkit.NewTestKit(t, store)
Expand Down Expand Up @@ -602,7 +590,6 @@ func TestDiscreteDistribution(t *testing.T) {
}

func TestSelectCombinedLowBound(t *testing.T) {
domain.RunAutoAnalyze = false
store, clean := testkit.CreateMockStore(t)
defer clean()
testKit := testkit.NewTestKit(t, store)
Expand All @@ -629,7 +616,6 @@ func TestSelectCombinedLowBound(t *testing.T) {

// TestDNFCondSelectivity tests selectivity calculation with DNF conditions covered by using independence assumption.
func TestDNFCondSelectivity(t *testing.T) {
domain.RunAutoAnalyze = false
store, dom, clean := testkit.CreateMockStoreAndDomain(t)
defer clean()
testKit := testkit.NewTestKit(t, store)
Expand Down Expand Up @@ -699,7 +685,6 @@ func TestDNFCondSelectivity(t *testing.T) {
}

func TestIndexEstimationCrossValidate(t *testing.T) {
domain.RunAutoAnalyze = false
store, clean := testkit.CreateMockStore(t)
defer clean()
tk := testkit.NewTestKit(t, store)
Expand Down Expand Up @@ -729,7 +714,6 @@ func TestIndexEstimationCrossValidate(t *testing.T) {
}

func TestRangeStepOverflow(t *testing.T) {
domain.RunAutoAnalyze = false
store, dom, clean := testkit.CreateMockStoreAndDomain(t)
defer clean()
tk := testkit.NewTestKit(t, store)
Expand All @@ -748,7 +732,6 @@ func TestRangeStepOverflow(t *testing.T) {
}

func TestSmallRangeEstimation(t *testing.T) {
domain.RunAutoAnalyze = false
store, dom, clean := testkit.CreateMockStoreAndDomain(t)
defer clean()
testKit := testkit.NewTestKit(t, store)
Expand Down
2 changes: 0 additions & 2 deletions statistics/trace_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"encoding/json"
"testing"

"github.com/pingcap/tidb/domain"
"github.com/pingcap/tidb/infoschema"
"github.com/pingcap/tidb/parser"
plannercore "github.com/pingcap/tidb/planner/core"
Expand All @@ -32,7 +31,6 @@ import (
)

func TestTraceCE(t *testing.T) {
domain.RunAutoAnalyze = false
store, dom, clean := testkit.CreateMockStoreAndDomain(t)
defer clean()
tk := testkit.NewTestKit(t, store)
Expand Down
1 change: 0 additions & 1 deletion tidb-server/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,6 @@ func setGlobalVars() {
planReplayerGCLease := parseDuration(cfg.Performance.PlanReplayerGCLease)
session.SetPlanReplayerGCLease(planReplayerGCLease)
bindinfo.Lease = parseDuration(cfg.Performance.BindInfoLease)
domain.RunAutoAnalyze = cfg.Performance.RunAutoAnalyze
statistics.FeedbackProbability.Store(cfg.Performance.FeedbackProbability)
statistics.MaxQueryFeedbackCount.Store(int64(cfg.Performance.QueryFeedbackLimit))
statistics.RatioOfPseudoEstimate.Store(cfg.Performance.PseudoEstimateRatio)
Expand Down