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

planner: converting to TableDual only when the empty range is not derived from parameterized constants (#7579) #7808

Merged
merged 7 commits into from
Oct 16, 2018
Merged

planner: converting to TableDual only when the empty range is not derived from parameterized constants (#7579) #7808

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
11d5f30
planner: converting to TableDual only when the empty range is not der…
dbjoa Sep 28, 2018
e37cbf0
Merge branch 'master' into fix-issue7579
eurekaka Sep 29, 2018
3c1522d
Merge branch 'master' into fix-issue7579
zz-jason Oct 16, 2018
d69a043
Merge branch 'master' into fix-issue7579
eurekaka Oct 16, 2018
77b76d0
Merge branch 'master' into fix-issue7579
eurekaka Oct 16, 2018
dad6c20
Merge branch 'master' into fix-issue7579
eurekaka Oct 16, 2018
b5c01c0
Merge branch 'master' into fix-issue7579
eurekaka Oct 16, 2018
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
43 changes: 43 additions & 0 deletions executor/prepared_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,3 +322,46 @@ func generateBatchSQL(paramCount int) (sql string, paramSlice []interface{}) {
}
return "insert into t values " + strings.Join(placeholders, ","), params
}

func (s *testSuite) TestPreparedIssue7579(c *C) {
orgEnable := plannercore.PreparedPlanCacheEnabled()
orgCapacity := plannercore.PreparedPlanCacheCapacity
defer func() {
plannercore.SetPreparedPlanCache(orgEnable)
plannercore.PreparedPlanCacheCapacity = orgCapacity
}()
flags := []bool{false, true}
for _, flag := range flags {
plannercore.SetPreparedPlanCache(flag)
plannercore.PreparedPlanCacheCapacity = 100
tk := testkit.NewTestKit(c, s.store)
tk.MustExec("use test")
tk.MustExec("drop table if exists t")
tk.MustExec("create table t (a int, b int, index a_idx(a))")
tk.MustExec("insert into t values (1,1), (2,2), (null,3)")

r := tk.MustQuery("select a, b from t order by b asc;")
r.Check(testkit.Rows("1 1", "2 2", "<nil> 3"))

tk.MustExec(`prepare stmt from 'select a, b from t where ? order by b asc'`)

r = tk.MustQuery(`execute stmt using @param;`)
r.Check(nil)

tk.MustExec(`set @param = true`)
r = tk.MustQuery(`execute stmt using @param;`)
r.Check(testkit.Rows("1 1", "2 2", "<nil> 3"))

tk.MustExec(`set @param = false`)
r = tk.MustQuery(`execute stmt using @param;`)
r.Check(nil)

tk.MustExec(`set @param = 1`)
r = tk.MustQuery(`execute stmt using @param;`)
r.Check(testkit.Rows("1 1", "2 2", "<nil> 3"))

tk.MustExec(`set @param = 0`)
r = tk.MustQuery(`execute stmt using @param;`)
r.Check(nil)
}
}
2 changes: 1 addition & 1 deletion planner/core/find_best_task.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ func (ds *DataSource) tryToGetMemTask(prop *property.PhysicalProperty) (task tas
// tryToGetDualTask will check if the push down predicate has false constant. If so, it will return table dual.
func (ds *DataSource) tryToGetDualTask() (task, error) {
for _, cond := range ds.pushedDownConds {
if _, ok := cond.(*expression.Constant); ok {
if con, ok := cond.(*expression.Constant); ok && con.DeferredExpr == nil {
result, err := expression.EvalBool(ds.ctx, []expression.Expression{cond}, chunk.Row{})
if err != nil {
return nil, errors.Trace(err)
Expand Down
2 changes: 1 addition & 1 deletion planner/core/logical_plan_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ func (b *planBuilder) buildSelection(p LogicalPlan, where ast.ExprNode, AggMappe
}
cnfItems := expression.SplitCNFItems(expr)
for _, item := range cnfItems {
if con, ok := item.(*expression.Constant); ok {
if con, ok := item.(*expression.Constant); ok && con.DeferredExpr == nil {
Copy link
Contributor

@eurekaka eurekaka Sep 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I have 2 more concerns regarding this change:

  • Shall we add UseCache check here to allow building TableDual when plan cache is not used for this query?
  • There may exist more hidden cases we need to handle, for example, in planBuilder::buildLimit, we return TableDual when offset+count=0, what if offset or count is from parameter setting? will it still break plan cache(just checked the code logic, not experimentally verified yet)?

The key point of this problem is the contradiction between optimizer and plan cache. From optimizer perspective, we should optimize the query as much as we can to get a faster plan, thus we should be aggressive, while from plan cache perspective, we should generate a general plan to be reused by different parameters, that is to say, we should be conservative.

Anyway, it is good we are starting fixing this at least, and thanks for this contribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Adding UseCache is technically possible, but it can not replace the code to check whether or not a constant is parameterized since prepared statements can have the parameterized constants or the non-parameterized ones. That is, 'UseCache == False" is the condition to guarantee all 'DeferredExpr == nil', but 'UseCache == True' is not the condition to guarantee all 'DeferredExpr != nil'.

  2. If a prepared statement has a limit expression, it is not cached(see the code planner/core/cacheable_checker.go). Thus, TableDual will be generated without breaking the plan cache.

  3. To address the optimization issue, we can extend the plan cache to maintain plans according to the parameter ranges. In addition, the optimizer is required to be extended to generate general plans for the given parameters' ranges.

Copy link
Contributor

Choose a reason for hiding this comment

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

but it can not replace the code to check whether or not a constant is parameterized since prepared statements can have the parameterized constants or the non-parameterized ones

I mean we should use condition like:

if con, ok := item.(*expression.Constant); ok && (con.DeferredExpr == nil || !xxx.UseCache)

otherwise, when plan cache is disabled, we indeed should return TableDual, but we don't.

If a prepared statement has a limit expression, it is not cached(see the code planner/core/cacheable_checker.go).

Got it, thanks.

Copy link
Member

Choose a reason for hiding this comment

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

we only need to check con.DeferredExpr, because when con.DeferredExpr != nil, UseCache should always be true.

ret, err := expression.EvalBool(b.ctx, expression.CNFExprs{con}, chunk.Row{})
if err != nil || ret {
continue
Expand Down