sql: new schema changer does not deterministically rank nodes

Fixes: cockroachdb#72562, cockroachdb#72561 Previously, when we updated the sorting inside the new schema changer to a rank based approach we incorrectly assumed that DepEdges will always be generated in a deterministic fashion. Unfortunately, this turned out to be false, which can lead to ranks for neighbour edges to be unstable. To address this, this patch iterates over DepEdges in a ordered fashion based on the To nodes, fixing intermittent failures inside the new schema changer tests. Release note: None
fqazi · Nov 9, 2021 · a1de3ad · a1de3ad
1 parent 5d68dff
commit a1de3ad
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 45 deletions.
diff --git a/pkg/sql/schemachanger/scgraph/graph.go b/pkg/sql/schemachanger/scgraph/graph.go
@@ -294,7 +294,7 @@ func (g *Graph) GetNodeRanks() map[*scpb.Node]int {
 			return
 		}
 		marks[n] = false
-		_ = g.ForEachDepEdgeFrom(n, func(de *DepEdge) error {
+		_ = g.ForEachDepEdgeFromOrdered(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.

diff --git a/pkg/sql/schemachanger/scgraph/iteration.go b/pkg/sql/schemachanger/scgraph/iteration.go
@@ -11,6 +11,8 @@
 package scgraph
 
 import (
+	"sort"
+
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
 	"github.com/cockroachdb/cockroach/pkg/util/iterutil"
 )
@@ -70,3 +72,24 @@ func (g *Graph) ForEachDepEdgeFrom(n *scpb.Node, it DepEdgeIterator) error {
 	}
 	return nil
 }
+
+// ForEachDepEdgeFromOrdered iterates the dep edges in the graph,
+// sorting them based on the target index.
+func (g *Graph) ForEachDepEdgeFromOrdered(n *scpb.Node, it DepEdgeIterator) error {
+	edges := g.nodeDepEdgesFrom[n]
+	// Order edges based on the target indexes.
+	sortedEdges := make([]*DepEdge, len(edges))
+	copy(sortedEdges, edges)
+	sort.SliceStable(sortedEdges, func(i, j int) bool {
+		return g.targetIdxMap[sortedEdges[i].to.Target] < g.targetIdxMap[sortedEdges[j].to.Target]
+	})
+	for _, e := range sortedEdges {
+		if err := it(e); err != nil {
+			if iterutil.Done(err) {
+				err = nil
+			}
+			return err
+		}
+	}
+	return nil
+}
diff --git a/pkg/sql/schemachanger/scplan/testdata/drop_database b/pkg/sql/schemachanger/scplan/testdata/drop_database
@@ -136,20 +136,10 @@ Stage 1 (non-revertible)
     DependedOnBy: 58
     TableID: 56
   *scop.MarkDescriptorAsDropped
-    DescID: 62
-  *scop.RemoveTypeBackRef
-    DescID: 64
-    TypeID: 62
-  *scop.MarkDescriptorAsDropped
-    DescID: 63
-  *scop.RemoveTypeBackRef
-    DescID: 64
-    TypeID: 63
-  *scop.MarkDescriptorAsDropped
-    DescID: 61
+    DescID: 58
   *scop.RemoveRelationDependedOnBy
-    DependedOnBy: 64
-    TableID: 61
+    DependedOnBy: 59
+    TableID: 58
   *scop.MarkDescriptorAsDropped
     DescID: 59
   *scop.RemoveRelationDependedOnBy
@@ -159,10 +149,20 @@ Stage 1 (non-revertible)
     DependedOnBy: 61
     TableID: 59
   *scop.MarkDescriptorAsDropped
-    DescID: 58
+    DescID: 61
   *scop.RemoveRelationDependedOnBy
-    DependedOnBy: 59
-    TableID: 58
+    DependedOnBy: 64
+    TableID: 61
+  *scop.MarkDescriptorAsDropped
+    DescID: 62
+  *scop.RemoveTypeBackRef
+    DescID: 64
+    TypeID: 62
+  *scop.MarkDescriptorAsDropped
+    DescID: 63
+  *scop.RemoveTypeBackRef
+    DescID: 64
+    TypeID: 63
   *scop.RemoveRelationDependedOnBy
     DependedOnBy: 60
     TableID: 58

diff --git a/pkg/sql/schemachanger/scplan/testdata/drop_schema b/pkg/sql/schemachanger/scplan/testdata/drop_schema
@@ -162,20 +162,10 @@ Stage 1 (non-revertible)
     DependedOnBy: 55
     TableID: 54
   *scop.MarkDescriptorAsDropped
-    DescID: 59
-  *scop.RemoveTypeBackRef
-    DescID: 61
-    TypeID: 59
-  *scop.MarkDescriptorAsDropped
-    DescID: 60
-  *scop.RemoveTypeBackRef
-    DescID: 61
-    TypeID: 60
-  *scop.MarkDescriptorAsDropped
-    DescID: 58
+    DescID: 55
   *scop.RemoveRelationDependedOnBy
-    DependedOnBy: 61
-    TableID: 58
+    DependedOnBy: 56
+    TableID: 55
   *scop.MarkDescriptorAsDropped
     DescID: 56
   *scop.RemoveRelationDependedOnBy
@@ -185,10 +175,20 @@ Stage 1 (non-revertible)
     DependedOnBy: 58
     TableID: 56
   *scop.MarkDescriptorAsDropped
-    DescID: 55
+    DescID: 58
   *scop.RemoveRelationDependedOnBy
-    DependedOnBy: 56
-    TableID: 55
+    DependedOnBy: 61
+    TableID: 58
+  *scop.MarkDescriptorAsDropped
+    DescID: 59
+  *scop.RemoveTypeBackRef
+    DescID: 61
+    TypeID: 59
+  *scop.MarkDescriptorAsDropped
+    DescID: 60
+  *scop.RemoveTypeBackRef
+    DescID: 61
+    TypeID: 60
   *scop.RemoveRelationDependedOnBy
     DependedOnBy: 57
     TableID: 55

diff --git a/pkg/sql/schemachanger/scplan/testdata/drop_view b/pkg/sql/schemachanger/scplan/testdata/drop_view
@@ -70,17 +70,11 @@ Stage 1 (non-revertible)
   *scop.RemoveRelationDependedOnBy
     DependedOnBy: 53
     TableID: 52
-  *scop.RemoveTypeBackRef
-    DescID: 59
-    TypeID: 57
-  *scop.RemoveTypeBackRef
-    DescID: 59
-    TypeID: 58
   *scop.MarkDescriptorAsDropped
-    DescID: 56
+    DescID: 53
   *scop.RemoveRelationDependedOnBy
-    DependedOnBy: 59
-    TableID: 56
+    DependedOnBy: 54
+    TableID: 53
   *scop.MarkDescriptorAsDropped
     DescID: 54
   *scop.RemoveRelationDependedOnBy
@@ -90,10 +84,16 @@ Stage 1 (non-revertible)
     DependedOnBy: 56
     TableID: 54
   *scop.MarkDescriptorAsDropped
-    DescID: 53
+    DescID: 56
   *scop.RemoveRelationDependedOnBy
-    DependedOnBy: 54
-    TableID: 53
+    DependedOnBy: 59
+    TableID: 56
+  *scop.RemoveTypeBackRef
+    DescID: 59
+    TypeID: 57
+  *scop.RemoveTypeBackRef
+    DescID: 59
+    TypeID: 58
   *scop.RemoveRelationDependedOnBy
     DependedOnBy: 55
     TableID: 53