Skip to content

Commit

Permalink
sql/schemachanger/scgraph, scplan: fixed a bug when drawing dep graph
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Xiang-Gu committed Aug 8, 2022
1 parent d58473e commit caa3c6c
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 3 deletions.
2 changes: 1 addition & 1 deletion pkg/sql/schemachanger/scplan/internal/scgraph/iteration.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/schemachanger/scplan/plan_explain.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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)
}
}()

Expand Down

0 comments on commit caa3c6c

Please sign in to comment.