-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
*: add memory tracker for InsertExec and ReplaceExec #14179
Conversation
fb6ab45
to
1bb2640
Compare
/run-unit-test |
/re-run |
session/txn.go
Outdated
@@ -455,6 +461,9 @@ func (s *session) StmtCommit() error { | |||
st.doNotCommit = err | |||
return err | |||
} | |||
if memTracker != nil { | |||
memTracker.Consume(int64(st.Transaction.Size())) |
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.
for situation like:
begin;
insert1
insert2
commit;
insert1 will add 1k to transaction buffer, insert2 will add 1k to transaction buffer.
so when insert2 step in here will +3k at here..
it's hard to track transaction memory in stmt level if one transaction have multiple statements. - -
@@ -431,7 +437,7 @@ func (s *session) getTxnFuture(ctx context.Context) *txnFuture { | |||
} | |||
|
|||
// StmtCommit implements the sessionctx.Context interface. | |||
func (s *session) StmtCommit() error { | |||
func (s *session) StmtCommit(memTracker *memory.Tracker) error { | |||
defer s.txn.cleanup() |
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.
if we follow code logic, this defer maybe need memTracker.Consume(-int64(txn.Size()))
, before this line we have put all data from stmt buffer into transaction buffer, and this line will cleanup stmt buffer
executor/replace.go
Outdated
@@ -202,6 +207,7 @@ func (e *ReplaceExec) exec(ctx context.Context, newRows [][]types.Datum) error { | |||
return err | |||
} | |||
} | |||
memTracker.Consume(int64(txn.Size())) |
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.
this also have potential be duplicate add consume amount if user use batch insert(although we are not recommend use batch insert)
exec
be executed for each batch, so second batch will see first batch's memory still in stmt buffer
927ab7c
to
ab3258d
Compare
ab3258d
to
46e698d
Compare
/run-all-tests |
/run-all-tests |
@@ -431,9 +437,13 @@ func (s *session) getTxnFuture(ctx context.Context) *txnFuture { | |||
} | |||
|
|||
// StmtCommit implements the sessionctx.Context interface. | |||
func (s *session) StmtCommit() error { | |||
defer s.txn.cleanup() | |||
func (s *session) StmtCommit(memTracker *memory.Tracker) error { |
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.
The statement is finished and the memory is allocated when StmtCommit
, I think it is meanless. Consume it at
Line 260 in d666ed4
return st.buf.Set(k, v) |
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.
But stmtBuffer can be GCed after be written to txnBuffer?
/run-unit-test |
…into trace-write-memory
/run-unit-test |
/run-unit-test |
@@ -220,6 +232,8 @@ func insertRows(ctx context.Context, base insertCommon) (err error) { | |||
} | |||
rows = append(rows, row) | |||
if batchInsert && e.rowCount%uint64(batchSize) == 0 { | |||
memUsageOfRows = types.EstimatedMemUsage(rows[0], len(rows)) | |||
memTracker.Consume(memUsageOfRows) |
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.
For LoadDataExec, here is nil pointer??
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.
LoadDataExec
will not touch here.
/run-all-tests |
LGTM |
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
What problem does this PR solve?
Before this commit, the memory usage of InsertExec and ReplaceExec is not traced.
For example,
we set the MemQuotaQuery in config.go to 32bytes.
We run the following SQLs, and check the tidb.log
From the tidb.log, we can ONLY got the out of memory message of the second sql, because that the second sql will read data from a TableReader, and the memory usage of the TableReader is already traced
After this commit, we can get the out-of-memory message of the 2 SQLs:
What is changed and how it works?
Check List
Tests
Code changes
Side effects
N/A
Related changes
N/A
Release note
N/A