Skip to content

Commit

Permalink
opt: display memo cycle when potential cycle is detected
Browse files Browse the repository at this point in the history
The optimizer will error with "memo group optimization passes surpassed
limit of 100000; there may be a cycle in the memo" when it detects a
potential memo cycle. The error includes the memo in the error details
to aid in debugging. However, it can be hard for a human to find the
cycle in the formatted memo when it contains many groups (see cockroachdb#80901).

This commit updates the formatted memo within the error details to
include the path to the cycle, if one exists. A cycle is not searched
for in other cases where the memo is formatted, to avoid the overhead of
cycle detection.

A simple cycle detection library has been added that the memo formatter
uses to locate a path in a memo to a cycle.

Release note: None
  • Loading branch information
mgartner committed May 13, 2022
1 parent dd35ff8 commit 7adf89b
Show file tree
Hide file tree
Showing 8 changed files with 432 additions and 4 deletions.
1 change: 1 addition & 0 deletions pkg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ ALL_TESTS = [
"//pkg/sql/opt/bench:bench_test",
"//pkg/sql/opt/cat:cat_test",
"//pkg/sql/opt/constraint:constraint_test",
"//pkg/sql/opt/cycle:cycle_test",
"//pkg/sql/opt/distribution:distribution_test",
"//pkg/sql/opt/exec/execbuilder:execbuilder_test",
"//pkg/sql/opt/exec/explain:explain_test",
Expand Down
14 changes: 14 additions & 0 deletions pkg/sql/opt/cycle/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

go_library(
name = "cycle",
srcs = ["detector.go"],
importpath = "github.com/cockroachdb/cockroach/pkg/sql/opt/cycle",
visibility = ["//visibility:public"],
)

go_test(
name = "cycle_test",
srcs = ["detector_test.go"],
embed = [":cycle"],
)
118 changes: 118 additions & 0 deletions pkg/sql/opt/cycle/detector.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
// Copyright 2022 The Cockroach Authors.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package cycle

// Detector finds cycles in directed graphs. Vertices are identified as unique
// integers by the caller.
//
// Example usage:
//
// var d Detector
// d.AddEdge(Vertex(0), Vertex(1))
// d.AddEdge(Vertex(1), Vertex(2))
// d.AddEdge(Vertex(2), Vertex(0))
// d.FindCycleStartingAtVertex(Vertex(0))
// => [0, 1, 2, 0], true
//
type Detector struct {
// edges are the directed edges in the graph.
edges map[Vertex][]Vertex
}

// Vertex is a vertex in the graph.
type Vertex int

// status represents one of three possible states of a vertex during cycle
// detection.
type status int

const (
// statusNotVisited indicates that a vertex has not been previously visited.
statusNotVisited status = iota
// statusVisited indicates that a vertex has been visited but a verdict on
// its involvement in a cycle has not yet been reached.
statusVisited
// statusDone indicates that a vertex has been visited and has been proven
// innocent of being involved in a cycle.
statusDone
)

// AddEdge adds a directed edge to the graph.
func (cd *Detector) AddEdge(from, to Vertex) {
if cd.edges == nil {
cd.edges = make(map[Vertex][]Vertex)
}
cd.edges[from] = append(cd.edges[from], to)
}

// FindCycleStartingAtVertex searches for a cycle starting from v. If one is
// found, a path containing that cycle is returned. After finding the first
// cycle, searching ceases. If no cycle is found, ok=false is returned.
func (cd *Detector) FindCycleStartingAtVertex(v Vertex) (cycle []Vertex, ok bool) {
vertices := make(map[Vertex]status)
var s stack
if ok := cd.hasCycle(v, vertices, &s); ok {
// If a cycle was found, s will contain a path that includes the cycle.
return s, true
}
return nil, false
}

// hasCycle performs a depth-first search of the graph starting at v in search
// of a cycle. It returns true when the first cycle is found. If a cycle was
// found, s contains a path that includes the cycle. Cycles are detected by
// finding edges that backtrack to a previously visited vertex.
func (cd *Detector) hasCycle(v Vertex, vertices map[Vertex]status, s *stack) bool {
// Mark the given Vertex as visited and push it onto the stack.
vertices[v] = statusVisited
s.push(v)

// Search all children of v for cycles.
for _, child := range cd.edges[v] {
switch vertices[child] {
case statusVisited:
// A cycle has been found. The current stack is a path to the cycle.
// Push the child onto the stack so that the cycle is more obvious
// in the path.
s.push(child)
return true
case statusNotVisited:
// Recurse if this child of v has not yet been visited.
if ok := cd.hasCycle(child, vertices, s); ok {
// If a cycle was found deeper down, propagate the result and
// halt searching other children of v.
return true
}
case statusDone:
// We have already proven that this child does not backtrack to
// anywhere further up the stack, so we do not need to recurse into
// it.
}
}

// If we didn't return early from the loop, we did not find a cycle below
// this vertex, so we can mark the vertex as done and pop it off the stack.
vertices[v] = statusDone
s.pop()
return false
}

// stack is a simple implementation of a stack of vertices. It allows more
// ergonomic mutation by callees than a slice.
type stack []Vertex

func (s *stack) push(i Vertex) {
*s = append(*s, i)
}

func (s *stack) pop() {
*s = (*s)[:len(*s)-1]
}
183 changes: 183 additions & 0 deletions pkg/sql/opt/cycle/detector_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
// Copyright 2022 The Cockroach Authors.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package cycle

import (
"reflect"
"testing"
)

func TestDetector(t *testing.T) {
testCases := []struct {
edges []edge
start Vertex
expected []Vertex
}{
// No cycles.
{
edges: []edge{},
start: 0,
expected: nil,
},
{
edges: []edge{
{0, 1},
{1, 2},
{2, 3},
},
start: 0,
expected: nil,
},
{
edges: []edge{
{0, 1},
{0, 2},
{0, 3},
{1, 2},
{2, 3},
},
start: 0,
expected: nil,
},
{
edges: []edge{
{0, 1},
{0, 2},
{0, 3},
{1, 2},
{3, 1},
},
start: 0,
expected: nil,
},
{
edges: []edge{
{0, 1},
{0, 4},
{1, 2},
{1, 3},
{4, 3},
{4, 5},
{5, 3},
},
start: 0,
expected: nil,
},
{
edges: []edge{
{1, 2},
{1, 3},
{1, 4},
{1, 7},
{4, 5},
{5, 6},
{6, 7},
{7, 4},
},
start: 0,
expected: nil,
},

// Cycles.
{
edges: []edge{
{0, 0},
},
start: 0,
expected: []Vertex{0, 0},
},
{
edges: []edge{
{0, 1},
{1, 0},
},
start: 0,
expected: []Vertex{0, 1, 0},
},
{
edges: []edge{
{0, 1},
{1, 0},
},
start: 1,
expected: []Vertex{1, 0, 1},
},
{
edges: []edge{
{0, 1},
{1, 2},
{2, 3},
{3, 4},
{4, 1},
},
start: 0,
expected: []Vertex{0, 1, 2, 3, 4, 1},
},
{
edges: []edge{
{0, 1},
{1, 3},
{3, 2},
{2, 1},
},
start: 0,
expected: []Vertex{0, 1, 3, 2, 1},
},
{
edges: []edge{
{0, 1},
{0, 4},
{1, 2},
{1, 3},
{3, 5},
{4, 3},
{4, 5},
{5, 1},
},
start: 0,
expected: []Vertex{0, 1, 3, 5, 1},
},
{
edges: []edge{
{1, 2},
{1, 3},
{1, 4},
{1, 7},
{4, 5},
{5, 6},
{6, 7},
{7, 4},
},
start: 1,
expected: []Vertex{1, 4, 5, 6, 7, 4},
},
{
edges: []edge{
{0, 1},
{1, 2},
{2, 3},
{3, 3},
},
start: 1,
expected: []Vertex{1, 2, 3, 3},
},
}
for _, tc := range testCases {
var d Detector
for _, edge := range tc.edges {
d.AddEdge(edge.from, edge.to)
}
res, _ := d.FindCycleStartingAtVertex(tc.start)
if !reflect.DeepEqual(res, tc.expected) {
t.Errorf("expected %v, got %v", tc.expected, res)
}
}
}
1 change: 1 addition & 0 deletions pkg/sql/opt/xform/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ go_library(
"//pkg/sql/opt",
"//pkg/sql/opt/cat",
"//pkg/sql/opt/constraint",
"//pkg/sql/opt/cycle",
"//pkg/sql/opt/distribution",
"//pkg/sql/opt/idxconstraint",
"//pkg/sql/opt/invertedexpr",
Expand Down
Loading

0 comments on commit 7adf89b

Please sign in to comment.