session, statistics, util: fix data race of Handle.mu.ctx #33732

xuyifangreeneyes · 2022-04-06T09:11:08Z

What problem does this PR solve?

Issue Number: close #32867 #33001 #32822

Problem Summary:

We call (*Handle).execRestrictedSQL concurrently, which uses Handle.mu.ctx to fetch another session from session pool to execute internal SQL.

tidb/statistics/handle/handle.go

Lines 132 to 134 in bd8c710

    
           func (h *Handle) execRestrictedSQL(ctx context.Context, sql string, params ...interface{}) ([]chunk.Row, []*ast.ResultField, error) { 
        
           	return h.mu.ctx.(sqlexec.RestrictedSQLExecutor).ExecRestrictedSQL(ctx, []sqlexec.OptionFuncAlias{sqlexec.ExecOptionUseSessionPool}, sql, params...) 
        
           }

However, the method may cause data race in the following code.

tidb/session/session.go

Lines 1748 to 1764 in bd8c710

    
           return se, func() { 
        
           	se.sessionVars.AnalyzeVersion = prevStatsVer 
        
           	if err := se.sessionVars.SetSystemVar(variable.TiDBSnapshot, ""); err != nil { 
        
           		logutil.BgLogger().Error("set tidbSnapshot error", zap.Error(err)) 
        
           	} 
        
           	se.sessionVars.SnapshotInfoschema = nil 
        
           	if !execOption.IgnoreWarning { 
        
           		if se != nil && se.GetSessionVars().StmtCtx.WarningCount() > 0 { 
        
           			warnings := se.GetSessionVars().StmtCtx.GetWarnings() 
        
           			s.GetSessionVars().StmtCtx.AppendWarnings(warnings) 
        
           		} 
        
           	} 
        
           	se.sessionVars.PartitionPruneMode.Store(prePruneMode) 
        
           	se.sessionVars.OptimizerUseInvisibleIndexes = false 
        
           	se.sessionVars.InspectionTableCache = nil 
        
           	s.sysSessionPool().Put(tmp) 
        
           }, nil

At Line 1757, we append warnings for Handle.mu.ctx, which causes data race.

What is changed and how it works?

IMO it is better to make (*Handle).execRestrictedSQL inrelevant with Handle.mu.ctx.

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

None

ti-chi-bot · 2022-04-06T09:11:09Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

hawkingrei
qw4990

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.

xuyifangreeneyes · 2022-04-06T09:11:27Z

/cc @chrysan @hawkingrei

xuyifangreeneyes · 2022-04-06T09:32:02Z

/cc @tiancaiamao

XuHuaiyu · 2022-04-06T10:10:42Z

statistics/handle/handle.go

+	if err != nil {
+		return nil, nil, errors.Trace(err)
+	}
+	defer h.pool.Put(se)


Should we put this line before the if check?

IMO we should check error first. According to the implementation of (*sessionPool).Get, if the returned err is not nil, then the returned se must be nil. Besides, in the following code we also check error first and then do the Put operation.

tidb/session/session.go

Lines 1713 to 1765 in 37d86da

