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

sessionctx/stmtctx: do not use copy-on-read for GetWarnings function #39742

Merged
merged 5 commits into from
Dec 9, 2022
+6 −13
Merged

sessionctx/stmtctx: do not use copy-on-read for GetWarnings function #39742

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
08487ed
sessionctx/stmtctx: do not use copy-on-read for GetWarnings function
tiancaiamao Dec 7, 2022
34f2c42
fix CI
tiancaiamao Dec 8, 2022
4378fbd
Merge branch 'master' into issue-39702
tiancaiamao Dec 9, 2022
1688e68
Merge branch 'master' into issue-39702
hawkingrei Dec 9, 2022
397c190
Merge branch 'master' into issue-39702
ti-chi-bot Dec 9, 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
19 changes: 6 additions & 13 deletions sessionctx/stmtctx/stmtctx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,6 @@ type StatementContext struct {

message string
warnings []SQLWarn
errorCount uint16
execDetails execdetails.ExecDetails
allExecDetails []*execdetails.DetailsNeedP90
}
Expand Down Expand Up @@ -730,9 +729,7 @@ func (sc *StatementContext) SetMessage(msg string) {
func (sc *StatementContext) GetWarnings() []SQLWarn {
sc.mu.Lock()
defer sc.mu.Unlock()
warns := make([]SQLWarn, len(sc.mu.warnings))
copy(warns, sc.mu.warnings)
return warns
return sc.mu.warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

Will return warnings directly cause conflict?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code date back to very early commit:

5230f5eabd7 (Ewan Chou    2016-11-29 14:25:38 +0800 407) // GetWarnings gets warnings.
5230f5eabd7 (Ewan Chou    2016-11-29 14:25:38 +0800 408) func (sc *StatementContext) GetWarnings() []error {
5230f5eabd7 (Ewan Chou    2016-11-29 14:25:38 +0800 409)        sc.mu.Lock()
5230f5eabd7 (Ewan Chou    2016-11-29 14:25:38 +0800 410)        warns := make([]error, len(sc.mu.warnings))
5230f5eabd7 (Ewan Chou    2016-11-29 14:25:38 +0800 411)        copy(warns, sc.mu.warnings)
5230f5eabd7 (Ewan Chou    2016-11-29 14:25:38 +0800 412)        sc.mu.Unlock()
5230f5eabd7 (Ewan Chou    2016-11-29 14:25:38 +0800 413)        return warns
5230f5eabd7 (Ewan Chou    2016-11-29 14:25:38 +0800 414) }

The commit is sooooooo old that it's hard to figure out why it's written like that.

The change should work, if not, at least we can see where the data race happen and try to fix again.
@Defined2014

Copy link
Contributor

Choose a reason for hiding this comment

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

without copy, does the lock necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without copy, does the lock necessary here?

I think so ... GetWarnings / SetWarnings may be visited by many different component, maybe concurrently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. The value of sc.mu.warnings will be changed by SetWarnings.

}

// TruncateWarnings truncates warnings begin from start and returns the truncated warnings.
Expand Down Expand Up @@ -763,7 +760,11 @@ func (sc *StatementContext) WarningCount() uint16 {
func (sc *StatementContext) NumErrorWarnings() (ec uint16, wc int) {
sc.mu.Lock()
defer sc.mu.Unlock()
ec = sc.mu.errorCount
for _, w := range sc.mu.warnings {
if w.Level == WarnLevelError {
ec++
}
}
wc = len(sc.mu.warnings)
return
}
Expand All @@ -773,12 +774,6 @@ func (sc *StatementContext) SetWarnings(warns []SQLWarn) {
sc.mu.Lock()
defer sc.mu.Unlock()
sc.mu.warnings = warns
sc.mu.errorCount = 0
for _, w := range warns {
if w.Level == WarnLevelError {
sc.mu.errorCount++
}
}
}

// AppendWarning appends a warning with level 'Warning'.
Expand Down Expand Up @@ -814,7 +809,6 @@ func (sc *StatementContext) AppendError(warn error) {
defer sc.mu.Unlock()
if len(sc.mu.warnings) < math.MaxUint16 {
sc.mu.warnings = append(sc.mu.warnings, SQLWarn{WarnLevelError, warn})
sc.mu.errorCount++
}
}

Expand Down Expand Up @@ -860,7 +854,6 @@ func (sc *StatementContext) resetMuForRetry() {
sc.mu.copied = 0
sc.mu.touched = 0
sc.mu.message = ""
sc.mu.errorCount = 0
sc.mu.warnings = nil
sc.mu.execDetails = execdetails.ExecDetails{}
sc.mu.allExecDetails = make([]*execdetails.DetailsNeedP90, 0, 4)
Expand Down