From 3698b5fc48ab370dc17b342e6fe433d6b41b0e8f Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Sat, 10 Oct 2020 17:17:19 +0800 Subject: [PATCH 1/7] executor: fix analyze update panic cause by duplicate call analyze executor Close method Signed-off-by: crazycs520 --- executor/explain.go | 14 +++++++------- executor/explain_test.go | 4 +++- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/executor/explain.go b/executor/explain.go index ac4c30e699f34..ed756ab91ba94 100644 --- a/executor/explain.go +++ b/executor/explain.go @@ -73,14 +73,14 @@ func (e *ExplainExec) Next(ctx context.Context, req *chunk.Chunk) error { } 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 - } - }() if e.analyzeExec != nil && !e.executed { + closed := false + defer func() { + if !closed && e.analyzeExec != nil { + err = e.analyzeExec.Close() + closed = true + } + }() e.executed = true chk := newFirstChunk(e.analyzeExec) var nextErr, closeErr error diff --git a/executor/explain_test.go b/executor/explain_test.go index b73d33e4870f8..dbbb9758b52c0 100644 --- a/executor/explain_test.go +++ b/executor/explain_test.go @@ -100,7 +100,9 @@ func (s *testSuite1) TestExplainWrite(c *C) { tk.MustExec("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.MustExec("explain analyze update t set a=a+1") + tk.MustExec("explain analyze replace into t values (4)") + tk.MustQuery("select * from t order by a").Check(testkit.Rows("2", "3", "4")) } func (s *testSuite1) TestExplainAnalyzeMemory(c *C) { From 33761ddc225b63b5e64e2d321de6e6c5cfd14f2b Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Sat, 10 Oct 2020 17:21:37 +0800 Subject: [PATCH 2/7] refine code Signed-off-by: crazycs520 --- executor/explain_test.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/executor/explain_test.go b/executor/explain_test.go index dbbb9758b52c0..e8e108e3580c3 100644 --- a/executor/explain_test.go +++ b/executor/explain_test.go @@ -93,16 +93,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.MustExec("explain analyze update t set a=a+1") - tk.MustExec("explain analyze replace into t values (4)") - tk.MustQuery("select * from t order by a").Check(testkit.Rows("2", "3", "4")) + 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) { From 231bf4700dbd4a4e875a7f66ed1130a418ea0f5f Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Sat, 10 Oct 2020 20:21:10 +0800 Subject: [PATCH 3/7] address comment Signed-off-by: crazycs520 --- executor/explain.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/executor/explain.go b/executor/explain.go index ed756ab91ba94..f66f14740cba1 100644 --- a/executor/explain.go +++ b/executor/explain.go @@ -74,11 +74,9 @@ func (e *ExplainExec) Next(ctx context.Context, req *chunk.Chunk) error { func (e *ExplainExec) generateExplainInfo(ctx context.Context) (rows [][]string, err error) { if e.analyzeExec != nil && !e.executed { - closed := false defer func() { - if !closed && e.analyzeExec != nil { + if e.analyzeExec != nil { err = e.analyzeExec.Close() - closed = true } }() e.executed = true @@ -90,8 +88,6 @@ func (e *ExplainExec) generateExplainInfo(ctx context.Context) (rows [][]string, break } } - closeErr = e.analyzeExec.Close() - closed = true if nextErr != nil { if closeErr != nil { err = errors.New(nextErr.Error() + ", " + closeErr.Error()) From d0c5d75bf48a2c98ebe607e9e67e9ec8f45e874c Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Sat, 10 Oct 2020 20:45:28 +0800 Subject: [PATCH 4/7] Revert "address comment" This reverts commit 231bf4700dbd4a4e875a7f66ed1130a418ea0f5f. --- executor/explain.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/executor/explain.go b/executor/explain.go index f66f14740cba1..ed756ab91ba94 100644 --- a/executor/explain.go +++ b/executor/explain.go @@ -74,9 +74,11 @@ func (e *ExplainExec) Next(ctx context.Context, req *chunk.Chunk) error { func (e *ExplainExec) generateExplainInfo(ctx context.Context) (rows [][]string, err error) { if e.analyzeExec != nil && !e.executed { + closed := false defer func() { - if e.analyzeExec != nil { + if !closed && e.analyzeExec != nil { err = e.analyzeExec.Close() + closed = true } }() e.executed = true @@ -88,6 +90,8 @@ func (e *ExplainExec) generateExplainInfo(ctx context.Context) (rows [][]string, break } } + closeErr = e.analyzeExec.Close() + closed = true if nextErr != nil { if closeErr != nil { err = errors.New(nextErr.Error() + ", " + closeErr.Error()) From 8c702da801d3140ed65454de8134b30330dc0e75 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Mon, 12 Oct 2020 16:07:05 +0800 Subject: [PATCH 5/7] fix test Signed-off-by: crazycs520 --- executor/explain_unit_test.go | 4 ++++ 1 file changed, 4 insertions(+) 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 6e7f2699115c445a3ef87ef5df30721a276547b6 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Mon, 12 Oct 2020 17:00:17 +0800 Subject: [PATCH 6/7] fix test Signed-off-by: crazycs520 --- executor/explain.go | 39 +++++++++++++++++---------------------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/executor/explain.go b/executor/explain.go index ed756ab91ba94..519dad2b962d7 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) { +func (e *ExplainExec) ExecuteAnalyzeExec(ctx context.Context) (err error) { if e.analyzeExec != nil && !e.executed { - closed := false defer func() { - if !closed && e.analyzeExec != nil { - err = e.analyzeExec.Close() - closed = true + 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 From 63b0b162579133e69942f9894965a82a2aa26502 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Mon, 12 Oct 2020 17:04:31 +0800 Subject: [PATCH 7/7] refine code Signed-off-by: crazycs520 --- executor/explain.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/executor/explain.go b/executor/explain.go index 519dad2b962d7..5547573272c10 100644 --- a/executor/explain.go +++ b/executor/explain.go @@ -72,7 +72,7 @@ func (e *ExplainExec) Next(ctx context.Context, req *chunk.Chunk) error { return nil } -func (e *ExplainExec) ExecuteAnalyzeExec(ctx context.Context) (err error) { +func (e *ExplainExec) executeAnalyzeExec(ctx context.Context) (err error) { if e.analyzeExec != nil && !e.executed { defer func() { err1 := e.analyzeExec.Close() @@ -97,7 +97,7 @@ func (e *ExplainExec) ExecuteAnalyzeExec(ctx context.Context) (err error) { } func (e *ExplainExec) generateExplainInfo(ctx context.Context) (rows [][]string, err error) { - if err = e.ExecuteAnalyzeExec(ctx); err != nil { + if err = e.executeAnalyzeExec(ctx); err != nil { return nil, err } if err = e.explain.RenderResult(); err != nil {