func (s *session) getInternalSession(execOption sqlexec.ExecOption) (*session, func(), error) {

tmp, err := s.sysSessionPool().Get()

if err != nil {

return nil, nil, errors.Trace(err)

}

se := tmp.(*session)

// The special session will share the `InspectionTableCache` with current session

// if the current session in inspection mode.

if cache := s.sessionVars.InspectionTableCache; cache != nil {

se.sessionVars.InspectionTableCache = cache

}

if ok := s.sessionVars.OptimizerUseInvisibleIndexes; ok {

se.sessionVars.OptimizerUseInvisibleIndexes = true

}

prePruneMode := se.sessionVars.PartitionPruneMode.Load()

if execOption.SnapshotTS != 0 {

se.sessionVars.SnapshotInfoschema, err = getSnapshotInfoSchema(s, execOption.SnapshotTS)

if err != nil {

return nil, nil, err

}

if err := se.sessionVars.SetSystemVar(variable.TiDBSnapshot, strconv.FormatUint(execOption.SnapshotTS, 10)); err != nil {

return nil, nil, err

}

}

prevStatsVer := se.sessionVars.AnalyzeVersion

if execOption.AnalyzeVer != 0 {

se.sessionVars.AnalyzeVersion = execOption.AnalyzeVer

}

// for analyze stmt we need let worker session follow user session that executing stmt.

se.sessionVars.PartitionPruneMode.Store(s.sessionVars.PartitionPruneMode.Load())

return se, func() {

se.sessionVars.AnalyzeVersion = prevStatsVer

if err := se.sessionVars.SetSystemVar(variable.TiDBSnapshot, ""); err != nil {

logutil.BgLogger().Error("set tidbSnapshot error", zap.Error(err))

}

se.sessionVars.SnapshotInfoschema = nil

if !execOption.IgnoreWarning {

if se != nil && se.GetSessionVars().StmtCtx.WarningCount() > 0 {

warnings := se.GetSessionVars().StmtCtx.GetWarnings()

s.GetSessionVars().StmtCtx.AppendWarnings(warnings)

}

}

se.sessionVars.PartitionPruneMode.Store(prePruneMode)

se.sessionVars.OptimizerUseInvisibleIndexes = false

se.sessionVars.InspectionTableCache = nil

s.sysSessionPool().Put(tmp)

}, nil

}

Should all internal sql executions using internal session pools ignore warnings?

Currently we don't check warnings when executing internal sql through (*Handle).execRestrictedSQL, (*Handle).execRestrictedSQLWithStatsVer and (*Handle).execRestrictedSQLWithSnapshot. Maybe we can ignore warnings for now and make improvements if later warnings are needed.

sre-bot · 2022-04-07T07:20:20Z

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

xuyifangreeneyes · 2022-04-11T02:35:17Z

/cc @winoros

xuyifangreeneyes · 2022-04-12T08:11:20Z

/cc @time-and-fate

qw4990 · 2022-04-13T06:53:51Z

statistics/handle/handle.go

