From 28f9545507e5b1e52d1440b5df73fb678f2ebf6f Mon Sep 17 00:00:00 2001 From: Reminiscent Date: Mon, 19 Sep 2022 11:39:56 +0800 Subject: [PATCH 1/3] planner: fix panic when prepare and execute the insert on duplicate --- executor/insert_test.go | 13 +++++++++++++ parser/ast/dml.go | 1 + planner/core/planbuilder.go | 11 +++++++---- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/executor/insert_test.go b/executor/insert_test.go index d0d17199c13f7..06c9927bba008 100644 --- a/executor/insert_test.go +++ b/executor/insert_test.go @@ -352,6 +352,19 @@ func TestUpdateDuplicateKey(t *testing.T) { "[kv:1062]Duplicate entry '1-2-4' for key 'PRIMARY'") } +func TestIssue37187(t *testing.T) { + store := testkit.CreateMockStore(t) + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test") + + tk.MustExec("drop table if exists a, b") + tk.MustExec("create table t1 (a int(11) ,b varchar(100) ,primary key (a));") + tk.MustExec("create table t2 (c int(11) ,d varchar(100) ,primary key (c));") + tk.MustExec("prepare in1 from 'insert into t1 (a,b) select c,null from t2 t on duplicate key update b=t.d';") + err := tk.ExecToErr("execute in1;") + require.NoError(t, err) +} + func TestInsertWrongValueForField(t *testing.T) { store := testkit.CreateMockStore(t) tk := testkit.NewTestKit(t, store) diff --git a/parser/ast/dml.go b/parser/ast/dml.go index 525426946b0fd..5548f843babcf 100644 --- a/parser/ast/dml.go +++ b/parser/ast/dml.go @@ -2023,6 +2023,7 @@ type InsertStmt struct { // TableHints represents the table level Optimizer Hint for join type. TableHints []*TableOptimizerHint PartitionNames []model.CIStr + ColLen int } // Restore implements Node interface. diff --git a/planner/core/planbuilder.go b/planner/core/planbuilder.go index dbb9195ba3e79..a52bb26846711 100644 --- a/planner/core/planbuilder.go +++ b/planner/core/planbuilder.go @@ -3821,13 +3821,13 @@ func (b *PlanBuilder) buildSelectPlanOfInsert(ctx context.Context, insert *ast.I if err != nil { return err } - actualColLen := -1 + actualColLen := insert.ColLen // For MYSQL, it handles the case that the columns in ON DUPLICATE UPDATE is not the project column of the SELECT clause // but just in the table occurs in the SELECT CLAUSE. // e.g. insert into a select x from b ON DUPLICATE KEY UPDATE a.x=b.y; the `y` is not a column of select's output. // MySQL won't throw error and will execute this SQL successfully. // To make compatible with this strange feature, we add the variable `actualColLen` and the following IF branch. - if len(insert.OnDuplicate) > 0 { + if len(insert.OnDuplicate) > 0 && actualColLen == 0 { // If the select has aggregation, it cannot see the columns not in the select field. // e.g. insert into a select x from b ON DUPLICATE KEY UPDATE a.x=b.y; can be executed successfully. // insert into a select x from b group by x ON DUPLICATE KEY UPDATE a.x=b.y; will report b.y not found. @@ -3874,7 +3874,7 @@ func (b *PlanBuilder) buildSelectPlanOfInsert(ctx context.Context, insert *ast.I } // Check to guarantee that the length of the row returned by select is equal to that of affectedValuesCols. - if (actualColLen == -1 && selectPlan.Schema().Len() != len(affectedValuesCols)) || (actualColLen != -1 && actualColLen != len(affectedValuesCols)) { + if (actualColLen == 0 && selectPlan.Schema().Len() != len(affectedValuesCols)) || (actualColLen != 0 && actualColLen != len(affectedValuesCols)) { return ErrWrongValueCountOnRow.GenWithStackByArgs(1) } @@ -3897,9 +3897,12 @@ func (b *PlanBuilder) buildSelectPlanOfInsert(ctx context.Context, insert *ast.I return err } - if actualColLen == -1 { + if actualColLen == 0 { actualColLen = selectPlan.Schema().Len() } + if insert.ColLen == 0 { + insert.ColLen = actualColLen + } insertPlan.RowLen = actualColLen // schema4NewRow is the schema for the newly created data record based on // the result of the select statement. From 95765cbcc772e898b3db444ebfa6155da4fb7693 Mon Sep 17 00:00:00 2001 From: Reminiscent Date: Thu, 22 Sep 2022 15:23:01 +0800 Subject: [PATCH 2/3] address comments --- parser/ast/dml.go | 1 - planner/core/planbuilder.go | 16 +++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/parser/ast/dml.go b/parser/ast/dml.go index 5548f843babcf..525426946b0fd 100644 --- a/parser/ast/dml.go +++ b/parser/ast/dml.go @@ -2023,7 +2023,6 @@ type InsertStmt struct { // TableHints represents the table level Optimizer Hint for join type. TableHints []*TableOptimizerHint PartitionNames []model.CIStr - ColLen int } // Restore implements Node interface. diff --git a/planner/core/planbuilder.go b/planner/core/planbuilder.go index fc41b45a47524..c74679f1a15b0 100644 --- a/planner/core/planbuilder.go +++ b/planner/core/planbuilder.go @@ -3821,13 +3821,13 @@ func (b *PlanBuilder) buildSelectPlanOfInsert(ctx context.Context, insert *ast.I if err != nil { return err } - actualColLen := insert.ColLen + actualColLen := -1 // For MYSQL, it handles the case that the columns in ON DUPLICATE UPDATE is not the project column of the SELECT clause // but just in the table occurs in the SELECT CLAUSE. // e.g. insert into a select x from b ON DUPLICATE KEY UPDATE a.x=b.y; the `y` is not a column of select's output. // MySQL won't throw error and will execute this SQL successfully. // To make compatible with this strange feature, we add the variable `actualColLen` and the following IF branch. - if len(insert.OnDuplicate) > 0 && actualColLen == 0 { + if len(insert.OnDuplicate) > 0 { // If the select has aggregation, it cannot see the columns not in the select field. // e.g. insert into a select x from b ON DUPLICATE KEY UPDATE a.x=b.y; can be executed successfully. // insert into a select x from b group by x ON DUPLICATE KEY UPDATE a.x=b.y; will report b.y not found. @@ -3865,6 +3865,11 @@ func (b *PlanBuilder) buildSelectPlanOfInsert(ctx context.Context, insert *ast.I } sel.Fields.Fields = append(sel.Fields.Fields, &ast.SelectField{Expr: colName, Offset: len(sel.Fields.Fields)}) } + defer func() { + // Revert the change for ast. Because when we use the 'prepare' and 'execute' statement it will both build plan which will cause problem. + // You can see the issue #37187 for more details. + sel.Fields.Fields = sel.Fields.Fields[:actualColLen] + }() } } } @@ -3874,7 +3879,7 @@ func (b *PlanBuilder) buildSelectPlanOfInsert(ctx context.Context, insert *ast.I } // Check to guarantee that the length of the row returned by select is equal to that of affectedValuesCols. - if (actualColLen == 0 && selectPlan.Schema().Len() != len(affectedValuesCols)) || (actualColLen != 0 && actualColLen != len(affectedValuesCols)) { + if (actualColLen == -1 && selectPlan.Schema().Len() != len(affectedValuesCols)) || (actualColLen != -1 && actualColLen != len(affectedValuesCols)) { return ErrWrongValueCountOnRow.GenWithStackByArgs(1) } @@ -3897,12 +3902,9 @@ func (b *PlanBuilder) buildSelectPlanOfInsert(ctx context.Context, insert *ast.I return err } - if actualColLen == 0 { + if actualColLen == -1 { actualColLen = selectPlan.Schema().Len() } - if insert.ColLen == 0 { - insert.ColLen = actualColLen - } insertPlan.RowLen = actualColLen // schema4NewRow is the schema for the newly created data record based on // the result of the select statement. From 7b26599435dfc8aed457281ec319eb60d7720b91 Mon Sep 17 00:00:00 2001 From: Reminiscent Date: Thu, 22 Sep 2022 15:52:32 +0800 Subject: [PATCH 3/3] address comments --- planner/core/planbuilder.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/planner/core/planbuilder.go b/planner/core/planbuilder.go index 5c178f5c430d6..41d01c61cc983 100644 --- a/planner/core/planbuilder.go +++ b/planner/core/planbuilder.go @@ -3892,11 +3892,11 @@ func (b *PlanBuilder) buildSelectPlanOfInsert(ctx context.Context, insert *ast.I } sel.Fields.Fields = append(sel.Fields.Fields, &ast.SelectField{Expr: colName, Offset: len(sel.Fields.Fields)}) } - defer func() { + defer func(originSelFieldLen int) { // Revert the change for ast. Because when we use the 'prepare' and 'execute' statement it will both build plan which will cause problem. // You can see the issue #37187 for more details. - sel.Fields.Fields = sel.Fields.Fields[:actualColLen] - }() + sel.Fields.Fields = sel.Fields.Fields[:originSelFieldLen] + }(actualColLen) } } }