-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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.
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
conn: fix lost connection when insert from select panic cause by out of memory quota #12090
conn: fix lost connection when insert from select panic cause by out of memory quota #12090
Conversation
@zz-jason @tiancaiamao PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov Report
@@ Coverage Diff @@
## master #12090 +/- ##
===========================================
Coverage 81.6432% 81.6432%
===========================================
Files 449 449
Lines 96624 96624
===========================================
Hits 78887 78887
Misses 12169 12169
Partials 5568 5568 |
session/session.go
Outdated
buf := make([]byte, 4096) | ||
stackSize := runtime.Stack(buf, false) | ||
buf = buf[:stackSize] | ||
logutil.Logger(ctx).Error("execute sql panic", zap.String("sql", sql), zap.String("stack", string(buf))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer using zap.Stack()
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, done.
session/session.go
Outdated
if r == nil { | ||
return | ||
} | ||
if str, ok := r.(string); !ok || !strings.HasPrefix(str, memory.PanicMemoryExceed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better place is ExecStmt.Exec()
function, or ExecStmt.handleNoDelay()
and ExecStmt.handlePessimisticSelectForUpdate()
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
/run-all-tests |
/run-common-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/rebuild |
cherry pick to release-3.0 failed |
cherry pick to release-2.1 failed |
What problem does this PR solve?
Set
oom-action = "cancel"
in the tidb config file.When
insert ... select
out of memory quota, TiDB should return errorOut Of Memory Quota
, instead of lost connection.What is changed and how it works?
Check List
Tests
Code changes
Side effects
Related changes
Release note
insert from select
statement panic caused by out of memory quota.