+	return h.withRestrictedSQLExecutor(ctx, func(ctx context.Context, exec sqlexec.RestrictedSQLExecutor) ([]chunk.Row, []*ast.ResultField, error) {
+		return exec.ExecRestrictedSQL(ctx, []sqlexec.OptionFuncAlias{sqlexec.ExecOptionUseCurSession}, sql, params...)


It seems that we can decouple/detach execRestrictedSQL from Handle thoroughly in the future.

qw4990

LGTM

qw4990 · 2022-04-13T06:55:35Z

/merge

ti-chi-bot · 2022-04-13T06:55:38Z

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

Commit hash: 6f2e509

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2022-04-13T07:37:41Z

cherry pick to release-6.0 in PR #33919

purelind · 2022-04-13T08:24:22Z

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/integration-cdc-test	🟥 failed 12, success 22, total 34	27 min	New failing
idc-jenkins-ci-tidb/integration-br-test	🟥 failed 11, success 18, total 29	41 min	Existing failure
idc-jenkins-ci-tidb/code-coverage	✅ Lines coverage: 63.48%	13 min	Fixed
idc-jenkins-ci-tidb/integration-copr-test	✅ all 1 tests passed	12 min	Fixed
idc-jenkins-ci-tidb/common-test	✅ all 12 tests passed	10 min	Fixed
idc-jenkins-ci-tidb/integration-compatibility-test	✅ all 1 tests passed	10 min	Fixed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	19 min	Existing passed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 11 tests passed	14 min	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	9 min 4 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	8 min 30 sec	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	8 min 26 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 37 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

fix handle data race

9b63bfc

ti-chi-bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 6, 2022

ti-chi-bot requested review from chrysan and hawkingrei April 6, 2022 09:11

ti-chi-bot added needs-cherry-pick-6.0 and removed do-not-merge/needs-linked-issue labels Apr 6, 2022

fix

f887906

ti-chi-bot requested a review from tiancaiamao April 6, 2022 09:32

XuHuaiyu reviewed Apr 6, 2022

View reviewed changes

hawkingrei approved these changes Apr 6, 2022

View reviewed changes

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

Merge branch 'master' into handle-data-race

e870a77

Merge branch 'master' into handle-data-race

b26e6a1

ti-chi-bot requested a review from winoros April 11, 2022 02:35

ti-chi-bot requested a review from time-and-fate April 12, 2022 08:11

qw4990 reviewed Apr 13, 2022

View reviewed changes

qw4990 added the sig/planner SIG: Planner label Apr 13, 2022

qw4990 approved these changes Apr 13, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 13, 2022

Merge branch 'master' into handle-data-race

6f2e509

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

ti-chi-bot added 2 commits April 13, 2022 15:01

Merge branch 'master' into handle-data-race

5f65bcb

Merge branch 'master' into handle-data-race

31822f1

ti-chi-bot merged commit 9c836a5 into pingcap:master Apr 13, 2022

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Apr 13, 2022

cherry pick pingcap#33732 to release-6.0

950d708

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Apr 13, 2022

session, statistics, util: fix data race of Handle.mu.ctx (#33732) #33919

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

session, statistics, util: fix data race of Handle.mu.ctx #33732

session, statistics, util: fix data race of Handle.mu.ctx #33732

xuyifangreeneyes commented Apr 6, 2022 •

edited

Loading

ti-chi-bot commented Apr 6, 2022 •

edited

Loading

xuyifangreeneyes commented Apr 6, 2022

xuyifangreeneyes commented Apr 6, 2022

XuHuaiyu Apr 6, 2022

xuyifangreeneyes Apr 6, 2022

chrysan Apr 7, 2022

xuyifangreeneyes Apr 8, 2022

sre-bot commented Apr 7, 2022 •

edited

Loading

xuyifangreeneyes commented Apr 11, 2022

xuyifangreeneyes commented Apr 12, 2022

qw4990 Apr 13, 2022

qw4990 left a comment

qw4990 commented Apr 13, 2022

ti-chi-bot commented Apr 13, 2022

ti-srebot commented Apr 13, 2022

purelind commented Apr 13, 2022

	func (h Handle) execRestrictedSQL(ctx context.Context, sql string, params ...interface{}) ([]chunk.Row, []ast.ResultField, error) {
	return h.mu.ctx.(sqlexec.RestrictedSQLExecutor).ExecRestrictedSQL(ctx, []sqlexec.OptionFuncAlias{sqlexec.ExecOptionUseSessionPool}, sql, params...)
	}

	return se, func() {
	se.sessionVars.AnalyzeVersion = prevStatsVer
	if err := se.sessionVars.SetSystemVar(variable.TiDBSnapshot, ""); err != nil {
	logutil.BgLogger().Error("set tidbSnapshot error", zap.Error(err))
	}
	se.sessionVars.SnapshotInfoschema = nil
	if !execOption.IgnoreWarning {
	if se != nil && se.GetSessionVars().StmtCtx.WarningCount() > 0 {
	warnings := se.GetSessionVars().StmtCtx.GetWarnings()
	s.GetSessionVars().StmtCtx.AppendWarnings(warnings)
	}
	}
	se.sessionVars.PartitionPruneMode.Store(prePruneMode)
	se.sessionVars.OptimizerUseInvisibleIndexes = false
	se.sessionVars.InspectionTableCache = nil
	s.sysSessionPool().Put(tmp)
	}, nil

	func (s session) getInternalSession(execOption sqlexec.ExecOption) (session, func(), error) {
	tmp, err := s.sysSessionPool().Get()
	if err != nil {
	return nil, nil, errors.Trace(err)
	}
	se := tmp.(*session)

	// The special session will share the `InspectionTableCache` with current session
	// if the current session in inspection mode.
	if cache := s.sessionVars.InspectionTableCache; cache != nil {
	se.sessionVars.InspectionTableCache = cache
	}
	if ok := s.sessionVars.OptimizerUseInvisibleIndexes; ok {
	se.sessionVars.OptimizerUseInvisibleIndexes = true
	}
	prePruneMode := se.sessionVars.PartitionPruneMode.Load()

	if execOption.SnapshotTS != 0 {
	se.sessionVars.SnapshotInfoschema, err = getSnapshotInfoSchema(s, execOption.SnapshotTS)
	if err != nil {
	return nil, nil, err
	}
	if err := se.sessionVars.SetSystemVar(variable.TiDBSnapshot, strconv.FormatUint(execOption.SnapshotTS, 10)); err != nil {
	return nil, nil, err
	}
	}

	prevStatsVer := se.sessionVars.AnalyzeVersion
	if execOption.AnalyzeVer != 0 {
	se.sessionVars.AnalyzeVersion = execOption.AnalyzeVer
	}

	// for analyze stmt we need let worker session follow user session that executing stmt.
	se.sessionVars.PartitionPruneMode.Store(s.sessionVars.PartitionPruneMode.Load())

	return se, func() {
	se.sessionVars.AnalyzeVersion = prevStatsVer
	if err := se.sessionVars.SetSystemVar(variable.TiDBSnapshot, ""); err != nil {
	logutil.BgLogger().Error("set tidbSnapshot error", zap.Error(err))
	}
	se.sessionVars.SnapshotInfoschema = nil
	if !execOption.IgnoreWarning {
	if se != nil && se.GetSessionVars().StmtCtx.WarningCount() > 0 {
	warnings := se.GetSessionVars().StmtCtx.GetWarnings()
	s.GetSessionVars().StmtCtx.AppendWarnings(warnings)
	}
	}
	se.sessionVars.PartitionPruneMode.Store(prePruneMode)
	se.sessionVars.OptimizerUseInvisibleIndexes = false
	se.sessionVars.InspectionTableCache = nil
	s.sysSessionPool().Put(tmp)
	}, nil
	}

		return h.withRestrictedSQLExecutor(ctx, func(ctx context.Context, exec sqlexec.RestrictedSQLExecutor) ([]chunk.Row, []*ast.ResultField, error) {
		return exec.ExecRestrictedSQL(ctx, []sqlexec.OptionFuncAlias{sqlexec.ExecOptionUseCurSession}, sql, params...)

session, statistics, util: fix data race of Handle.mu.ctx #33732

session, statistics, util: fix data race of Handle.mu.ctx #33732

Conversation

xuyifangreeneyes commented Apr 6, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 6, 2022 • edited Loading

xuyifangreeneyes commented Apr 6, 2022

xuyifangreeneyes commented Apr 6, 2022

XuHuaiyu Apr 6, 2022

Choose a reason for hiding this comment

xuyifangreeneyes Apr 6, 2022

Choose a reason for hiding this comment

chrysan Apr 7, 2022

Choose a reason for hiding this comment

xuyifangreeneyes Apr 8, 2022

Choose a reason for hiding this comment

sre-bot commented Apr 7, 2022 • edited Loading

xuyifangreeneyes commented Apr 11, 2022

xuyifangreeneyes commented Apr 12, 2022

qw4990 Apr 13, 2022

Choose a reason for hiding this comment

qw4990 left a comment

Choose a reason for hiding this comment

qw4990 commented Apr 13, 2022

ti-chi-bot commented Apr 13, 2022

ti-srebot commented Apr 13, 2022

purelind commented Apr 13, 2022

TiDB MergeCI notify

xuyifangreeneyes commented Apr 6, 2022 •

edited

Loading

ti-chi-bot commented Apr 6, 2022 •

edited

Loading

sre-bot commented Apr 7, 2022 •

edited

Loading