Skip to content

Commit e58fa7d

Browse files
craig[bot]aerfreiyuzefovich
committed
152415: changefeedccl: allow for more tables in bank workload r=KeithCh a=aerfrei When running the bank workload with thousands of tables, the workload would fail to initialize all the tables because it was using too high concurrency. This PR sets a sensible default, allowing us to add tests of multi-table changefeed functionality with more tables (up to 50_000). Epic: CRDB-1421 Release note: None 152478: sql: break dependencies on sql/parser r=yuzefovich a=yuzefovich This PR contains multiple commits that break dependency on `sql/parser` from multiple packages that are on the critical path for builds. Optimizing `sql/parser` build time doesn't seem trivial, but doing more work in parallel with that is the next best option. The general idea is to inject the necessary parser functions into packages that need them at `init` time. See each commit for more details. Informs: #79357. Epic: None Release note: None Co-authored-by: Aerin Freilich <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]>
3 parents bdefcf7 + c925e86 + 15efbd0 commit e58fa7d

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

64 files changed

+477
-268
lines changed

‎pkg/BUILD.bazel‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,7 @@ ALL_TESTS = [
360360
"//pkg/server/pgurl:pgurl_test",
361361
"//pkg/server/privchecker:privchecker_test",
362362
"//pkg/server/profiler:profiler_test",
363+
"//pkg/server/serverpb:serverpb_disallowed_imports_test",
363364
"//pkg/server/serverpb:serverpb_test",
364365
"//pkg/server/serverrules:serverrules_test",
365366
"//pkg/server/settingswatcher:settingswatcher_test",
@@ -407,6 +408,7 @@ ALL_TESTS = [
407408
"//pkg/sql/catalog/descpb:descpb_test",
408409
"//pkg/sql/catalog/descs:descs_test",
409410
"//pkg/sql/catalog/externalcatalog:externalcatalog_test",
411+
"//pkg/sql/catalog/funcdesc:funcdesc_disallowed_imports_test",
410412
"//pkg/sql/catalog/funcdesc:funcdesc_test",
411413
"//pkg/sql/catalog/hydrateddesccache:hydrateddesccache_test",
412414
"//pkg/sql/catalog/internal/catkv:catkv_test",
@@ -419,6 +421,7 @@ ALL_TESTS = [
419421
"//pkg/sql/catalog/replication:replication_test",
420422
"//pkg/sql/catalog/resolver:resolver_test",
421423
"//pkg/sql/catalog/schemadesc:schemadesc_test",
424+
"//pkg/sql/catalog/schemaexpr:schemaexpr_disallowed_imports_test",
422425
"//pkg/sql/catalog/schemaexpr:schemaexpr_test",
423426
"//pkg/sql/catalog/schematelemetry:schematelemetry_test",
424427
"//pkg/sql/catalog/seqexpr:seqexpr_disallowed_imports_test",
@@ -567,6 +570,7 @@ ALL_TESTS = [
567570
"//pkg/sql/opt:opt_test",
568571
"//pkg/sql/parser:parser_disallowed_imports_test",
569572
"//pkg/sql/parser:parser_test",
573+
"//pkg/sql/parserutils:parserutils_disallowed_imports_test",
570574
"//pkg/sql/pgrepl/pgreplparser:pgreplparser_test",
571575
"//pkg/sql/pgrepl:pgrepl_test",
572576
"//pkg/sql/pgwire/hba:hba_test",
@@ -2373,6 +2377,7 @@ GO_TARGETS = [
23732377
"//pkg/sql/sqlstats/insights/integration:integration_test",
23742378
"//pkg/sql/sqlstats/insights:insights",
23752379
"//pkg/sql/sqlstats/insights:insights_test",
2380+
"//pkg/sql/sqlstats/insightspb:insightspb",
23762381
"//pkg/sql/sqlstats/persistedsqlstats/sqlstatstestutil:sqlstatstestutil",
23772382
"//pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil:sqlstatsutil",
23782383
"//pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil:sqlstatsutil_test",

‎pkg/cmd/roachtest/tests/cdc.go‎

Lines changed: 52 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1787,6 +1787,7 @@ func runMessageTooLarge(ctx context.Context, t test.Test, c cluster.Cluster) {
17871787

17881788
type multiTablePTSBenchmarkParams struct {
17891789
numTables int
1790+
numRanges int
17901791
numRows int
17911792
duration string
17921793
}
@@ -1812,8 +1813,13 @@ func runCDCMultiTablePTSBenchmark(
18121813
t.Fatalf("failed to set cluster settings: %v", err)
18131814
}
18141815

1815-
initCmd := fmt.Sprintf("./cockroach workload init bank --rows=%d --num-tables=%d {pgurl%s}",
1816-
params.numRows, params.numTables, ct.crdbNodes.RandNode())
1816+
numRanges := 10
1817+
if params.numRanges > 0 {
1818+
numRanges = params.numRanges
1819+
}
1820+
1821+
initCmd := fmt.Sprintf("./cockroach workload init bank --rows=%d --ranges=%d --num-tables=%d {pgurl%s}",
1822+
params.numRows, numRanges, params.numTables, ct.crdbNodes.RandNode())
18171823
if err := c.RunE(ctx, option.WithNodes(ct.workloadNode), initCmd); err != nil {
18181824
t.Fatalf("failed to initialize bank tables: %v", err)
18191825
}
@@ -1855,11 +1861,11 @@ func runCDCMultiTablePTSBenchmark(
18551861
t.Status("workload finished, verifying metrics")
18561862

18571863
// These metrics are in nanoseconds, so we are asserting that both
1858-
// of these latency metrics are less than 10 milliseconds.
1859-
ct.verifyMetrics(ctx, verifyMetricsUnderThreshold([]string{
1864+
// of these latency metrics are less than 25 milliseconds.
1865+
ct.verifyMetrics(ctx, ct.verifyMetricsUnderThreshold([]string{
18601866
"changefeed_stage_pts_manage_latency",
18611867
"changefeed_stage_pts_create_latency",
1862-
}, float64(10*time.Millisecond)))
1868+
}, float64(25*time.Millisecond)))
18631869

18641870
t.Status("multi-table PTS benchmark finished")
18651871
}
@@ -2897,7 +2903,7 @@ func registerCDC(r registry.Registry) {
28972903
Run: runMessageTooLarge,
28982904
})
28992905
r.Add(registry.TestSpec{
2900-
Name: "cdc/multi-table-pts-benchmark",
2906+
Name: "cdc/multi-table-pts-benchmark/num-tables=500",
29012907
Owner: registry.OwnerCDC,
29022908
Benchmark: true,
29032909
Cluster: r.MakeClusterSpec(4, spec.CPU(16), spec.WorkloadNode()),
@@ -2913,6 +2919,43 @@ func registerCDC(r registry.Registry) {
29132919
runCDCMultiTablePTSBenchmark(ctx, t, c, params)
29142920
},
29152921
})
2922+
r.Add(registry.TestSpec{
2923+
Name: "cdc/multi-table-pts-benchmark/num-tables=5000",
2924+
Owner: registry.OwnerCDC,
2925+
Benchmark: true,
2926+
Cluster: r.MakeClusterSpec(4, spec.CPU(16), spec.WorkloadNode()),
2927+
CompatibleClouds: registry.AllClouds,
2928+
Suites: registry.Suites(registry.Nightly),
2929+
Timeout: 1 * time.Hour,
2930+
Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
2931+
params := multiTablePTSBenchmarkParams{
2932+
numTables: 5000,
2933+
numRows: 100,
2934+
duration: "20m",
2935+
}
2936+
runCDCMultiTablePTSBenchmark(ctx, t, c, params)
2937+
},
2938+
})
2939+
r.Add(registry.TestSpec{
2940+
Name: "cdc/multi-table-pts-benchmark/num-tables=50000",
2941+
Owner: registry.OwnerCDC,
2942+
Benchmark: true,
2943+
Cluster: r.MakeClusterSpec(4, spec.CPU(16), spec.WorkloadNode()),
2944+
CompatibleClouds: registry.AllClouds,
2945+
Suites: registry.Suites(registry.Nightly),
2946+
Timeout: 1 * time.Hour,
2947+
Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
2948+
params := multiTablePTSBenchmarkParams{
2949+
numTables: 50_000,
2950+
// Splitting tables into ranges slows down test setup at this scale.
2951+
// Therefore, we don't split the tables into multiple ranges.
2952+
numRanges: 1,
2953+
numRows: 10,
2954+
duration: "20m",
2955+
}
2956+
runCDCMultiTablePTSBenchmark(ctx, t, c, params)
2957+
},
2958+
})
29162959
}
29172960

29182961
const (
@@ -4598,7 +4641,7 @@ func verifyMetricsNonZero(names ...string) func(metrics map[string]*prompb.Metri
45984641
}
45994642
}
46004643

4601-
func verifyMetricsUnderThreshold(
4644+
func (ct *cdcTester) verifyMetricsUnderThreshold(
46024645
names []string, threshold float64,
46034646
) func(metrics map[string]*prompb.MetricFamily) (ok bool) {
46044647
namesMap := make(map[string]struct{}, len(names))
@@ -4622,6 +4665,8 @@ func verifyMetricsUnderThreshold(
46224665
observedValue := m.Histogram.GetSampleSum() / float64(m.Histogram.GetSampleCount())
46234666
if observedValue < threshold {
46244667
found[name] = struct{}{}
4668+
} else {
4669+
ct.t.Fatalf("observed value for metric %s over threshold. observedValue: %f, threshold: %f", name, observedValue, threshold)
46254670
}
46264671
}
46274672

‎pkg/gen/protobuf.bzl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ PROTOBUF_SRCS = [
7272
"//pkg/sql/sem/idxtype:idxtype_go_proto",
7373
"//pkg/sql/sem/semenumpb:semenumpb_go_proto",
7474
"//pkg/sql/sessiondatapb:sessiondatapb_go_proto",
75-
"//pkg/sql/sqlstats/insights:insights_go_proto",
75+
"//pkg/sql/sqlstats/insightspb:insightspb_go_proto",
7676
"//pkg/sql/sqlstats/persistedsqlstats:persistedsqlstats_go_proto",
7777
"//pkg/sql/stats:stats_go_proto",
7878
"//pkg/sql/types:types_go_proto",

‎pkg/protos.bzl‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ SERVER_PROTOS = [
3838
"//pkg/sql/sem/idxtype:idxtype_proto",
3939
"//pkg/sql/sem/semenumpb:semenumpb_proto",
4040
"//pkg/sql/sessiondatapb:sessiondatapb_proto",
41-
"//pkg/sql/sqlstats/insights:insights_proto",
41+
"//pkg/sql/sqlstats/insightspb:insightspb_proto",
4242
"//pkg/sql/types:types_proto",
4343
"//pkg/sql/vecindex/vecpb:vecpb_proto",
4444
"//pkg/storage/enginepb:enginepb_proto",
@@ -132,7 +132,7 @@ PROTO_FILES = [
132132
"//pkg/sql/sessiondatapb:session_data.proto",
133133
"//pkg/sql/sessiondatapb:session_migration.proto",
134134
"//pkg/sql/sessiondatapb:session_revival_token.proto",
135-
"//pkg/sql/sqlstats/insights:insights.proto",
135+
"//pkg/sql/sqlstats/insightspb:insights.proto",
136136
"//pkg/sql/types:types.proto",
137137
"//pkg/sql/vecindex/vecpb:vec.proto",
138138
"//pkg/storage/enginepb:file_registry.proto",

‎pkg/server/BUILD.bazel‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -278,7 +278,7 @@ go_library(
278278
"//pkg/sql/sqlliveness/slinstance",
279279
"//pkg/sql/sqlliveness/slprovider",
280280
"//pkg/sql/sqlstats",
281-
"//pkg/sql/sqlstats/insights",
281+
"//pkg/sql/sqlstats/insightspb",
282282
"//pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil",
283283
"//pkg/sql/sqlstats/sslocal",
284284
"//pkg/sql/stats",
@@ -567,6 +567,7 @@ go_test(
567567
"//pkg/sql/sessiondata",
568568
"//pkg/sql/sqlstats",
569569
"//pkg/sql/sqlstats/insights",
570+
"//pkg/sql/sqlstats/insightspb",
570571
"//pkg/sql/sqlstats/persistedsqlstats/sqlstatstestutil",
571572
"//pkg/sql/tablemetadatacache",
572573
"//pkg/sql/tablemetadatacache/util",

‎pkg/server/serverpb/BUILD.bazel‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
44
load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")
55
load("@rules_proto//proto:defs.bzl", "proto_library")
6+
load("//pkg/testutils:buildutil/buildutil.bzl", "disallowed_imports_test")
67

78
proto_library(
89
name = "serverpb_proto",
@@ -33,7 +34,7 @@ proto_library(
3334
"//pkg/server/status/statuspb:statuspb_proto",
3435
"//pkg/sql/appstatspb:appstatspb_proto",
3536
"//pkg/sql/contentionpb:contentionpb_proto",
36-
"//pkg/sql/sqlstats/insights:insights_proto",
37+
"//pkg/sql/sqlstats/insightspb:insightspb_proto",
3738
"//pkg/storage/enginepb:enginepb_proto",
3839
"//pkg/ts/catalog:catalog_proto",
3940
"//pkg/util:util_proto",
@@ -88,7 +89,7 @@ go_proto_library(
8889
"//pkg/server/status/statuspb",
8990
"//pkg/sql/appstatspb",
9091
"//pkg/sql/contentionpb",
91-
"//pkg/sql/sqlstats/insights",
92+
"//pkg/sql/sqlstats/insightspb",
9293
"//pkg/storage/enginepb",
9394
"//pkg/ts/catalog",
9495
"//pkg/util",
@@ -126,3 +127,8 @@ go_test(
126127
embed = [":serverpb"],
127128
deps = ["@com_github_stretchr_testify//assert"],
128129
)
130+
131+
disallowed_imports_test(
132+
"serverpb",
133+
disallowed_list = ["//pkg/sql/parser"],
134+
)

‎pkg/server/serverpb/status.proto‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import "server/serverpb/index_recommendations.proto";
2323
import "server/status/statuspb/status.proto";
2424
import "sql/appstatspb/app_stats.proto";
2525
import "sql/contentionpb/contention.proto";
26-
import "sql/sqlstats/insights/insights.proto";
26+
import "sql/sqlstats/insightspb/insights.proto";
2727
import "storage/enginepb/key_registry.proto";
2828
import "storage/enginepb/mvcc.proto";
2929
import "storage/enginepb/stats.proto";

‎pkg/server/status.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ import (
7474
"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
7575
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
7676
"github.com/cockroachdb/cockroach/pkg/sql/sqlinstance"
77-
"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/insights"
77+
"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/insightspb"
7878
"github.com/cockroachdb/cockroach/pkg/util/grpcutil"
7979
"github.com/cockroachdb/cockroach/pkg/util/hlc"
8080
"github.com/cockroachdb/cockroach/pkg/util/httputil"
@@ -395,18 +395,18 @@ func (b *baseStatusServer) localExecutionInsights(
395395
) (*serverpb.ListExecutionInsightsResponse, error) {
396396
var response serverpb.ListExecutionInsightsResponse
397397

398-
highContentionInsights := make(map[uuid.UUID]insights.Insight, 0)
398+
highContentionInsights := make(map[uuid.UUID]insightspb.Insight, 0)
399399
reader := b.sqlServer.pgServer.SQLServer.GetInsightsReader()
400-
reader.IterateInsights(ctx, func(ctx context.Context, insight *insights.Insight) {
400+
reader.IterateInsights(ctx, func(ctx context.Context, insight *insightspb.Insight) {
401401
if insight == nil {
402402
return
403403
}
404404

405405
// Copy statements slice - these insights objects can be read concurrently.
406406
insightsCopy := *insight
407-
insightsCopy.Statements = make([]*insights.Statement, len(insight.Statements))
407+
insightsCopy.Statements = make([]*insightspb.Statement, len(insight.Statements))
408408
copy(insightsCopy.Statements, insight.Statements)
409-
if insight.Transaction != nil && slices.Contains(insight.Transaction.Causes, insights.Cause_HighContention) {
409+
if insight.Transaction != nil && slices.Contains(insight.Transaction.Causes, insightspb.Cause_HighContention) {
410410
// Collect high contention insights seperately, they need additional validation / filtering.
411411
// If it is valid we will add it to the response later.
412412
highContentionInsights[insightsCopy.Transaction.ID] = insightsCopy
@@ -437,7 +437,7 @@ func (b *baseStatusServer) localExecutionInsights(
437437
// events that are related to the passed in (contention) insights. The insights that have
438438
// valid (resolved BlockingTxnFingerprintID) contention events are returned.
439439
func validContentionInsights(
440-
registry *contention.Registry, contentionInsights map[uuid.UUID]insights.Insight,
440+
registry *contention.Registry, contentionInsights map[uuid.UUID]insightspb.Insight,
441441
) (map[uuid.UUID]bool, error) {
442442
valid := make(map[uuid.UUID]bool, len(contentionInsights))
443443
err := registry.ForEachEvent(func(event *contentionpb.ExtendedContentionEvent) error {

‎pkg/server/status_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030
"github.com/cockroachdb/cockroach/pkg/sql/contention"
3131
"github.com/cockroachdb/cockroach/pkg/sql/contentionpb"
3232
"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/insights"
33+
"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/insightspb"
3334
tablemetadatacacheutil "github.com/cockroachdb/cockroach/pkg/sql/tablemetadatacache/util"
3435
"github.com/cockroachdb/cockroach/pkg/testutils"
3536
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -534,7 +535,7 @@ func TestLocalExecutionInsights(t *testing.T) {
534535
id1 := uuid.MakeV4()
535536
id2 := uuid.MakeV4()
536537
id3 := uuid.MakeV4()
537-
insights := map[uuid.UUID]insights.Insight{
538+
insights := map[uuid.UUID]insightspb.Insight{
538539
id1: {},
539540
id2: {},
540541
id3: {},

‎pkg/sql/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -463,6 +463,7 @@ go_library(
463463
"//pkg/sql/paramparse",
464464
"//pkg/sql/parser",
465465
"//pkg/sql/parser/statements",
466+
"//pkg/sql/parserutils",
466467
"//pkg/sql/pgrepl/lsn",
467468
"//pkg/sql/pgrepl/lsnutil",
468469
"//pkg/sql/pgrepl/pgrepltree",

0 commit comments

Comments
 (0)