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

executor: fix point get -1 return max.uInt64 value #10113

Merged
merged 2 commits into from
May 22, 2019
Merged

executor: fix point get -1 return max.uInt64 value #10113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e92d4b6
executor: fix point get -1 return max.UInt64 value
lysu May 14, 2019
29a2074
Merge branch 'master' into dev-fix-point-get-big-unsign-handle
zz-jason May 22, 2019
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
1 change: 1 addition & 0 deletions executor/point_get.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func (b *executorBuilder) buildPointGet(p *plannercore.PointGetPlan) Executor {
idxVals: p.IndexValues,
handle: p.Handle,
startTS: startTS,
done: p.UnsignedHandle && p.Handle < 0,
Copy link
Member

Choose a reason for hiding this comment

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

What's this?

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 make PointGetExecutor's Next return no result, we cannot use tabledual, this just like tabedual, and no need add more flag- -

}
}

Expand Down
14 changes: 8 additions & 6 deletions planner/core/point_get_plan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ type PointGetPlan struct {
IndexInfo *model.IndexInfo
Handle int64
HandleParam *driver.ParamMarkerExpr
UnsignedHandle bool
IndexValues []types.Datum
IndexValueParams []*driver.ParamMarkerExpr
expr expression.Expression
Expand Down Expand Up @@ -185,7 +186,7 @@ func tryPointGetPlan(ctx sessionctx.Context, selStmt *ast.SelectStmt) *PointGetP
if pairs == nil {
return nil
}
handlePair := findPKHandle(tbl, pairs)
handlePair, unsigned := findPKHandle(tbl, pairs)
if handlePair.value.Kind() != types.KindNull && len(pairs) == 1 {
schema := buildSchemaFromFields(ctx, tblName.Schema, tbl, selStmt.Fields.Fields)
if schema == nil {
Expand All @@ -197,6 +198,7 @@ func tryPointGetPlan(ctx sessionctx.Context, selStmt *ast.SelectStmt) *PointGetP
if err != nil {
return nil
}
p.UnsignedHandle = unsigned
p.HandleParam = handlePair.param
return p
}
Expand Down Expand Up @@ -362,20 +364,20 @@ func getNameValuePairs(nvPairs []nameValuePair, expr ast.ExprNode) []nameValuePa
return nil
}

func findPKHandle(tblInfo *model.TableInfo, pairs []nameValuePair) (handlePair nameValuePair) {
func findPKHandle(tblInfo *model.TableInfo, pairs []nameValuePair) (handlePair nameValuePair, unsigned bool) {
if !tblInfo.PKIsHandle {
return handlePair
return handlePair, unsigned
}
for _, col := range tblInfo.Columns {
if mysql.HasPriKeyFlag(col.Flag) {
i := findInPairs(col.Name.L, pairs)
if i == -1 {
return handlePair
return handlePair, unsigned
}
return pairs[i]
return pairs[i], mysql.HasUnsignedFlag(col.Flag)
}
}
return handlePair
return handlePair, unsigned
}

func getIndexValues(idxInfo *model.IndexInfo, pairs []nameValuePair) ([]types.Datum, []*driver.ParamMarkerExpr) {
Expand Down
16 changes: 15 additions & 1 deletion planner/core/point_get_plan_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func (s *testPointGetSuite) TestPointGetPlanCache(c *C) {
core.PreparedPlanCacheMaxMemory.Store(math.MaxUint64)
tk.MustExec("use test")
tk.MustExec("drop table if exists t")
tk.MustExec("create table t(a int primary key, b int, c int, key idx_bc(b,c))")
tk.MustExec("create table t(a bigint unsigned primary key, b int, c int, key idx_bc(b,c))")
tk.MustExec("insert into t values(1, 1, 1), (2, 2, 2), (3, 3, 3)")
tk.MustQuery("explain select * from t where a = 1").Check(testkit.Rows(
"Point_Get_1 1.00 root table:t, handle:1",
Expand All @@ -68,6 +68,11 @@ func (s *testPointGetSuite) TestPointGetPlanCache(c *C) {
tk.MustQuery("explain delete from t where a = 1").Check(testkit.Rows(
"Point_Get_1 1.00 root table:t, handle:1",
))
tk.MustQuery("explain select a from t where a = -1").Check(testkit.Rows(
"TableDual_5 0.00 root rows:0"))
tk.MustExec(`prepare stmt0 from "select a from t where a = ?"`)
tk.MustExec("set @p0 = -1")
tk.MustQuery("execute stmt0 using @p0").Check(testkit.Rows())
metrics.ResettablePlanCacheCounterFortTest = true
metrics.PlanCacheCounter.Reset()
counter := metrics.PlanCacheCounter.WithLabelValues("prepare")
Expand Down Expand Up @@ -137,4 +142,13 @@ func (s *testPointGetSuite) TestPointGetPlanCache(c *C) {
counter.Write(pb)
hit = pb.GetCounter().GetValue()
c.Check(hit, Equals, float64(2))
tk.MustExec("insert into t (a, b, c) values (18446744073709551615, 4, 4)")
tk.MustExec("set @p1=-1")
tk.MustExec("set @p2=1")
tk.MustExec(`prepare stmt7 from "select a from t where a = ?"`)
tk.MustQuery("execute stmt7 using @p1").Check(testkit.Rows())
tk.MustQuery("execute stmt7 using @p2").Check(testkit.Rows("1"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This test case cannot test PointGet plan actually? the plan generated using @p1 is table_scan + selection, not point get, so @p2 actually reuses this table_scan + selection plan?

Copy link
Contributor Author

@lysu lysu Apr 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@eurekaka oh...... this logic is magic...

This test case cannot test PointGet plan actually? the plan generated using @p1 is table_scan + selection, not point get, so @p2 actually reuses this table_scan + selection plan?

In my test this can use point-get in two executes, because prepare use ? instead of -1 or 1, so getNameValuePair can handle ParramMarkExpr, then prepare success a point-get and execute as point-get.

mysql> show create table t1;
+-------+------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                                                               |
+-------+------------------------------------------------------------------------------------------------------------------------------------------------------------+
| t1    | CREATE TABLE `t1` (
  `a` bigint(20) NOT NULL,
  `b` int(11) DEFAULT NULL,
  PRIMARY KEY (`a`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |
+-------+------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

mysql> explain select * from t1 where a = -1;                                                                                                                                                                                                                                                                     +-------------------+-------+------+---------------------------------------------------------+
| id                | count | task | operator info                                           |
+-------------------+-------+------+---------------------------------------------------------+
| TableReader_6     | 1.00  | root | data:TableScan_5                                        |
| └─TableScan_5     | 1.00  | cop  | table:t1, range:[-1,-1], keep order:false, stats:pseudo |
+-------------------+-------+------+---------------------------------------------------------+
2 rows in set (0.01 sec)

mysql> prepare st from "explain select * from t1 where a = ?";
Query OK, 0 rows affected (0.00 sec)

mysql> set @x=-1;
Query OK, 0 rows affected (0.00 sec)

mysql> execute st using @x;                                                                                                                                                                                                                                                                                    
+-------------+-------+------+---------------------+
| id          | count | task | operator info       |
+-------------+-------+------+---------------------+
| Point_Get_1 | 1.00  | root | table:t1, handle:-1 |
+-------------+-------+------+---------------------+
1 row in set (0.00 sec)

same as the first case, but handle column is int64, would the second execute report error?

so for this case:

same as the first case, but handle column is int64, would the second execute report error?

maybe luck is just fine

handle column is unsigned, if the first execute is pk = 1, and the second execute is like pk = 18446744073709551615, since we are using int64 as handle type in PointGet, would the second execute cause overflow and wrong result?

will ok for both use point-get plan, but there are another question, we keep param as string datum and try to cast it to int64, so will got a overflow question, it seems not easy to fix so it take another issue in here #10111.

🤣 it's very stranger -1 + int64 cannot use plan cache, and prepare and normal query use different plan..

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the root cause of these problems is that we are using int64 to store the handle in PointGet, while it can be unsigned.

counter.Write(pb)
hit = pb.GetCounter().GetValue()
c.Check(hit, Equals, float64(3))
}