From 445218a58fc920484cb84d0025dca9e5d39cd5df Mon Sep 17 00:00:00 2001 From: crazycs Date: Mon, 12 Oct 2020 20:25:18 +0800 Subject: [PATCH 1/3] cherry pick #20390 to release-4.0 Signed-off-by: ti-srebot --- executor/explain.go | 42 +++++++++++++++++++++-------------- executor/explain_test.go | 11 ++++----- executor/explain_unit_test.go | 4 ++++ 3 files changed, 35 insertions(+), 22 deletions(-) diff --git a/executor/explain.go b/executor/explain.go index 2e12fd9d417b2..6747a130f2c26 100644 --- a/executor/explain.go +++ b/executor/explain.go @@ -71,6 +71,7 @@ func (e *ExplainExec) Next(ctx context.Context, req *chunk.Chunk) error { return nil } +<<<<<<< HEAD func (e *ExplainExec) generateExplainInfo(ctx context.Context) (rows [][]string, err error) { closed := false defer func() { @@ -80,28 +81,35 @@ func (e *ExplainExec) generateExplainInfo(ctx context.Context) (rows [][]string, } }() if e.analyzeExec != nil { +======= +func (e *ExplainExec) executeAnalyzeExec(ctx context.Context) (err error) { + if e.analyzeExec != nil && !e.executed { + defer func() { + err1 := e.analyzeExec.Close() + if err1 != nil { + if err != nil { + err = errors.New(err.Error() + ", " + err1.Error()) + } else { + err = err1 + } + } + }() + e.executed = true +>>>>>>> 875cf6dfb... executor: fix analyze update panic cause by duplicate call analyze executor Close method (#20390) chk := newFirstChunk(e.analyzeExec) - var nextErr, closeErr error for { - nextErr = Next(ctx, e.analyzeExec, chk) - if nextErr != nil || chk.NumRows() == 0 { + err = Next(ctx, e.analyzeExec, chk) + if err != nil || chk.NumRows() == 0 { break } } - closeErr = e.analyzeExec.Close() - closed = true - if nextErr != nil { - if closeErr != nil { - err = errors.New(nextErr.Error() + ", " + closeErr.Error()) - } else { - err = nextErr - } - } else if closeErr != nil { - err = closeErr - } - if err != nil { - return nil, err - } + } + return err +} + +func (e *ExplainExec) generateExplainInfo(ctx context.Context) (rows [][]string, err error) { + if err = e.executeAnalyzeExec(ctx); err != nil { + return nil, err } if err = e.explain.RenderResult(); err != nil { return nil, err diff --git a/executor/explain_test.go b/executor/explain_test.go index 57a3c5ed5d265..0965a0a9f9ee0 100644 --- a/executor/explain_test.go +++ b/executor/explain_test.go @@ -97,14 +97,15 @@ func (s *testSuite1) TestExplainWrite(c *C) { tk := testkit.NewTestKitWithInit(c, s.store) tk.MustExec("drop table if exists t") tk.MustExec("create table t (a int)") - tk.MustExec("explain analyze insert into t select 1") + tk.MustQuery("explain analyze insert into t select 1") tk.MustQuery("select * from t").Check(testkit.Rows("1")) - tk.MustExec("explain analyze update t set a=2 where a=1") + tk.MustQuery("explain analyze update t set a=2 where a=1") tk.MustQuery("select * from t").Check(testkit.Rows("2")) - tk.MustExec("explain insert into t select 1") + tk.MustQuery("explain insert into t select 1") tk.MustQuery("select * from t").Check(testkit.Rows("2")) - tk.MustExec("explain analyze insert into t select 1") - tk.MustQuery("select * from t order by a").Check(testkit.Rows("1", "2")) + tk.MustQuery("explain analyze insert into t select 1") + tk.MustQuery("explain analyze replace into t values (3)") + tk.MustQuery("select * from t order by a").Check(testkit.Rows("1", "2", "3")) } func (s *testSuite1) TestExplainAnalyzeMemory(c *C) { diff --git a/executor/explain_unit_test.go b/executor/explain_unit_test.go index fa9b13bc66e48..1276575f47c28 100644 --- a/executor/explain_unit_test.go +++ b/executor/explain_unit_test.go @@ -81,6 +81,10 @@ func TestExplainAnalyzeInvokeNextAndClose(t *testing.T) { t.Errorf(err.Error()) } // mockErrorOperator panic + explainExec = &ExplainExec{ + baseExecutor: baseExec, + explain: nil, + } mockOpr = mockErrorOperator{baseExec, true, false} explainExec.analyzeExec = &mockOpr defer func() { From 651bddb8c015131c52f823b1a818cc7ad78d177a Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Mon, 16 Nov 2020 17:15:12 +0800 Subject: [PATCH 2/3] Revert "cherry pick #20390 to release-4.0" This reverts commit 445218a58fc920484cb84d0025dca9e5d39cd5df. --- executor/explain.go | 42 ++++++++++++++--------------------- executor/explain_test.go | 11 +++++---- executor/explain_unit_test.go | 4 ---- 3 files changed, 22 insertions(+), 35 deletions(-) diff --git a/executor/explain.go b/executor/explain.go index 6747a130f2c26..2e12fd9d417b2 100644 --- a/executor/explain.go +++ b/executor/explain.go @@ -71,7 +71,6 @@ func (e *ExplainExec) Next(ctx context.Context, req *chunk.Chunk) error { return nil } -<<<<<<< HEAD func (e *ExplainExec) generateExplainInfo(ctx context.Context) (rows [][]string, err error) { closed := false defer func() { @@ -81,35 +80,28 @@ func (e *ExplainExec) generateExplainInfo(ctx context.Context) (rows [][]string, } }() if e.analyzeExec != nil { -======= -func (e *ExplainExec) executeAnalyzeExec(ctx context.Context) (err error) { - if e.analyzeExec != nil && !e.executed { - defer func() { - err1 := e.analyzeExec.Close() - if err1 != nil { - if err != nil { - err = errors.New(err.Error() + ", " + err1.Error()) - } else { - err = err1 - } - } - }() - e.executed = true ->>>>>>> 875cf6dfb... executor: fix analyze update panic cause by duplicate call analyze executor Close method (#20390) chk := newFirstChunk(e.analyzeExec) + var nextErr, closeErr error for { - err = Next(ctx, e.analyzeExec, chk) - if err != nil || chk.NumRows() == 0 { + nextErr = Next(ctx, e.analyzeExec, chk) + if nextErr != nil || chk.NumRows() == 0 { break } } - } - return err -} - -func (e *ExplainExec) generateExplainInfo(ctx context.Context) (rows [][]string, err error) { - if err = e.executeAnalyzeExec(ctx); err != nil { - return nil, err + closeErr = e.analyzeExec.Close() + closed = true + if nextErr != nil { + if closeErr != nil { + err = errors.New(nextErr.Error() + ", " + closeErr.Error()) + } else { + err = nextErr + } + } else if closeErr != nil { + err = closeErr + } + if err != nil { + return nil, err + } } if err = e.explain.RenderResult(); err != nil { return nil, err diff --git a/executor/explain_test.go b/executor/explain_test.go index 0965a0a9f9ee0..57a3c5ed5d265 100644 --- a/executor/explain_test.go +++ b/executor/explain_test.go @@ -97,15 +97,14 @@ func (s *testSuite1) TestExplainWrite(c *C) { tk := testkit.NewTestKitWithInit(c, s.store) tk.MustExec("drop table if exists t") tk.MustExec("create table t (a int)") - tk.MustQuery("explain analyze insert into t select 1") + tk.MustExec("explain analyze insert into t select 1") tk.MustQuery("select * from t").Check(testkit.Rows("1")) - tk.MustQuery("explain analyze update t set a=2 where a=1") + tk.MustExec("explain analyze update t set a=2 where a=1") tk.MustQuery("select * from t").Check(testkit.Rows("2")) - tk.MustQuery("explain insert into t select 1") + tk.MustExec("explain insert into t select 1") tk.MustQuery("select * from t").Check(testkit.Rows("2")) - tk.MustQuery("explain analyze insert into t select 1") - tk.MustQuery("explain analyze replace into t values (3)") - tk.MustQuery("select * from t order by a").Check(testkit.Rows("1", "2", "3")) + tk.MustExec("explain analyze insert into t select 1") + tk.MustQuery("select * from t order by a").Check(testkit.Rows("1", "2")) } func (s *testSuite1) TestExplainAnalyzeMemory(c *C) { diff --git a/executor/explain_unit_test.go b/executor/explain_unit_test.go index 1276575f47c28..fa9b13bc66e48 100644 --- a/executor/explain_unit_test.go +++ b/executor/explain_unit_test.go @@ -81,10 +81,6 @@ func TestExplainAnalyzeInvokeNextAndClose(t *testing.T) { t.Errorf(err.Error()) } // mockErrorOperator panic - explainExec = &ExplainExec{ - baseExecutor: baseExec, - explain: nil, - } mockOpr = mockErrorOperator{baseExec, true, false} explainExec.analyzeExec = &mockOpr defer func() { From 3dde297eec2c1c044cc2b4ba255fd16d5eb42bc8 Mon Sep 17 00:00:00 2001 From: crazycs Date: Mon, 12 Oct 2020 20:25:18 +0800 Subject: [PATCH 3/3] executor: fix analyze update panic cause by duplicate call analyze executor Close method (#20390) --- executor/explain.go | 45 ++++++++++++++++------------------- executor/explain_test.go | 11 +++++---- executor/explain_unit_test.go | 4 ++++ 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/executor/explain.go b/executor/explain.go index ac4c30e699f34..5547573272c10 100644 --- a/executor/explain.go +++ b/executor/explain.go @@ -72,38 +72,33 @@ func (e *ExplainExec) Next(ctx context.Context, req *chunk.Chunk) error { return nil } -func (e *ExplainExec) generateExplainInfo(ctx context.Context) (rows [][]string, err error) { - closed := false - defer func() { - if !closed && e.analyzeExec != nil { - err = e.analyzeExec.Close() - closed = true - } - }() +func (e *ExplainExec) executeAnalyzeExec(ctx context.Context) (err error) { if e.analyzeExec != nil && !e.executed { + defer func() { + err1 := e.analyzeExec.Close() + if err1 != nil { + if err != nil { + err = errors.New(err.Error() + ", " + err1.Error()) + } else { + err = err1 + } + } + }() e.executed = true chk := newFirstChunk(e.analyzeExec) - var nextErr, closeErr error for { - nextErr = Next(ctx, e.analyzeExec, chk) - if nextErr != nil || chk.NumRows() == 0 { + err = Next(ctx, e.analyzeExec, chk) + if err != nil || chk.NumRows() == 0 { break } } - closeErr = e.analyzeExec.Close() - closed = true - if nextErr != nil { - if closeErr != nil { - err = errors.New(nextErr.Error() + ", " + closeErr.Error()) - } else { - err = nextErr - } - } else if closeErr != nil { - err = closeErr - } - if err != nil { - return nil, err - } + } + return err +} + +func (e *ExplainExec) generateExplainInfo(ctx context.Context) (rows [][]string, err error) { + if err = e.executeAnalyzeExec(ctx); err != nil { + return nil, err } if err = e.explain.RenderResult(); err != nil { return nil, err diff --git a/executor/explain_test.go b/executor/explain_test.go index 57a3c5ed5d265..0965a0a9f9ee0 100644 --- a/executor/explain_test.go +++ b/executor/explain_test.go @@ -97,14 +97,15 @@ func (s *testSuite1) TestExplainWrite(c *C) { tk := testkit.NewTestKitWithInit(c, s.store) tk.MustExec("drop table if exists t") tk.MustExec("create table t (a int)") - tk.MustExec("explain analyze insert into t select 1") + tk.MustQuery("explain analyze insert into t select 1") tk.MustQuery("select * from t").Check(testkit.Rows("1")) - tk.MustExec("explain analyze update t set a=2 where a=1") + tk.MustQuery("explain analyze update t set a=2 where a=1") tk.MustQuery("select * from t").Check(testkit.Rows("2")) - tk.MustExec("explain insert into t select 1") + tk.MustQuery("explain insert into t select 1") tk.MustQuery("select * from t").Check(testkit.Rows("2")) - tk.MustExec("explain analyze insert into t select 1") - tk.MustQuery("select * from t order by a").Check(testkit.Rows("1", "2")) + tk.MustQuery("explain analyze insert into t select 1") + tk.MustQuery("explain analyze replace into t values (3)") + tk.MustQuery("select * from t order by a").Check(testkit.Rows("1", "2", "3")) } func (s *testSuite1) TestExplainAnalyzeMemory(c *C) { diff --git a/executor/explain_unit_test.go b/executor/explain_unit_test.go index fa9b13bc66e48..1276575f47c28 100644 --- a/executor/explain_unit_test.go +++ b/executor/explain_unit_test.go @@ -81,6 +81,10 @@ func TestExplainAnalyzeInvokeNextAndClose(t *testing.T) { t.Errorf(err.Error()) } // mockErrorOperator panic + explainExec = &ExplainExec{ + baseExecutor: baseExec, + explain: nil, + } mockOpr = mockErrorOperator{baseExec, true, false} explainExec.analyzeExec = &mockOpr defer func() {