sql/schemachanger: remove cycle DepEdge rules, add SameStage kind

This commit seeks to rectify an early mistake in the architecture of the
declarative schema changer. In the original design, we knew we wanted certain
transitions to happen in the same stage. In order to deal with that, we created
rules that allowed for special types of cycles in dependencies to exist. This
was a mistake. Instead, we replace this by a `Kind` property of `DepEdge`s
which indicates whether the target pointed to merely needs to `HappenBefore`
or whether it also needs to happen in the `SameStage`. This allows us to
express exactly what we meant.

This change also uncovered some broken cycles which never were intended to
exist. The resultant plans generally look better.

Release note: None

pkg/sql/schemachanger/scgraph/BUILD.bazel

@@ -1,4 +1,5 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+load("//build:STRINGER.bzl", "stringer")
 
 go_library(
     name = "scgraph",
@@ -7,6 +8,7 @@ go_library(
         "edge.go",
         "graph.go",
         "iteration.go",
+        ":gen-depedgekind-stringer",  # keep
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scgraph",
     visibility = ["//visibility:public"],
@@ -37,3 +39,9 @@ go_test(
         "@com_github_stretchr_testify//require",
     ],
 )
+
+stringer(
+    name = "gen-depedgekind-stringer",
+    src = "edge.go",
+    typ = "DepEdgeKind",
+)

pkg/sql/schemachanger/scgraph/edge.go

@@ -49,11 +49,31 @@ func (oe *OpEdge) Type() scop.Type {
 	return oe.typ
 }
 
+// DepEdgeKind indicates the kind of constraint enforced by the edge.
+type DepEdgeKind int
+
+//go:generate stringer -type DepEdgeKind
+
+const (
+	_ DepEdgeKind = iota
+
+	// HappensAfter indicates that the source (from) of the edge must not be
+	// entered until after the destination (to) has entered the state. It could
+	// be in the same stage, or it could be in a subsequent stage.
+	HappensAfter
+
+	// SameStage indicates that the source (from) of the edge must
+	// not be entered until after the destination (to) has entered the state and
+	// that both nodes must enter the state in the same stage.
+	SameStage
+)
+
 // DepEdge represents a dependency between two nodes. A dependency
 // implies that the To() node cannot be reached before the From() node. It
 // can be reached concurrently.
 type DepEdge struct {
 	from, to *scpb.Node
+	kind     DepEdgeKind
 
 	// TODO(ajwerner): Deal with the possibility that multiple rules could
 	// generate the same edge.
@@ -68,3 +88,6 @@ func (de *DepEdge) To() *scpb.Node { return de.to }
 
 // Name returns the name of the rule which generated this edge.
 func (de *DepEdge) Name() string { return de.rule }
+
+// Kind returns the kind of the DepEdge.
+func (de *DepEdge) Kind() DepEdgeKind { return de.kind }

pkg/sql/schemachanger/scgraph/graph.go

@@ -50,6 +50,10 @@ type Graph struct {
 	// reached before or concurrently with this node.
 	depEdgesFrom *depEdgeTree
 
+	// sameStageDepEdgesTo maps a Node to the DepEdges with the
+	// SameStage kind incident upon the indexed node.
+	sameStageDepEdgesTo *depEdgeTree
+
 	// opToNode maps from an operation back to the
 	// opEdge that generated it as an index.
 	opToNode map[scop.Op]*scpb.Node
@@ -91,6 +95,7 @@ func New(initial scpb.State) (*Graph, error) {
 		authorization:       initial.Authorization,
 	}
 	g.depEdgesFrom = newDepEdgeTree(fromTo, g.compareNodes)
+	g.sameStageDepEdgesTo = newDepEdgeTree(toFrom, g.compareNodes)
 	for _, n := range initial.Nodes {
 		if existing, ok := g.targetIdxMap[n.Target]; ok {
 			return nil, errors.Errorf("invalid initial state contains duplicate target: %v and %v", n, initial.Nodes[existing])
@@ -120,6 +125,7 @@ func (g *Graph) ShallowClone() *Graph {
 		targetIdxMap:        g.targetIdxMap,
 		opEdgesFrom:         g.opEdgesFrom,
 		depEdgesFrom:        g.depEdgesFrom,
+		sameStageDepEdgesTo: g.sameStageDepEdgesTo,
 		opToNode:            g.opToNode,
 		edges:               g.edges,
 		entities:            g.entities,
@@ -230,12 +236,13 @@ var _ = (*Graph)(nil).GetNodeFromOp
 // and statuses).
 func (g *Graph) AddDepEdge(
 	rule string,
+	kind DepEdgeKind,
 	fromTarget *scpb.Target,
 	fromStatus scpb.Status,
 	toTarget *scpb.Target,
 	toStatus scpb.Status,
 ) (err error) {
-	de := &DepEdge{rule: rule}
+	de := &DepEdge{rule: rule, kind: kind}
 	if de.from, err = g.getOrCreateNode(fromTarget, fromStatus); err != nil {
 		return err
 	}
@@ -244,6 +251,9 @@ func (g *Graph) AddDepEdge(
 	}
 	g.edges = append(g.edges, de)
 	g.depEdgesFrom.insert(de)
+	if de.Kind() == SameStage {
+		g.sameStageDepEdgesTo.insert(de)
+	}
 	return nil
 }
 
@@ -284,14 +294,6 @@ func (g *Graph) GetNodeRanks() (nodeRanks map[*scpb.Node]int, err error) {
 			err = rAsErr
 		}
 	}()
-	backCycleExists := func(n *scpb.Node, de *DepEdge) bool {
-		var foundBack bool
-		_ = g.ForEachDepEdgeFrom(de.To(), func(maybeBack *DepEdge) error {
-			foundBack = foundBack || maybeBack.To() == n
-			return nil
-		})
-		return foundBack
-	}
 	l := list.New()
 	marks := make(map[*scpb.Node]bool)
 	var visit func(n *scpb.Node)
@@ -305,18 +307,12 @@ func (g *Graph) GetNodeRanks() (nodeRanks map[*scpb.Node]int, err error) {
 		}
 		marks[n] = false
 		_ = g.ForEachDepEdgeFrom(n, func(de *DepEdge) error {
-			// We want to eliminate cycles caused by swaps. In that
-			// case, we want to pretend that there is no edge from the
-			// add to the drop, and, in that way, the drop is ordered first.
-			if n.Direction == scpb.Target_ADD || !backCycleExists(n, de) {
-				visit(de.To())
-			}
+			visit(de.To())
 			return nil
 		})
 		marks[n] = true
 		l.PushFront(n)
 	}
-
 	_ = g.ForEachNode(func(n *scpb.Node) error {
 		visit(n)
 		return nil

pkg/sql/schemachanger/scgraph/graph_test.go

@@ -65,20 +65,17 @@ func TestGraphRanks(t *testing.T) {
 			expectedRankErr: "graph is not a dag",
 		},
 
-		// We will set up the dependency graph to have a swap:
-		// 1) 0 (adding) depends on 1 (dropping)
-		// 2) 1 (dropping) depends on 0 (adding)
-		// 3) 2 (adding) depends on 0 (adding)
+		// We will set up the dependency graph to have a swap, which won't affect
+		// the fact that there's still a cycle.
 		{
-
 			name:    "dependency graph with a swap",
 			addNode: []bool{true, false, true},
 			depEdges: []depEdge{
 				{0, 1},
 				{1, 0},
 				{2, 0},
 			},
-			expectedOrder: []int{1, 0, 2}, // We expect the drop to be ordered first.
+			expectedRankErr: "graph is not a dag",
 		},
 	}
 
@@ -133,6 +130,7 @@ func TestGraphRanks(t *testing.T) {
 		for _, edge := range tc.depEdges {
 			require.NoError(t, graph.AddDepEdge(
 				fmt.Sprintf("%d to %d", edge.from, edge.to),
+				scgraph.HappensAfter,
 				state.Nodes[edge.from].Target,
 				scpb.Status_PUBLIC,
 				state.Nodes[edge.to].Target,

pkg/sql/schemachanger/scgraph/iteration.go

@@ -61,3 +61,9 @@ type DepEdgeIterator func(de *DepEdge) error
 func (g *Graph) ForEachDepEdgeFrom(n *scpb.Node, it DepEdgeIterator) (err error) {
 	return g.depEdgesFrom.iterateSourceNode(n, it)
 }
+
+// ForEachSameStageDepEdgeTo iterates the dep edges in the graph of kind
+// SameStage which point to the provided node.
+func (g *Graph) ForEachSameStageDepEdgeTo(n *scpb.Node, it DepEdgeIterator) (err error) {
+	return g.sameStageDepEdgesTo.iterateSourceNode(n, it)
+}

pkg/sql/schemachanger/scplan/BUILD.bazel

@@ -12,6 +12,7 @@ go_library(
         "//pkg/sql/schemachanger/scplan/deprules",
         "//pkg/sql/schemachanger/scplan/opgen",
         "//pkg/sql/schemachanger/scplan/scopt",
+        "//pkg/sql/schemachanger/screl",
         "//pkg/util/iterutil",
         "@com_github_cockroachdb_errors//:errors",
     ],

pkg/sql/schemachanger/scplan/deprules/registry.go

@@ -26,7 +26,7 @@ func Apply(g *scgraph.Graph) error {
 			from := r.Var(dr.from).(*scpb.Node)
 			to := r.Var(dr.to).(*scpb.Node)
 			return g.AddDepEdge(
-				dr.name, from.Target, from.Status, to.Target, to.Status,
+				dr.name, dr.kind, from.Target, from.Status, to.Target, to.Status,
 			)
 		}); err != nil {
 			return err
@@ -39,15 +39,16 @@ func Apply(g *scgraph.Graph) error {
 var depRules []rule
 
 type rule struct {
-	name      string
-	from, to  rel.Var
-	q         *rel.Query
-	sameStage bool
+	name     string
+	from, to rel.Var
+	q        *rel.Query
+	kind     scgraph.DepEdgeKind
 }
 
-func register(ruleName string, from, to rel.Var, query *rel.Query) {
+func register(ruleName string, edgeKind scgraph.DepEdgeKind, from, to rel.Var, query *rel.Query) {
 	depRules = append(depRules, rule{
 		name: ruleName,
+		kind: edgeKind,
 		from: from,
 		to:   to,
 		q:    query,