-
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
plan: use point get plan for UPDATE statement #7586
Conversation
/run-all-tests |
@jackysp @tiancaiamao PTAL |
any benchmark results? |
@zz-jason will be provided later. |
/run-unit-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
@@ -95,6 +95,7 @@ func (*testSuite) TestSchemaValidator(c *C) { | |||
c.Assert(valid, Equals, ResultUnknown) | |||
|
|||
close(exit) | |||
time.Sleep(time.Millisecond) |
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.
What is this for?
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.
Wait for the goroutine to quit to pass leak test.
plan/point_get_plan.go
Outdated
if fastSelect == nil { | ||
return nil | ||
} | ||
checkFastPlanPrivilege(ctx, fastSelect, mysql.SelectPriv, mysql.UpdatePriv) |
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.
Check the result?
@zz-jason |
/run-all-tests |
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
tp := types.NewFieldType(mysql.TypeUnspecified) | ||
types.DefaultParamTypeForValue(v.GetValue(), tp) | ||
value := &Constant{Value: v.Datum, RetType: tp} | ||
sr.push(value) |
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 am confused about this newly added modification. Since we convert ParamMakerExpr to Constant here, what if we execute
the prepared statement with another parameter? will it still use the previous constant instead of the parameter we provide?
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.
We rebuild the plan for every execution.
And each execution updates the value in the ParamMakerExpr
.
What problem does this PR solve?
Optmize single row update statement.
What is changed and how it works?
Use point-get plan for
UPDATE
statement with a unique key equal condition.Check List
Tests
Related changes