From 03d46ee5135ce9eb6ef4327c6cddaed85854618c Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Tue, 25 Feb 2025 18:20:20 -0500 Subject: [PATCH 1/3] sql: remove unused field in VirtualSchemaHolder Release note: None --- pkg/sql/virtual_schema.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/pkg/sql/virtual_schema.go b/pkg/sql/virtual_schema.go index a87bf186ac1f..6e33d8d716b4 100644 --- a/pkg/sql/virtual_schema.go +++ b/pkg/sql/virtual_schema.go @@ -444,9 +444,6 @@ type VirtualSchemaHolder struct { orderedNames []string catalogCache nstree.MutableCatalog - // Needed for backward-compat on crdb_internal.ranges{_no_leases}. - // Remove in v23.2. - st *cluster.Settings } var _ VirtualTabler = (*VirtualSchemaHolder)(nil) @@ -930,10 +927,6 @@ func NewVirtualSchemaHolder( schemasByID: make(map[descpb.ID]*virtualSchemaEntry, len(virtualSchemas)), orderedNames: make([]string, len(virtualSchemas)), defsByID: make(map[descpb.ID]*virtualDefEntry, math.MaxUint32-catconstants.MinVirtualID), - - // Needed for backward-compat on crdb_internal.ranges{_no_leases}. - // Remove in v23.2. - st: st, } order := 0 From b44ea2b684fe2faa4fa2c8c0c4f22a3e009230b3 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Tue, 25 Feb 2025 18:40:21 -0500 Subject: [PATCH 2/3] sql: initialize virtual table descriptors concurrently This helps improve TestServer startup time. While working on this, I experimented with adding a concurrency limit, which didn't turn out to matter. However, I still kept the addition of SetLimit to the ctxgroup API, since it is generally useful. Release note: None --- pkg/sql/virtual_schema.go | 31 +++++++++++++++++++++++-------- pkg/util/ctxgroup/ctxgroup.go | 10 ++++++++++ 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/pkg/sql/virtual_schema.go b/pkg/sql/virtual_schema.go index 6e33d8d716b4..413f062ecd32 100644 --- a/pkg/sql/virtual_schema.go +++ b/pkg/sql/virtual_schema.go @@ -41,10 +41,12 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/util/ctxgroup" "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/iterutil" "github.com/cockroachdb/cockroach/pkg/util/stop" + "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/errors" "github.com/cockroachdb/redact" ) @@ -978,16 +980,29 @@ func NewVirtualSchemaHolder( return tableDesc, entry, nil } + // Initialize virtual tables concurrently. This happens all at once during + // server startup, which is a bottleneck for startup time, especially in + // the TestServer used by unit tests. Adding concurrency here speeds up + // TestServer startup by about 7% in SharedTenant mode. + g := ctxgroup.WithContext(ctx) + var mu syncutil.Mutex for id, def := range schema.tableDefs { - tableDesc, entry, err := doTheWork(id, def, false /* bumpVersion */) - if err != nil { - return nil, err - } - defs[tableDesc.Name] = entry - vs.defsByID[tableDesc.ID] = entry - orderedDefNames = append(orderedDefNames, tableDesc.Name) + g.GoCtx(func(ctx context.Context) error { + tableDesc, entry, err := doTheWork(id, def, false /* bumpVersion */) + if err != nil { + return err + } + mu.Lock() + defer mu.Unlock() + defs[tableDesc.Name] = entry + vs.defsByID[tableDesc.ID] = entry + orderedDefNames = append(orderedDefNames, tableDesc.Name) + return nil + }) + } + if err := g.Wait(); err != nil { + return nil, err } - sort.Strings(orderedDefNames) vse := &virtualSchemaEntry{ diff --git a/pkg/util/ctxgroup/ctxgroup.go b/pkg/util/ctxgroup/ctxgroup.go index f2bc811b5d78..8e2d5fca2461 100644 --- a/pkg/util/ctxgroup/ctxgroup.go +++ b/pkg/util/ctxgroup/ctxgroup.go @@ -166,6 +166,16 @@ func WithContext(ctx context.Context) Group { } } +// SetLimit limits the number of active goroutines in this group to at most n. A +// negative value indicates no limit. A limit of zero will prevent any new +// goroutines from being added. Any subsequent call to the Go method will block +// until it can add an active goroutine without exceeding the configured limit. +// The limit must not be modified while any goroutines in the group are active. +// This delegates to errgroup.Group.SetLimit. +func (g Group) SetLimit(n int) { + g.wrapped.SetLimit(n) +} + // Go calls the given function in a new goroutine. func (g Group) Go(f func() error) { g.wrapped.Go(f) From 959ef1f5c66978b9140317a4c9cd2079cc1ecf4d Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Wed, 26 Feb 2025 10:15:09 -0500 Subject: [PATCH 3/3] sql: don't validate virtual tables upon creation under tests This patch skips an expensive validation step when running in a unit test. The virtual tables are all initialized on startup, and validating all of them at once leads to many heap allocations that cause TestServer startup to take longer. Release note: None --- pkg/sql/catalog/tabledesc/BUILD.bazel | 1 + pkg/sql/catalog/tabledesc/structured.go | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/pkg/sql/catalog/tabledesc/BUILD.bazel b/pkg/sql/catalog/tabledesc/BUILD.bazel index 5f1e15b597ca..83cbf03c972d 100644 --- a/pkg/sql/catalog/tabledesc/BUILD.bazel +++ b/pkg/sql/catalog/tabledesc/BUILD.bazel @@ -60,6 +60,7 @@ go_library( "//pkg/sql/types", "//pkg/sql/vecindex/vecpb", "//pkg/util", + "//pkg/util/buildutil", "//pkg/util/errorutil/unimplemented", "//pkg/util/hlc", "//pkg/util/interval", diff --git a/pkg/sql/catalog/tabledesc/structured.go b/pkg/sql/catalog/tabledesc/structured.go index 3bfbf1a5a9c0..cafa664162f2 100644 --- a/pkg/sql/catalog/tabledesc/structured.go +++ b/pkg/sql/catalog/tabledesc/structured.go @@ -33,6 +33,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/types" + "github.com/cockroachdb/cockroach/pkg/util/buildutil" "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/intsets" @@ -636,6 +637,14 @@ func (desc *Mutable) AllocateIDs(ctx context.Context, version clusterversion.Clu return err } + // The virtual table descriptors are entirely under our control. Since + // validating is expensive, skip it for virtual tables if this code is running + // in a test environment. This step was found to be a bottleneck in TestServer + // startup. + if desc.IsVirtualTable() && buildutil.CrdbTestBuild { + return nil + } + // This is sort of ugly. If the descriptor does not have an ID, we hack one in // to pass the table ID check. We use a non-reserved ID, reserved ones being set // before AllocateIDs.