Merge cockroachdb#80272

80272: sql/schemachanger: add a type for RuleName r=ajwerner a=ajwerner

This lets it avoid getting redacted.

Release note: None

Co-authored-by: Andrew Werner <[email protected]>

docs/generated/redact_safe.md

@@ -37,6 +37,7 @@ pkg/sql/catalog/descpb/structured.go | `DescriptorState`
 pkg/sql/catalog/descpb/structured.go | `DescriptorVersion`
 pkg/sql/catalog/descpb/structured.go | `IndexDescriptorVersion`
 pkg/sql/catalog/descpb/structured.go | `MutationID`
+pkg/sql/schemachanger/scplan/internal/scgraph/graph.go | `RuleName`
 pkg/sql/sem/catid/ids.go | `ColumnID`
 pkg/sql/sem/catid/ids.go | `ConstraintID`
 pkg/sql/sem/catid/ids.go | `DescID`

pkg/sql/schemachanger/scplan/internal/rules/helpers.go

@@ -33,7 +33,7 @@ func targetNodeVars(el rel.Var) (element, target, node rel.Var) {
 }
 
 type depRuleSpec struct {
-	ruleName              string
+	ruleName              scgraph.RuleName
 	edgeKind              scgraph.DepEdgeKind
 	targetStatus          scpb.Status
 	from, to              elementSpec
@@ -44,7 +44,7 @@ type depRuleSpec struct {
 }
 
 func depRule(
-	ruleName string,
+	ruleName scgraph.RuleName,
 	edgeKind scgraph.DepEdgeKind,
 	targetStatus scpb.TargetStatus,
 	from, to elementSpec,

pkg/sql/schemachanger/scplan/internal/rules/registry.go

@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/screl"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
+	"github.com/cockroachdb/errors"
 )
 
 // ApplyDepRules adds dependency edges to the graph according to the
@@ -37,7 +38,7 @@ func ApplyDepRules(g *scgraph.Graph) error {
 				dr.name, dr.kind, from.Target, from.CurrentStatus, to.Target, to.CurrentStatus,
 			)
 		}); err != nil {
-			return err
+			return errors.Wrapf(err, "applying dep rule %s", dr.name)
 		}
 		if log.V(2) {
 			log.Infof(
@@ -53,7 +54,7 @@ func ApplyDepRules(g *scgraph.Graph) error {
 // to the registered rules.
 func ApplyOpRules(g *scgraph.Graph) (*scgraph.Graph, error) {
 	db := g.Database()
-	m := make(map[*screl.Node][]string)
+	m := make(map[*screl.Node][]scgraph.RuleName)
 	for _, rule := range registry.opRules {
 		var added int
 		start := timeutil.Now()
@@ -64,7 +65,7 @@ func ApplyOpRules(g *scgraph.Graph) (*scgraph.Graph, error) {
 			return nil
 		})
 		if err != nil {
-			return nil, err
+			return nil, errors.Wrapf(err, "applying op rule %s", rule.name)
 		}
 		if log.V(2) {
 			log.Infof(
@@ -90,25 +91,25 @@ var registry struct {
 }
 
 type registeredDepRule struct {
-	name     string
+	name     scgraph.RuleName
 	from, to rel.Var
 	q        *rel.Query
 	kind     scgraph.DepEdgeKind
 }
 
 type registeredOpRule struct {
-	name string
+	name scgraph.RuleName
 	from rel.Var
 	q    *rel.Query
 }
 
 // registerDepRule registers a rule from which a set of dependency edges will
 // be derived in a graph.
 func registerDepRule(
-	ruleName string, edgeKind scgraph.DepEdgeKind, from, to rel.Var, query *rel.Query,
+	rn scgraph.RuleName, edgeKind scgraph.DepEdgeKind, from, to rel.Var, query *rel.Query,
 ) {
 	registry.depRules = append(registry.depRules, registeredDepRule{
-		name: ruleName,
+		name: rn,
 		kind: edgeKind,
 		from: from,
 		to:   to,
@@ -119,9 +120,9 @@ func registerDepRule(
 // registerOpRule adds a graph q that will label as no-op the op edge originating
 // from this node. There can only be one such edge per node, as per the edge
 // definitions in opgen.
-func registerOpRule(ruleName string, from rel.Var, q *rel.Query) {
+func registerOpRule(rn scgraph.RuleName, from rel.Var, q *rel.Query) {
 	registry.opRules = append(registry.opRules, registeredOpRule{
-		name: ruleName,
+		name: rn,
 		from: from,
 		q:    q,
 	})

pkg/sql/schemachanger/scplan/internal/rules/rules_test.go

@@ -51,7 +51,7 @@ func (r registeredDepRule) MarshalYAML() (interface{}, error) {
 		Kind: yaml.MappingNode,
 		Content: []*yaml.Node{
 			{Kind: yaml.ScalarNode, Value: "name"},
-			{Kind: yaml.ScalarNode, Value: r.name},
+			{Kind: yaml.ScalarNode, Value: string(r.name)},
 			{Kind: yaml.ScalarNode, Value: "from"},
 			{Kind: yaml.ScalarNode, Value: string(r.from)},
 			{Kind: yaml.ScalarNode, Value: "kind"},
@@ -73,7 +73,7 @@ func (r registeredOpRule) MarshalYAML() (interface{}, error) {
 		Kind: yaml.MappingNode,
 		Content: []*yaml.Node{
 			{Kind: yaml.ScalarNode, Value: "name"},
-			{Kind: yaml.ScalarNode, Value: r.name},
+			{Kind: yaml.ScalarNode, Value: string(r.name)},
 			{Kind: yaml.ScalarNode, Value: "from"},
 			{Kind: yaml.ScalarNode, Value: string(r.from)},
 			{Kind: yaml.ScalarNode, Value: "query"},

pkg/sql/schemachanger/scplan/internal/scgraph/BUILD.bazel

@@ -19,6 +19,7 @@ go_library(
         "//pkg/sql/schemachanger/screl",
         "//pkg/util/iterutil",
         "@com_github_cockroachdb_errors//:errors",
+        "@com_github_cockroachdb_redact//:redact",
         "@com_github_google_btree//:btree",
     ],
 )

pkg/sql/schemachanger/scplan/internal/scgraph/edge.go

@@ -93,7 +93,7 @@ type DepEdge struct {
 
 	// TODO(ajwerner): Deal with the possibility that multiple rules could
 	// generate the same edge.
-	rule string
+	rule RuleName
 }
 
 // From implements the Edge interface.
@@ -103,7 +103,7 @@ func (de *DepEdge) From() *screl.Node { return de.from }
 func (de *DepEdge) To() *screl.Node { return de.to }
 
 // Name returns the name of the rule which generated this edge.
-func (de *DepEdge) Name() string { return de.rule }
+func (de *DepEdge) Name() RuleName { return de.rule }
 
 // Kind returns the kind of the DepEdge.
 func (de *DepEdge) Kind() DepEdgeKind { return de.kind }

pkg/sql/schemachanger/scplan/internal/scgraph/graph.go

@@ -18,6 +18,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/screl"
 	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/redact"
 )
 
 // Graph is a graph whose nodes are *scpb.Nodes. Graphs are constructed during
@@ -50,13 +51,22 @@ type Graph struct {
 
 	// noOpOpEdges that are marked optimized out, and will not generate
 	// any operations.
-	noOpOpEdges map[*OpEdge]map[string]struct{}
+	noOpOpEdges map[*OpEdge]map[RuleName]struct{}
 
 	edges []Edge
 
 	entities *rel.Database
 }
 
+// RuleName is the name of a rule. It exists as a type to avoid redaction and
+// clarify the meaning of the string.
+type RuleName string
+
+// SafeValue makes RuleName a redact.SafeValue.
+func (r RuleName) SafeValue() {}
+
+var _ redact.SafeValue = RuleName("")
+
 // Database returns a database of the graph's underlying entities.
 func (g *Graph) Database() *rel.Database {
 	return g.entities
@@ -77,7 +87,7 @@ func New(cs scpb.CurrentState) (*Graph, error) {
 	g := Graph{
 		targetIdxMap: map[*scpb.Target]int{},
 		opEdgesFrom:  map[*screl.Node]*OpEdge{},
-		noOpOpEdges:  map[*OpEdge]map[string]struct{}{},
+		noOpOpEdges:  map[*OpEdge]map[RuleName]struct{}{},
 		opToOpEdge:   map[scop.Op]*OpEdge{},
 		entities:     db,
 	}
@@ -114,7 +124,7 @@ func (g *Graph) ShallowClone() *Graph {
 		opToOpEdge:   g.opToOpEdge,
 		edges:        g.edges,
 		entities:     g.entities,
-		noOpOpEdges:  make(map[*OpEdge]map[string]struct{}),
+		noOpOpEdges:  make(map[*OpEdge]map[RuleName]struct{}),
 	}
 	// Any decorations for mutations will be copied.
 	for edge, noop := range g.noOpOpEdges {
@@ -219,7 +229,7 @@ func (g *Graph) GetOpEdgeFromOp(op scop.Op) *OpEdge {
 // AddDepEdge adds a dep edge connecting two nodes (specified by their targets
 // and statuses).
 func (g *Graph) AddDepEdge(
-	rule string,
+	rule RuleName,
 	kind DepEdgeKind,
 	fromTarget *scpb.Target,
 	fromStatus scpb.Status,
@@ -241,8 +251,8 @@ func (g *Graph) AddDepEdge(
 
 // MarkAsNoOp marks an edge as no-op, so that no operations are emitted from
 // this edge during planning.
-func (g *Graph) MarkAsNoOp(edge *OpEdge, rule ...string) {
-	m := make(map[string]struct{})
+func (g *Graph) MarkAsNoOp(edge *OpEdge, rule ...RuleName) {
+	m := make(map[RuleName]struct{})
 	for _, r := range rule {
 		m[r] = struct{}{}
 	}
@@ -259,15 +269,17 @@ func (g *Graph) IsNoOp(edge *OpEdge) bool {
 }
 
 // NoOpRules returns the rules which caused the edge to not emit any operations.
-func (g *Graph) NoOpRules(edge *OpEdge) (rules []string) {
+func (g *Graph) NoOpRules(edge *OpEdge) (rules []RuleName) {
 	if !g.IsNoOp(edge) {
 		return nil
 	}
 	m := g.noOpOpEdges[edge]
 	for rule := range m {
 		rules = append(rules, rule)
 	}
-	sort.Strings(rules)
+	sort.Slice(rules, func(i, j int) bool {
+		return rules[i] < rules[j]
+	})
 	return rules
 }
 

pkg/sql/schemachanger/scplan/internal/scgraph/graph_test.go

@@ -124,7 +124,7 @@ func TestGraphRanks(t *testing.T) {
 		// Add the dep edges next.
 		for _, edge := range tc.depEdges {
 			require.NoError(t, graph.AddDepEdge(
-				fmt.Sprintf("%d to %d", edge.from, edge.to),
+				scgraph.RuleName(fmt.Sprintf("%d to %d", edge.from, edge.to)),
 				scgraph.Precedence,
 				&ts.Targets[edge.from],
 				scpb.Status_PUBLIC,