From caa3c6ce6c37ce42048d0cd40a879a7929924e8a Mon Sep 17 00:00:00 2001 From: Xiang Gu Date: Mon, 8 Aug 2022 17:13:10 -0400 Subject: [PATCH] sql/schemachanger/scgraph, scplan: fixed a bug when drawing dep graph Previously, we define all stauses an element can be in in the declarative schema changer in the scpb package. We removed one status (TXN_DROPPED) previously from that list and leave its enum number as a reserved number. However, some logic in scgraph incorrectly made the assumption that all enum numbers are active and we can just iterate from 0 to len(enum_list)-1 in order to iterate over all possible status, part of the logic to draw the dep graph. This is problematic because as we continue to add more status in that enum list, such way of iteration will be incorrect to draw the dep graph. This PR fixes that. This PR also spotted and fixed an panic recover bug where we forget to correctly update the return error, causing a situation where if a panic happens and the recover catches it, we will return with a nil error. Release note (bug fix): Fixed a bug internal to drawing dependency graph of a DDL statement under the declarative schema changer. --- pkg/sql/schemachanger/scplan/internal/scgraph/iteration.go | 2 +- pkg/sql/schemachanger/scplan/plan_explain.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/sql/schemachanger/scplan/internal/scgraph/iteration.go b/pkg/sql/schemachanger/scplan/internal/scgraph/iteration.go index bdd3b223a455..0e8a5e4d8c7a 100644 --- a/pkg/sql/schemachanger/scplan/internal/scgraph/iteration.go +++ b/pkg/sql/schemachanger/scplan/internal/scgraph/iteration.go @@ -23,7 +23,7 @@ type NodeIterator func(n *screl.Node) error // ForEachNode iterates the nodes in the graph. func (g *Graph) ForEachNode(it NodeIterator) error { for _, m := range g.targetNodes { - for i := 0; i < scpb.NumStatus; i++ { + for i := range scpb.Status_name { if ts, ok := m[scpb.Status(i)]; ok { if err := it(ts); err != nil { return iterutil.Map(err) diff --git a/pkg/sql/schemachanger/scplan/plan_explain.go b/pkg/sql/schemachanger/scplan/plan_explain.go index 74cc47d34b1f..ce587df7275a 100644 --- a/pkg/sql/schemachanger/scplan/plan_explain.go +++ b/pkg/sql/schemachanger/scplan/plan_explain.go @@ -30,7 +30,7 @@ import ( ) // DecorateErrorWithPlanDetails adds plan graphviz URLs as error details. -func (p Plan) DecorateErrorWithPlanDetails(err error) error { +func (p Plan) DecorateErrorWithPlanDetails(err error) (retErr error) { if err == nil { return nil } @@ -40,7 +40,7 @@ func (p Plan) DecorateErrorWithPlanDetails(err error) error { if !ok { rAsErr = errors.Errorf("panic during scplan.DecorateErrorWithPlanDetails: %v", r) } - err = errors.CombineErrors(err, rAsErr) + retErr = errors.CombineErrors(err, rAsErr) } }()