From 74978fad9cc0f569bc635b60273f24ba83090206 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Sun, 8 Oct 2023 17:02:50 +0800 Subject: [PATCH 1/6] planner: do not convert update to point get if the expr has sub-query --- pkg/parser/ast/expressions.go | 2 +- pkg/planner/core/plan_test.go | 16 +++++++ pkg/planner/core/point_get_plan.go | 76 ++++++++++++++++++++++++++++-- 3 files changed, 88 insertions(+), 6 deletions(-) diff --git a/pkg/parser/ast/expressions.go b/pkg/parser/ast/expressions.go index 8cdeacf7c74fe..8b8ee5e1fa8db 100644 --- a/pkg/parser/ast/expressions.go +++ b/pkg/parser/ast/expressions.go @@ -981,7 +981,7 @@ type ParamMarkerExpr interface { SetOrder(int) } -// ParenthesesExpr is the parentheses expression. +// ParenthesesExpr is the parentheses' expression. type ParenthesesExpr struct { exprNode // Expr is the expression in parentheses. diff --git a/pkg/planner/core/plan_test.go b/pkg/planner/core/plan_test.go index 9949574eb87db..456eb943af9de 100644 --- a/pkg/planner/core/plan_test.go +++ b/pkg/planner/core/plan_test.go @@ -755,6 +755,22 @@ func TestIssue40535(t *testing.T) { require.Empty(t, tk.Session().LastMessage()) } +func TestIssue47445(t *testing.T) { + store, _ := testkit.CreateMockStoreAndDomain(t) + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test;") + tk.MustExec("CREATE TABLE golang1 ( `fcbpdt` CHAR (8) COLLATE utf8_general_ci NOT NULL, `fcbpsq` VARCHAR (20) COLLATE utf8_general_ci NOT NULL, `procst` char (4) COLLATE utf8_general_ci DEFAULT NULL,`cipstx` VARCHAR (105) COLLATE utf8_general_ci DEFAULT NULL, `cipsst` CHAR (4) COLLATE utf8_general_ci DEFAULT NULL, `dyngtg` VARCHAR(4) COLLATE utf8_general_ci DEFAULT NULL, `blncdt` VARCHAR (8) COLLATE utf8_general_ci DEFAULT NULL, PRIMARY KEY ( fcbpdt, fcbpsq ))") + tk.MustExec("insert into golang1 values('20230925','12023092502158016','abc','','','','')") + tk.MustExec("create table golang2 (`sysgrp` varchar(20) NOT NULL,`procst` varchar(8) NOT NULL,`levlid` int(11) NOT NULL,PRIMARY key (procst));") + tk.MustExec("insert into golang2 VALUES('COMMON','ACSC',90)") + tk.MustExec("insert into golang2 VALUES('COMMON','abc',8)") + tk.MustExec("insert into golang2 VALUES('COMMON','CH02',6)") + tk.MustExec("UPDATE golang1 a SET procst =(CASE WHEN ( SELECT levlid FROM golang2 b WHERE b.sysgrp = 'COMMON' AND b.procst = 'ACSC' ) > ( SELECT levlid FROM golang2 c WHERE c.sysgrp = 'COMMON' AND c.procst = a.procst ) THEN 'ACSC' ELSE a.procst END ), cipstx = 'CI010000', cipsst = 'ACSC', dyngtg = 'EAYT', blncdt= '20230925' WHERE fcbpdt = '20230925' AND fcbpsq = '12023092502158016'") + tk.MustQuery("select * from golang1").Check(testkit.Rows("20230925 12023092502158016 ACSC CI010000 ACSC EAYT 20230925")) + tk.MustExec("UPDATE golang1 a SET procst= (SELECT 1 FROM golang2 c WHERE c.procst = a.procst) WHERE fcbpdt = '20230925' AND fcbpsq = '12023092502158016'") + tk.MustQuery("select * from golang1").Check(testkit.Rows("20230925 12023092502158016 1 CI010000 ACSC EAYT 20230925")) +} + func TestExplainValuesStatement(t *testing.T) { store, _ := testkit.CreateMockStoreAndDomain(t) tk := testkit.NewTestKit(t, store) diff --git a/pkg/planner/core/point_get_plan.go b/pkg/planner/core/point_get_plan.go index 29bc6464c2a9a..3a46f1443a8d4 100644 --- a/pkg/planner/core/point_get_plan.go +++ b/pkg/planner/core/point_get_plan.go @@ -1552,13 +1552,79 @@ func findInPairs(colName string, pairs []nameValuePair) int { return -1 } -func tryUpdatePointPlan(ctx sessionctx.Context, updateStmt *ast.UpdateStmt) Plan { - // avoid using the point_get when assignment_list contains the subquery in the UPDATE. - for _, list := range updateStmt.List { - if _, ok := list.Expr.(*ast.SubqueryExpr); ok { - return nil +func checkIfCaseExprHasSubQuery(expr ast.ExprNode) bool { + if c, ok := expr.(*ast.CaseExpr); ok { + for _, clause := range c.WhenClauses { + if b, ok := clause.Expr.(*ast.BinaryOperationExpr); ok { + if _, ok := b.L.(*ast.SubqueryExpr); ok { + return true + } + if _, ok := b.R.(*ast.SubqueryExpr); ok { + return true + } + } } } + + return false +} + +func checkIfParenthesesCaseExprHasSubQuery(expr ast.ExprNode) bool { + if pe, ok := expr.(*ast.ParenthesesExpr); ok { + return checkIfCaseExprHasSubQuery(pe.Expr) + } + return false +} + +func checkIfExprHasSubQuery(expr ast.ExprNode) bool { + if _, ok := expr.(*ast.SubqueryExpr); ok { + return true + } + + return false +} + +func checkIfParenthesesExprHasSubQuery(expr ast.ExprNode) bool { + if pe, ok := expr.(*ast.ParenthesesExpr); ok { + return checkIfExprHasSubQuery(pe.Expr) + } + return false +} + +func checkIfAssignmentListHasSubQuery(list []*ast.Assignment) bool { + for _, a := range list { + // For example: update t a set foo = (select bar from t1 b where b.id = a.id) where id = 1; + if checkIfParenthesesExprHasSubQuery(a.Expr) { + return true + } + // TODO: do we really check this case? Shouldn't this case be handled by the check above? + if checkIfExprHasSubQuery(a.Expr) { + return true + } + + // For example: update t a set foo = + // (case when (select bar from t1 b where b.id = a.id) > (select bar from t2 c where c.id = a.id) then 1 else 0 end) + // where id = 1; + if checkIfParenthesesCaseExprHasSubQuery(a.Expr) { + return true + } + // For example: update t a set foo = + // case when (select bar from t1 b where b.id = a.id) > (select bar from t2 c where c.id = a.id) then 1 else 0 end + // where id = 1; + if checkIfCaseExprHasSubQuery(a.Expr) { + return true + } + } + + return false +} + +func tryUpdatePointPlan(ctx sessionctx.Context, updateStmt *ast.UpdateStmt) Plan { + // Avoid using the point_get when assignment_list contains the sub-query in the UPDATE. + if checkIfAssignmentListHasSubQuery(updateStmt.List) { + return nil + } + selStmt := &ast.SelectStmt{ Fields: &ast.FieldList{}, From: updateStmt.TableRefs, From 93f6f3679071fbba19762881e247669e18e2cd0a Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Fri, 13 Oct 2023 17:38:55 +0800 Subject: [PATCH 2/6] planner: use visitor --- pkg/planner/core/point_get_plan.go | 69 ++++++++---------------------- 1 file changed, 17 insertions(+), 52 deletions(-) diff --git a/pkg/planner/core/point_get_plan.go b/pkg/planner/core/point_get_plan.go index 3a46f1443a8d4..0882cf5bdd16d 100644 --- a/pkg/planner/core/point_get_plan.go +++ b/pkg/planner/core/point_get_plan.go @@ -1552,70 +1552,35 @@ func findInPairs(colName string, pairs []nameValuePair) int { return -1 } -func checkIfCaseExprHasSubQuery(expr ast.ExprNode) bool { - if c, ok := expr.(*ast.CaseExpr); ok { - for _, clause := range c.WhenClauses { - if b, ok := clause.Expr.(*ast.BinaryOperationExpr); ok { - if _, ok := b.L.(*ast.SubqueryExpr); ok { - return true - } - if _, ok := b.R.(*ast.SubqueryExpr); ok { - return true - } - } - } - } - - return false +type subQueryChecker struct { + hasSubQuery bool } -func checkIfParenthesesCaseExprHasSubQuery(expr ast.ExprNode) bool { - if pe, ok := expr.(*ast.ParenthesesExpr); ok { - return checkIfCaseExprHasSubQuery(pe.Expr) +func (s *subQueryChecker) Enter(in ast.Node) (node ast.Node, skipChildren bool) { + switch in.(type) { + case *ast.SubqueryExpr: + s.hasSubQuery = true + return in, true } - return false -} -func checkIfExprHasSubQuery(expr ast.ExprNode) bool { - if _, ok := expr.(*ast.SubqueryExpr); ok { - return true - } + return in, false +} - return false +func (s *subQueryChecker) Leave(in ast.Node) (ast.Node, bool) { + // Before we enter the sub-query, we should keep visiting its children. + return in, !s.hasSubQuery } -func checkIfParenthesesExprHasSubQuery(expr ast.ExprNode) bool { - if pe, ok := expr.(*ast.ParenthesesExpr); ok { - return checkIfExprHasSubQuery(pe.Expr) - } - return false +func isExprHasSubQuery(expr ast.Node) bool { + checker := &subQueryChecker{} + expr.Accept(checker) + return checker.hasSubQuery } func checkIfAssignmentListHasSubQuery(list []*ast.Assignment) bool { for _, a := range list { - // For example: update t a set foo = (select bar from t1 b where b.id = a.id) where id = 1; - if checkIfParenthesesExprHasSubQuery(a.Expr) { - return true - } - // TODO: do we really check this case? Shouldn't this case be handled by the check above? - if checkIfExprHasSubQuery(a.Expr) { - return true - } - - // For example: update t a set foo = - // (case when (select bar from t1 b where b.id = a.id) > (select bar from t2 c where c.id = a.id) then 1 else 0 end) - // where id = 1; - if checkIfParenthesesCaseExprHasSubQuery(a.Expr) { - return true - } - // For example: update t a set foo = - // case when (select bar from t1 b where b.id = a.id) > (select bar from t2 c where c.id = a.id) then 1 else 0 end - // where id = 1; - if checkIfCaseExprHasSubQuery(a.Expr) { - return true - } + return isExprHasSubQuery(a) } - return false } From 1a1820d2959a09a8414aa7e97149bb6a70aee7c0 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Fri, 13 Oct 2023 18:02:14 +0800 Subject: [PATCH 3/6] planner: address comment --- pkg/planner/core/point_get_plan.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pkg/planner/core/point_get_plan.go b/pkg/planner/core/point_get_plan.go index 0882cf5bdd16d..dc84040aae36c 100644 --- a/pkg/planner/core/point_get_plan.go +++ b/pkg/planner/core/point_get_plan.go @@ -1557,8 +1557,11 @@ type subQueryChecker struct { } func (s *subQueryChecker) Enter(in ast.Node) (node ast.Node, skipChildren bool) { - switch in.(type) { - case *ast.SubqueryExpr: + if s.hasSubQuery { + return in, true + } + + if _, ok := in.(*ast.SubqueryExpr); ok { s.hasSubQuery = true return in, true } @@ -1579,7 +1582,9 @@ func isExprHasSubQuery(expr ast.Node) bool { func checkIfAssignmentListHasSubQuery(list []*ast.Assignment) bool { for _, a := range list { - return isExprHasSubQuery(a) + if isExprHasSubQuery(a) { + return true + } } return false } From 180c4cf8acecec90cf35b814c32054a337caad44 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Fri, 13 Oct 2023 18:25:10 +0800 Subject: [PATCH 4/6] planner: address comment --- pkg/planner/core/point_get_plan.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/planner/core/point_get_plan.go b/pkg/planner/core/point_get_plan.go index dc84040aae36c..a98827cf1346f 100644 --- a/pkg/planner/core/point_get_plan.go +++ b/pkg/planner/core/point_get_plan.go @@ -19,6 +19,7 @@ import ( "sort" "strconv" "strings" + "sync" "unsafe" "github.com/pingcap/errors" @@ -1552,6 +1553,9 @@ func findInPairs(colName string, pairs []nameValuePair) int { return -1 } +// Use cache to avoid allocating memory every time. +var subQueryCheckerPool = &sync.Pool{New: func() any { return &subQueryChecker{} }} + type subQueryChecker struct { hasSubQuery bool } @@ -1575,7 +1579,8 @@ func (s *subQueryChecker) Leave(in ast.Node) (ast.Node, bool) { } func isExprHasSubQuery(expr ast.Node) bool { - checker := &subQueryChecker{} + checker := subQueryCheckerPool.Get().(*subQueryChecker) + defer subQueryCheckerPool.Put(checker) expr.Accept(checker) return checker.hasSubQuery } From 38862e3fa27f5c6a8cd32ef3e4c5e411b882d484 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Wed, 18 Oct 2023 10:28:50 +0800 Subject: [PATCH 5/6] Fix broken test Signed-off-by: hi-rustin --- pkg/planner/core/point_get_plan.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/planner/core/point_get_plan.go b/pkg/planner/core/point_get_plan.go index a98827cf1346f..d4f8c240451a1 100644 --- a/pkg/planner/core/point_get_plan.go +++ b/pkg/planner/core/point_get_plan.go @@ -1580,7 +1580,10 @@ func (s *subQueryChecker) Leave(in ast.Node) (ast.Node, bool) { func isExprHasSubQuery(expr ast.Node) bool { checker := subQueryCheckerPool.Get().(*subQueryChecker) - defer subQueryCheckerPool.Put(checker) + defer func() { + checker.hasSubQuery = false + subQueryCheckerPool.Put(checker) + }() expr.Accept(checker) return checker.hasSubQuery } From 973d25c338f0654e85b4add42b313bfe9a83f05d Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Wed, 18 Oct 2023 10:31:50 +0800 Subject: [PATCH 6/6] Add comment Signed-off-by: hi-rustin --- pkg/planner/core/point_get_plan.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/planner/core/point_get_plan.go b/pkg/planner/core/point_get_plan.go index d4f8c240451a1..63403cfe5256f 100644 --- a/pkg/planner/core/point_get_plan.go +++ b/pkg/planner/core/point_get_plan.go @@ -1581,6 +1581,7 @@ func (s *subQueryChecker) Leave(in ast.Node) (ast.Node, bool) { func isExprHasSubQuery(expr ast.Node) bool { checker := subQueryCheckerPool.Get().(*subQueryChecker) defer func() { + // Do not forget to reset the flag. checker.hasSubQuery = false subQueryCheckerPool.Put(checker) }()