diff --git a/docs/generated/redact_safe.md b/docs/generated/redact_safe.md index 916763f4e7ea..a196a22d18df 100644 --- a/docs/generated/redact_safe.md +++ b/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` diff --git a/pkg/sql/schemachanger/scplan/internal/rules/helpers.go b/pkg/sql/schemachanger/scplan/internal/rules/helpers.go index 10a262caa73f..96cafec0af2a 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/helpers.go +++ b/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, diff --git a/pkg/sql/schemachanger/scplan/internal/rules/registry.go b/pkg/sql/schemachanger/scplan/internal/rules/registry.go index 5ba36526f235..d9618cd290cf 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/registry.go +++ b/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,14 +91,14 @@ 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 } @@ -105,10 +106,10 @@ type registeredOpRule struct { // 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, }) diff --git a/pkg/sql/schemachanger/scplan/internal/rules/rules_test.go b/pkg/sql/schemachanger/scplan/internal/rules/rules_test.go index 106dc4b5153a..b1f6d4046932 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/rules_test.go +++ b/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"}, diff --git a/pkg/sql/schemachanger/scplan/internal/scgraph/BUILD.bazel b/pkg/sql/schemachanger/scplan/internal/scgraph/BUILD.bazel index 7542d03c709a..374b4cab98f4 100644 --- a/pkg/sql/schemachanger/scplan/internal/scgraph/BUILD.bazel +++ b/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", ], ) diff --git a/pkg/sql/schemachanger/scplan/internal/scgraph/edge.go b/pkg/sql/schemachanger/scplan/internal/scgraph/edge.go index bdc6adeba5af..9f3c9e7a3a59 100644 --- a/pkg/sql/schemachanger/scplan/internal/scgraph/edge.go +++ b/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 } diff --git a/pkg/sql/schemachanger/scplan/internal/scgraph/graph.go b/pkg/sql/schemachanger/scplan/internal/scgraph/graph.go index 2037f90f4f1e..2c55b31eaf47 100644 --- a/pkg/sql/schemachanger/scplan/internal/scgraph/graph.go +++ b/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,7 +269,7 @@ 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 } @@ -267,7 +277,9 @@ func (g *Graph) NoOpRules(edge *OpEdge) (rules []string) { 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 } diff --git a/pkg/sql/schemachanger/scplan/internal/scgraph/graph_test.go b/pkg/sql/schemachanger/scplan/internal/scgraph/graph_test.go index 995e94802867..bc01520c604f 100644 --- a/pkg/sql/schemachanger/scplan/internal/scgraph/graph_test.go +++ b/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,