From 0657d0991d0c0b12b466843a0cf763f48dcb1ac1 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Wed, 25 May 2022 15:53:36 -0400 Subject: [PATCH 1/2] sql/multiregion: move telemetry names to break catpb->tree dep It was a bottleneck in the build graph. Release note: None --- pkg/BUILD.bazel | 1 + pkg/sql/alter_table_locality.go | 4 +- pkg/sql/catalog/catpb/BUILD.bazel | 8 +++ pkg/sql/catalog/catpb/multiregion.go | 27 --------- pkg/sql/catalog/multiregion/BUILD.bazel | 1 + pkg/sql/catalog/multiregion/telemetry.go | 71 ++++++++++++++++++++++++ pkg/sql/create_table.go | 2 +- pkg/sql/sem/tree/region.go | 29 ---------- 8 files changed, 84 insertions(+), 59 deletions(-) create mode 100644 pkg/sql/catalog/multiregion/telemetry.go diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index f9e3949656c1..07aefe038777 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -229,6 +229,7 @@ ALL_TESTS = [ "//pkg/sql/backfill:backfill_test", "//pkg/sql/catalog/catalogkeys:catalogkeys_test", "//pkg/sql/catalog/catformat:catformat_test", + "//pkg/sql/catalog/catpb:catpb_disallowed_imports_test", "//pkg/sql/catalog/catpb:catpb_test", "//pkg/sql/catalog/catprivilege:catprivilege_test", "//pkg/sql/catalog/colinfo:colinfo_test", diff --git a/pkg/sql/alter_table_locality.go b/pkg/sql/alter_table_locality.go index aee7ab6f4ed4..f0c9932f98c7 100644 --- a/pkg/sql/alter_table_locality.go +++ b/pkg/sql/alter_table_locality.go @@ -462,14 +462,14 @@ func (n *alterTableSetLocalityNode) startExec(params runParams) error { newLocality := n.n.Locality existingLocality := n.tableDesc.LocalityConfig - existingLocalityTelemetryName, err := existingLocality.TelemetryName() + existingLocalityTelemetryName, err := multiregion.TelemetryNameForLocalityConfig(existingLocality) if err != nil { return err } telemetry.Inc( sqltelemetry.AlterTableLocalityCounter( existingLocalityTelemetryName, - newLocality.TelemetryName(), + multiregion.TelemetryNameForLocality(newLocality), ), ) diff --git a/pkg/sql/catalog/catpb/BUILD.bazel b/pkg/sql/catalog/catpb/BUILD.bazel index 34b45e57c20b..7d6520132ba9 100644 --- a/pkg/sql/catalog/catpb/BUILD.bazel +++ b/pkg/sql/catalog/catpb/BUILD.bazel @@ -1,6 +1,7 @@ load("@rules_proto//proto:defs.bzl", "proto_library") load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library") +load("//pkg/testutils/buildutil:buildutil.bzl", "disallowed_imports_test") load("//build:STRINGER.bzl", "stringer") proto_library( @@ -75,3 +76,10 @@ stringer( src = "privilege.go", typ = "PrivilegeDescVersion", ) + +disallowed_imports_test( + src = "catpb", + disallowed_list = [ + "//pkg/sql/sem/tree", + ], +) diff --git a/pkg/sql/catalog/catpb/multiregion.go b/pkg/sql/catalog/catpb/multiregion.go index 7e0b48d11205..623b421e80dd 100644 --- a/pkg/sql/catalog/catpb/multiregion.go +++ b/pkg/sql/catalog/catpb/multiregion.go @@ -10,11 +10,6 @@ package catpb -import ( - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" - "github.com/cockroachdb/errors" -) - // RegionName is an alias for a region stored on the database. type RegionName string @@ -34,25 +29,3 @@ func (regions RegionNames) ToStrings() []string { } return ret } - -// TelemetryName returns the name to use for the given locality. -func (cfg *LocalityConfig) TelemetryName() (string, error) { - switch l := cfg.Locality.(type) { - case *LocalityConfig_Global_: - return tree.TelemetryNameGlobal, nil - case *LocalityConfig_RegionalByTable_: - if l.RegionalByTable.Region != nil { - return tree.TelemetryNameRegionalByTableIn, nil - } - return tree.TelemetryNameRegionalByTable, nil - case *LocalityConfig_RegionalByRow_: - if l.RegionalByRow.As != nil { - return tree.TelemetryNameRegionalByRowAs, nil - } - return tree.TelemetryNameRegionalByRow, nil - } - return "", errors.AssertionFailedf( - "unknown locality config TelemetryName: type %T", - cfg.Locality, - ) -} diff --git a/pkg/sql/catalog/multiregion/BUILD.bazel b/pkg/sql/catalog/multiregion/BUILD.bazel index 70c4b417f640..c7b0a8a59992 100644 --- a/pkg/sql/catalog/multiregion/BUILD.bazel +++ b/pkg/sql/catalog/multiregion/BUILD.bazel @@ -4,6 +4,7 @@ go_library( name = "multiregion", srcs = [ "region_config.go", + "telemetry.go", "validate_table.go", ], importpath = "github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion", diff --git a/pkg/sql/catalog/multiregion/telemetry.go b/pkg/sql/catalog/multiregion/telemetry.go new file mode 100644 index 000000000000..739ca9f6840c --- /dev/null +++ b/pkg/sql/catalog/multiregion/telemetry.go @@ -0,0 +1,71 @@ +// 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 multiregion + +import ( + "fmt" + + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" + "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/errors" +) + +// Constants to use for telemetry for multi-region table localities. +const ( + TelemetryNameGlobal = "global" + TelemetryNameRegionalByTable = "regional_by_table" + TelemetryNameRegionalByTableIn = "regional_by_table_in" + TelemetryNameRegionalByRow = "regional_by_row" + TelemetryNameRegionalByRowAs = "regional_by_row_as" +) + +// TelemetryNameForLocality returns the telemetry name for a given locality level. +func TelemetryNameForLocality(node *tree.Locality) string { + switch node.LocalityLevel { + case tree.LocalityLevelGlobal: + return TelemetryNameGlobal + case tree.LocalityLevelTable: + if node.TableRegion != tree.RegionalByRowRegionNotSpecifiedName { + return TelemetryNameRegionalByTableIn + } + return TelemetryNameRegionalByTable + case tree.LocalityLevelRow: + if node.RegionalByRowColumn != tree.PrimaryRegionNotSpecifiedName { + return TelemetryNameRegionalByRowAs + } + return TelemetryNameRegionalByRow + default: + panic(fmt.Sprintf("unknown locality: %#v", node.LocalityLevel)) + } +} + +// TelemetryNameForLocalityConfig returns the name to use for the given +// locality config. +func TelemetryNameForLocalityConfig(cfg *catpb.LocalityConfig) (string, error) { + switch l := cfg.Locality.(type) { + case *catpb.LocalityConfig_Global_: + return TelemetryNameGlobal, nil + case *catpb.LocalityConfig_RegionalByTable_: + if l.RegionalByTable.Region != nil { + return TelemetryNameRegionalByTableIn, nil + } + return TelemetryNameRegionalByTable, nil + case *catpb.LocalityConfig_RegionalByRow_: + if l.RegionalByRow.As != nil { + return TelemetryNameRegionalByRowAs, nil + } + return TelemetryNameRegionalByRow, nil + } + return "", errors.AssertionFailedf( + "unknown locality config TelemetryNameForLocality: type %T", + cfg.Locality, + ) +} diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index ad4169d6cf3f..2767cffc35ea 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -2253,7 +2253,7 @@ func NewTableDesc( if regionConfig != nil || n.Locality != nil { localityTelemetryName := "unspecified" if n.Locality != nil { - localityTelemetryName = n.Locality.TelemetryName() + localityTelemetryName = multiregion.TelemetryNameForLocality(n.Locality) } telemetry.Inc(sqltelemetry.CreateTableLocalityCounter(localityTelemetryName)) if n.Locality == nil { diff --git a/pkg/sql/sem/tree/region.go b/pkg/sql/sem/tree/region.go index cf551ef9ce20..138bcbc7e716 100644 --- a/pkg/sql/sem/tree/region.go +++ b/pkg/sql/sem/tree/region.go @@ -56,35 +56,6 @@ type Locality struct { RegionalByRowColumn Name } -// Constants to use for telemetry for multi-region table localities. -const ( - TelemetryNameGlobal = "global" - TelemetryNameRegionalByTable = "regional_by_table" - TelemetryNameRegionalByTableIn = "regional_by_table_in" - TelemetryNameRegionalByRow = "regional_by_row" - TelemetryNameRegionalByRowAs = "regional_by_row_as" -) - -// TelemetryName returns the telemetry name for a given locality level. -func (node *Locality) TelemetryName() string { - switch node.LocalityLevel { - case LocalityLevelGlobal: - return TelemetryNameGlobal - case LocalityLevelTable: - if node.TableRegion != RegionalByRowRegionNotSpecifiedName { - return TelemetryNameRegionalByTableIn - } - return TelemetryNameRegionalByTable - case LocalityLevelRow: - if node.RegionalByRowColumn != PrimaryRegionNotSpecifiedName { - return TelemetryNameRegionalByRowAs - } - return TelemetryNameRegionalByRow - default: - panic(fmt.Sprintf("unknown locality: %#v", node.LocalityLevel)) - } -} - // Format implements the NodeFormatter interface. func (node *Locality) Format(ctx *FmtCtx) { ctx.WriteString("LOCALITY ") From cb55b51f02509711ec4bf638eac96c2db421c185 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Wed, 25 May 2022 15:54:28 -0400 Subject: [PATCH 2/2] sql/sem/builtins: remove dependency of builtins on sql/catalog/descs builtins is a major bottleneck in the build graph. Reducing its deps helps move it to be earlier. Release note: None --- pkg/BUILD.bazel | 1 + pkg/sql/sem/builtins/BUILD.bazel | 9 ++++++++- pkg/sql/sem/builtins/builtins.go | 15 ++++----------- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index 07aefe038777..e7d8e511401e 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -387,6 +387,7 @@ ALL_TESTS = [ "//pkg/sql/schemachanger/screl:screl_test", "//pkg/sql/schemachanger/scrun:scrun_test", "//pkg/sql/schemachanger:schemachanger_test", + "//pkg/sql/sem/builtins:builtins_disallowed_imports_test", "//pkg/sql/sem/builtins:builtins_test", "//pkg/sql/sem/cast:cast_test", "//pkg/sql/sem/catconstants:catconstants_disallowed_imports_test", diff --git a/pkg/sql/sem/builtins/BUILD.bazel b/pkg/sql/sem/builtins/BUILD.bazel index 73ee89806c82..7474fbea58a8 100644 --- a/pkg/sql/sem/builtins/BUILD.bazel +++ b/pkg/sql/sem/builtins/BUILD.bazel @@ -1,4 +1,5 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") +load("//pkg/testutils/buildutil:buildutil.bzl", "disallowed_imports_test") go_library( name = "builtins", @@ -50,7 +51,6 @@ go_library( "//pkg/sql/catalog", "//pkg/sql/catalog/catalogkeys", "//pkg/sql/catalog/descpb", - "//pkg/sql/catalog/descs", "//pkg/sql/colexecerror", "//pkg/sql/lex", "//pkg/sql/lexbase", @@ -168,3 +168,10 @@ go_test( "@com_github_stretchr_testify//require", ], ) + +disallowed_imports_test( + src = "builtins", + disallowed_list = [ + "//pkg/sql/catalog/descs", + ], +) diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index d3df4489f7ae..8643d49f0675 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -50,7 +50,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/colexecerror" "github.com/cockroachdb/cockroach/pkg/sql/lex" "github.com/cockroachdb/cockroach/pkg/sql/lexbase" @@ -5558,14 +5557,11 @@ value if you rely on the HLC for accuracy.`, tableID := int(tree.MustBeDInt(args[0])) indexID := int(tree.MustBeDInt(args[1])) g := tree.MustBeDGeography(args[2]) - // TODO(postamar): give the eval.Context a useful interface - // instead of cobbling a descs.Collection in this way. - cf := descs.NewBareBonesCollectionFactory(ctx.Settings, ctx.Codec) - descsCol := cf.MakeCollection(ctx.Context, descs.NewTemporarySchemaProvider(ctx.SessionDataStack), nil /* monitor */) - tableDesc, err := descsCol.Direct().MustGetTableDescByID(ctx.Ctx(), ctx.Txn, descpb.ID(tableID)) + tableI, err := ctx.Planner.GetImmutableTableInterfaceByID(ctx.Ctx(), tableID) if err != nil { return nil, err } + tableDesc := tableI.(catalog.TableDescriptor) index, err := tableDesc.FindIndexWithID(descpb.IndexID(indexID)) if err != nil { return nil, err @@ -5596,14 +5592,11 @@ value if you rely on the HLC for accuracy.`, tableID := int(tree.MustBeDInt(args[0])) indexID := int(tree.MustBeDInt(args[1])) g := tree.MustBeDGeometry(args[2]) - // TODO(postamar): give the eval.Context a useful interface - // instead of cobbling a descs.Collection in this way. - cf := descs.NewBareBonesCollectionFactory(ctx.Settings, ctx.Codec) - descsCol := cf.MakeCollection(ctx.Context, descs.NewTemporarySchemaProvider(ctx.SessionDataStack), nil /* monitor */) - tableDesc, err := descsCol.Direct().MustGetTableDescByID(ctx.Ctx(), ctx.Txn, descpb.ID(tableID)) + tableI, err := ctx.Planner.GetImmutableTableInterfaceByID(ctx.Ctx(), tableID) if err != nil { return nil, err } + tableDesc := tableI.(catalog.TableDescriptor) index, err := tableDesc.FindIndexWithID(descpb.IndexID(indexID)) if err != nil { return nil, err