From bad59441210b7698faae09fb69a9d70a3079d0e6 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Tue, 19 Jul 2022 14:30:38 -0400 Subject: [PATCH 1/6] importer: support {} format for arrays in CSV Release note (sql change): Arrays can now be imported in a CSV file using the {} format, similar to COPY FROM. Importing array expressions (e.g. ARRAY[1, 2, 3]) is still supported as well. --- pkg/sql/importer/import_stmt_test.go | 19 +++++++++++++++++++ pkg/sql/importer/read_import_csv.go | 17 ++++++++++++----- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/pkg/sql/importer/import_stmt_test.go b/pkg/sql/importer/import_stmt_test.go index 78a606760a33..cf8cd97f7f96 100644 --- a/pkg/sql/importer/import_stmt_test.go +++ b/pkg/sql/importer/import_stmt_test.go @@ -702,6 +702,25 @@ ORDER BY table_name }, }, }, + { + name: "array", + create: `a string, b string[]`, + typ: "CSV", + data: `cat,"{somevalue,anothervalue,anothervalue123}"`, + query: map[string][][]string{ + `SELECT * from t`: { + {"cat", "{somevalue,anothervalue,anothervalue123}"}, + }, + }, + }, + { + name: "array", + create: `a string, b string[]`, + typ: "CSV", + data: `dog,{some,thing}`, + err: "error parsing row 1: expected 2 fields, got 3", + rejected: "dog,{some,thing}\n", + }, // PG COPY { diff --git a/pkg/sql/importer/read_import_csv.go b/pkg/sql/importer/read_import_csv.go index f2abf3f97b57..342fe404cdbf 100644 --- a/pkg/sql/importer/read_import_csv.go +++ b/pkg/sql/importer/read_import_csv.go @@ -221,11 +221,18 @@ func (c *csvRowConsumer) FillDatums( var err error conv.Datums[datumIdx], err = rowenc.ParseDatumStringAs(conv.VisibleColTypes[i], field.Val, conv.EvalCtx) if err != nil { - col := conv.VisibleCols[i] - return newImportRowError( - errors.Wrapf(err, "parse %q as %s", col.GetName(), col.GetType().SQLString()), - strRecord(record, c.opts.Comma), - rowNum) + // Fallback to parsing as a string literal. This allows us to support + // both array expressions (like `ARRAY[1, 2, 3]`) and literals (like + // `{1, 2, 3}`). + var err2 error + conv.Datums[datumIdx], _, err2 = tree.ParseAndRequireString(conv.VisibleColTypes[i], field.Val, conv.EvalCtx) + if err2 != nil { + col := conv.VisibleCols[i] + return newImportRowError( + errors.Wrapf(errors.CombineErrors(err, err2), "parse %q as %s", col.GetName(), col.GetType().SQLString()), + strRecord(record, c.opts.Comma), + rowNum) + } } } datumIdx++ From 41c89ab622967d84c212d7062411f2bfe2a0f29e Mon Sep 17 00:00:00 2001 From: Chengxiong Ruan Date: Fri, 12 Aug 2022 00:03:01 -0400 Subject: [PATCH 2/6] sql: logic test to inform maximum builtin function oid change This commit adds a logic test to let engineers who added a new builtin function know that the new builtin function is constructed earlier than some existing builtin functions at init time. Release note: None --- pkg/sql/logictest/testdata/logic_test/pg_catalog | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog index 56b92f683e27..128aa545be89 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -4670,6 +4670,21 @@ query T SELECT proname FROM pg_catalog.pg_proc WHERE oid = 0 ---- +let $cur_max_builtin_oid +SELECT cur_max_builtin_oid FROM [SELECT max(oid) as cur_max_builtin_oid FROM pg_catalog.pg_proc] + +## If this test failed (proname is the same, but oid increased), it's likely that a +## new builtin function is implemented and it's somewhere in the middle of the +## existing functions at init time. Though the changes to builtin function OID is +## generally ok, it's still better if we could move the new implement to end of the +## list at init time (see all_builtins.go) +## TODO(chengxiong): consider to have a deterministic list of builtin function oids +## so that new implementations can just be added to it. +query TT +SELECT proname, oid FROM pg_catalog.pg_proc WHERE oid = $cur_max_builtin_oid +---- +to_regtype 2031 + ## Ensure that unnest works with oid wrapper arrays query O From 5b0982e48810c4d06d876f00b75b904cc5d075c2 Mon Sep 17 00:00:00 2001 From: irfan sharif Date: Mon, 14 Mar 2022 12:24:16 -0400 Subject: [PATCH 3/6] grunning: add library for precise on-CPU time measurement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Package grunning is a library that's able to retrieve on-CPU running time for individual goroutines. It relies on using a patched Go and provides a primitive for fine-grained CPU attribution and control through a single API: package grunning // Time returns the time spent by the current goroutine in the // running state. func Time() time.Duration The motivating RFC is over at #82356. Informs #82625. We build CRDB using use the patched Go runtime for all officially supported platforms when built using Bazel (#84867). Engineers commonly building CRDB also use happen to use two platforms we don't use a patched Go for: - FreeBSD (we don't have cross-compilers setup), and - M1/M2 Macs (we don't have a code-signing pipeline, yet). We use '(darwin && arm64) || freebsd || !bazel' as the build tag to exclude such platforms. See #84867 for more details. This package tests various properties we should expect over the running time value. It does not make assertions given the CI environments we run these under (CPU-starved, lot of OS thread pre-emption, dissimilar to healthy CRDB deployments). This is also why they're skipped under stress. Still, these tests are useful to understand the properties we expect running time to have: === RUN TestEquivalentGoroutines thread=03 expected≈10.00% got= 9.98% of on-cpu time thread=06 expected≈10.00% got=10.00% of on-cpu time thread=02 expected≈10.00% got=10.01% of on-cpu time thread=10 expected≈10.00% got=10.01% of on-cpu time thread=07 expected≈10.00% got= 9.99% of on-cpu time thread=04 expected≈10.00% got= 9.99% of on-cpu time thread=09 expected≈10.00% got=10.00% of on-cpu time thread=01 expected≈10.00% got= 9.99% of on-cpu time thread=08 expected≈10.00% got=10.02% of on-cpu time thread=05 expected≈10.00% got=10.02% of on-cpu time --- PASS: TestEquivalentGoroutines (0.56s) === RUN TestProportionalGoroutines thread=01 got 1.82% of on-cpu time: expected≈ 1.00x got=1.00x thread=02 got 3.64% of on-cpu time: expected≈ 2.00x got=2.00x thread=03 got 5.47% of on-cpu time: expected≈ 3.00x got=3.00x thread=04 got 7.28% of on-cpu time: expected≈ 4.00x got=4.00x thread=05 got 9.09% of on-cpu time: expected≈ 5.00x got=4.99x thread=06 got 10.91% of on-cpu time: expected≈ 6.00x got=5.99x thread=07 got 12.73% of on-cpu time: expected≈ 7.00x got=6.99x thread=08 got 14.54% of on-cpu time: expected≈ 8.00x got=7.99x thread=09 got 16.36% of on-cpu time: expected≈ 9.00x got=8.99x thread=10 got 18.16% of on-cpu time: expected≈10.00x got=9.97x --- PASS: TestProportionalGoroutines (1.72s) === RUN TestPingPongHog pinger/ponger expected≈1.00x got=0.96x --- PASS: TestPingPongHog (0.91s) Release note: None --- .github/CODEOWNERS | 2 + pkg/BUILD.bazel | 4 + pkg/util/grunning/BUILD.bazel | 284 +++++++++++++++++++++++++++++ pkg/util/grunning/disabled.go | 20 ++ pkg/util/grunning/disabled_test.go | 29 +++ pkg/util/grunning/enabled.go | 28 +++ pkg/util/grunning/enabled_test.go | 202 ++++++++++++++++++++ pkg/util/grunning/grunning.go | 43 +++++ 8 files changed, 612 insertions(+) create mode 100644 pkg/util/grunning/BUILD.bazel create mode 100644 pkg/util/grunning/disabled.go create mode 100644 pkg/util/grunning/disabled_test.go create mode 100644 pkg/util/grunning/enabled.go create mode 100644 pkg/util/grunning/enabled_test.go create mode 100644 pkg/util/grunning/grunning.go diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index b78ce5af6e7a..39cadd872b58 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -352,6 +352,8 @@ /pkg/util/addr/ @cockroachdb/cli-prs @cockroachdb/obs-inf-prs /pkg/util/metric/ @cockroachdb/obs-inf-prs /pkg/util/stop/ @cockroachdb/kv-prs +/pkg/util/grunning/ @cockroachdb/kv-prs +/pkg/util/admission/ @cockroachdb/kv-prs /pkg/util/tracing @cockroachdb/obs-inf-prs /pkg/workload/ @cockroachdb/sql-experience-noreview /pkg/obsservice/ @cockroachdb/obs-inf-prs diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index 178177aeff77..a7f90e2223e6 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -532,6 +532,7 @@ ALL_TESTS = [ "//pkg/util/fuzzystrmatch:fuzzystrmatch_test", "//pkg/util/goschedstats:goschedstats_test", "//pkg/util/grpcutil:grpcutil_test", + "//pkg/util/grunning:grunning_test", "//pkg/util/hlc:hlc_test", "//pkg/util/httputil:httputil_test", "//pkg/util/humanizeutil:humanizeutil_test", @@ -1866,6 +1867,8 @@ GO_TARGETS = [ "//pkg/util/growstack:growstack", "//pkg/util/grpcutil:grpcutil", "//pkg/util/grpcutil:grpcutil_test", + "//pkg/util/grunning:grunning", + "//pkg/util/grunning:grunning_test", "//pkg/util/hlc:hlc", "//pkg/util/hlc:hlc_test", "//pkg/util/httputil:httputil", @@ -2821,6 +2824,7 @@ GET_X_DATA_TARGETS = [ "//pkg/util/goschedstats:get_x_data", "//pkg/util/growstack:get_x_data", "//pkg/util/grpcutil:get_x_data", + "//pkg/util/grunning:get_x_data", "//pkg/util/hlc:get_x_data", "//pkg/util/httputil:get_x_data", "//pkg/util/humanizeutil:get_x_data", diff --git a/pkg/util/grunning/BUILD.bazel b/pkg/util/grunning/BUILD.bazel new file mode 100644 index 000000000000..9fe68e765990 --- /dev/null +++ b/pkg/util/grunning/BUILD.bazel @@ -0,0 +1,284 @@ +load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "grunning", + srcs = [ + "disabled.go", + "enabled.go", + "grunning.go", + ], + importpath = "github.com/cockroachdb/cockroach/pkg/util/grunning", + visibility = ["//visibility:public"], +) + +go_test( + name = "grunning_test", + srcs = [ + "disabled_test.go", + "enabled_test.go", + ], + deps = select({ + "@io_bazel_rules_go//go/platform:aix_ppc64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:android_386": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:android_amd64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:android_arm": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:android_arm64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:darwin_386": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:darwin_amd64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:darwin_arm": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:darwin_arm64": [ + ":grunning", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:dragonfly_amd64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:freebsd_386": [ + ":grunning", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:freebsd_amd64": [ + ":grunning", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:freebsd_arm": [ + ":grunning", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:freebsd_arm64": [ + ":grunning", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:illumos_amd64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:ios_amd64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:ios_arm64": [ + ":grunning", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:js_wasm": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:linux_386": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:linux_amd64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:linux_arm": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:linux_arm64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:linux_mips": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:linux_mips64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:linux_mips64le": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:linux_mipsle": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:linux_ppc64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:linux_ppc64le": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:linux_riscv64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:linux_s390x": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:netbsd_386": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:netbsd_amd64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:netbsd_arm": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:netbsd_arm64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:openbsd_386": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:openbsd_amd64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:openbsd_arm": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:openbsd_arm64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:plan9_386": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:plan9_amd64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:plan9_arm": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:solaris_amd64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:windows_386": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:windows_amd64": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "@io_bazel_rules_go//go/platform:windows_arm": [ + ":grunning", + "//pkg/testutils/skip", + "//pkg/util/syncutil", + "@com_github_stretchr_testify//require", + ], + "//conditions:default": [], + }), +) + +get_x_data(name = "get_x_data") diff --git a/pkg/util/grunning/disabled.go b/pkg/util/grunning/disabled.go new file mode 100644 index 000000000000..3cf943e508e5 --- /dev/null +++ b/pkg/util/grunning/disabled.go @@ -0,0 +1,20 @@ +// 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. + +// See grunning.Supported() for an explanation behind this build tag. +// +//go:build (darwin && arm64) || freebsd || !bazel +// +build darwin,arm64 freebsd !bazel + +package grunning + +func grunningnanos() int64 { return 0 } + +func supported() bool { return false } diff --git a/pkg/util/grunning/disabled_test.go b/pkg/util/grunning/disabled_test.go new file mode 100644 index 000000000000..effc8d6f4948 --- /dev/null +++ b/pkg/util/grunning/disabled_test.go @@ -0,0 +1,29 @@ +// 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. + +// See grunning.Supported() for an explanation behind this build tag. +// +//go:build (darwin && arm64) || freebsd || !bazel +// +build darwin,arm64 freebsd !bazel + +package grunning_test + +import ( + "testing" + + "github.com/cockroachdb/cockroach/pkg/util/grunning" + "github.com/stretchr/testify/require" +) + +func TestDisabled(t *testing.T) { + require.False(t, grunning.Supported()) + require.Zero(t, grunning.Time()) + require.Zero(t, grunning.Subtract(grunning.Time(), grunning.Time())) +} diff --git a/pkg/util/grunning/enabled.go b/pkg/util/grunning/enabled.go new file mode 100644 index 000000000000..48a4aad35eb2 --- /dev/null +++ b/pkg/util/grunning/enabled.go @@ -0,0 +1,28 @@ +// 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. + +// See grunning.Supported() for an explanation behind this build tag. +// +//go:build !((darwin && arm64) || freebsd || !bazel) +// +build !darwin !arm64 +// +build !freebsd +// +build bazel + +package grunning + +import _ "unsafe" // for go:linkname + +// grunningnanos returns the running time observed by the current goroutine by +// linking to a private symbol in the (patched) runtime package. +// +//go:linkname grunningnanos runtime.grunningnanos +func grunningnanos() int64 + +func supported() bool { return true } diff --git a/pkg/util/grunning/enabled_test.go b/pkg/util/grunning/enabled_test.go new file mode 100644 index 000000000000..068a86c71e95 --- /dev/null +++ b/pkg/util/grunning/enabled_test.go @@ -0,0 +1,202 @@ +// 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. + +// See grunning.Supported() for an explanation behind this build tag. +// +//go:build !((darwin && arm64) || freebsd || !bazel) +// +build !darwin !arm64 +// +build !freebsd +// +build bazel + +package grunning_test + +import ( + "runtime" + "sync" + "sync/atomic" + "testing" + + "github.com/cockroachdb/cockroach/pkg/testutils/skip" + "github.com/cockroachdb/cockroach/pkg/util/grunning" + "github.com/cockroachdb/cockroach/pkg/util/syncutil" + "github.com/stretchr/testify/require" +) + +// This file tests various properties we should expect over the running time +// value. It does not make assertions given the CI environments we run these +// under (CPU-starved, lot of OS thread pre-emption, dissimilar to healthy CRDB +// deployments). This is also why they're skipped under stress. Still, these +// tests are useful to understand the properties we expect running time to have. + +func TestEnabled(t *testing.T) { + require.True(t, grunning.Supported()) +} + +// TestEquivalentGoroutines is a variant of the "parallel test" in +// https://github.com/golang/go/issues/36821. It tests whether goroutines that +// (should) spend the same amount of time on-CPU have similar measured on-CPU +// time. +func TestEquivalentGoroutines(t *testing.T) { + skip.UnderStress(t, "not applicable") + + mu := struct { + syncutil.Mutex + nanos map[int]int64 + }{} + mu.nanos = make(map[int]int64) + + f := func(wg *sync.WaitGroup, id int) { + defer wg.Done() + + var sum int + for i := 0; i < 500000000; i++ { + sum -= i / 2 + sum *= i + sum /= i/3 + 1 + sum -= i / 4 + } + + nanos := grunning.Time().Nanoseconds() + mu.Lock() + mu.nanos[id] = nanos + mu.Unlock() + } + + const threads = 10 + var wg sync.WaitGroup + for i := 0; i < threads; i++ { + i := i // copy loop variable + wg.Add(1) + go f(&wg, i) + } + wg.Wait() + + mu.Lock() + defer mu.Unlock() + + total := int64(0) + for _, nanos := range mu.nanos { + total += nanos + } + + exp := 1.0 / threads + for i, nanos := range mu.nanos { + got := float64(nanos) / float64(total) + + t.Logf("thread=%02d expected≈%5.2f%% got=%5.2f%% of on-cpu time", + i+1, exp*100, got*100) + } +} + +// TestProportionalGoroutines is a variant of the "serial test" in +// https://github.com/golang/go/issues/36821. It tests whether goroutines that +// (should) spend a proportion of time on-CPU actually do so as measured by this +// package. +func TestProportionalGoroutines(t *testing.T) { + skip.UnderStress(t, "not applicable") + + f := func(wg *sync.WaitGroup, v uint64, trip uint64, result *int64) { + defer wg.Done() + + ret := v + for i := trip; i > 0; i-- { + ret += i + ret = ret ^ (i + 0xcafebabe) + } + + nanos := grunning.Time().Nanoseconds() + atomic.AddInt64(result, nanos) + } + + results := make([]int64, 10) + var wg sync.WaitGroup + + for iters := 0; iters < 10000; iters++ { + for i := uint64(0); i < 10; i++ { + i := i // copy loop variable + wg.Add(1) + go f(&wg, i+1, (i+1)*100000, &results[i]) + } + } + + wg.Wait() + + total := int64(0) + for _, result := range results { + total += result + } + + initial := float64(results[0]) / float64(total) + + for i, result := range results { + got := float64(result) / float64(total) + mult := got / initial + t.Logf("thread=%02d got %5.2f%% of on-cpu time: expected≈%5.2fx got=%4.2fx ", + i+1, got*100, float64(i+1), mult) + } +} + +// TestPingPongHog is adapted from a benchmark in the Go runtime, forcing the +// scheduler to continually schedule goroutines. +func TestPingPongHog(t *testing.T) { + skip.UnderStress(t, "not applicable") + + defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(1)) + + // Create a CPU hog. + stop, done := make(chan bool), make(chan bool) + go func() { + for { + select { + case <-stop: + done <- true + return + default: //lint:ignore SA5004 empty default case is intentional, we want it to spin + } + } + }() + + // Ping-pong 1000000 times. + const large = 1000000 + ping, pong := make(chan bool), make(chan bool) + var pingern, pongern int64 + go func() { + for j := 0; j < large; j++ { + pong <- <-ping + } + pingern = grunning.Time().Nanoseconds() + close(stop) + done <- true + }() + go func() { + for i := 0; i < large; i++ { + ping <- <-pong + } + pongern = grunning.Time().Nanoseconds() + done <- true + }() + ping <- true // start ping-pong + <-stop + <-ping // let last ponger exit + <-done // make sure goroutines exit + <-done + <-done + + mult := float64(pingern) / float64(pongern) + t.Logf("pinger/ponger expected≈1.00x got=%0.2fx", mult) +} + +// BenchmarkGRunningTime measures how costly it is to read the current +// goroutine's running time. +func BenchmarkGRunningTime(b *testing.B) { + for n := 0; n < b.N; n++ { + _ = grunning.Time() + } +} diff --git a/pkg/util/grunning/grunning.go b/pkg/util/grunning/grunning.go new file mode 100644 index 000000000000..c89306e0bb85 --- /dev/null +++ b/pkg/util/grunning/grunning.go @@ -0,0 +1,43 @@ +// 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 grunning is a library that's able to retrieve on-CPU running time for +// individual goroutines. It relies on using a patched Go and provides a +// primitive for fine-grained CPU attribution and control. See #82356 for more +// details. +package grunning + +import "time" + +// Time returns the time spent by the current goroutine in the running state. +func Time() time.Duration { + return time.Duration(grunningnanos()) +} + +// Subtract is a helper function to subtract a duration from another. It's +// commonly used to measure how much running time is spent doing some piece of +// work. +// +//gcassert:inline +func Subtract(end, start time.Duration) time.Duration { + return time.Duration(end.Nanoseconds() - start.Nanoseconds()) +} + +// Supported returns true iff per-goroutine running time is available in this +// build. We use a patched Go runtime for all platforms officially supported for +// CRDB when built using Bazel. Engineers commonly building CRDB also use happen +// to use two platforms we don't use a patched Go for: +// - FreeBSD (we don't have cross-compilers setup), and +// - M1/M2 Macs (we don't have a code-signing pipeline, yet). +// We use '(darwin && arm64) || freebsd || !bazel' as the build tag to exclude +// unsupported platforms. +func Supported() bool { + return supported() +} From 9ea2a19b25cdba98592c2594fa6d0514e77ebba3 Mon Sep 17 00:00:00 2001 From: Celia La Date: Wed, 10 Aug 2022 16:43:49 -0700 Subject: [PATCH 4/6] clusterversion,storage: remove 22.1 PebbleFormat version gates Release note: None --- pkg/clusterversion/cockroach_versions.go | 20 +--- pkg/clusterversion/key_string.go | 114 +++++++++++------------ pkg/storage/min_version_test.go | 6 +- pkg/storage/pebble.go | 6 +- 4 files changed, 64 insertions(+), 82 deletions(-) diff --git a/pkg/clusterversion/cockroach_versions.go b/pkg/clusterversion/cockroach_versions.go index c84167f661b2..0bf648ad66da 100644 --- a/pkg/clusterversion/cockroach_versions.go +++ b/pkg/clusterversion/cockroach_versions.go @@ -164,11 +164,6 @@ const ( // Start22_1 demarcates work towards CockroachDB v22.1. Start22_1 - // PebbleFormatBlockPropertyCollector switches to a backwards incompatible - // Pebble version that provides block property collectors that can be used - // for fine-grained time bound iteration. See - // https://github.com/cockroachdb/pebble/issues/1190 for details. - PebbleFormatBlockPropertyCollector // ProbeRequest is the version at which roachpb.ProbeRequest was introduced. // This version must be active before any ProbeRequest is issued on the // cluster. @@ -244,9 +239,6 @@ const ( EnableDeclarativeSchemaChanger // RowLevelTTL is the version where we allow row level TTL tables. RowLevelTTL - // PebbleFormatSplitUserKeysMarked performs a Pebble-level migration and - // upgrades the Pebble format major version to FormatSplitUserKeysMarked. - PebbleFormatSplitUserKeysMarked // EnableNewStoreRebalancer enables the new store rebalancer introduced in // 22.1. EnableNewStoreRebalancer @@ -369,6 +361,10 @@ const ( // previously referenced a < 21.2 version until that check/gate can be removed. const TODOPreV21_2 = V21_2 +// TODOPreV22_1 is an alias for V22_1 for use in any version gate/check that +// previously referenced a < 22.1 version until that check/gate can be removed. +const TODOPreV22_1 = V22_1 + // versionsSingleton lists all historical versions here in chronological order, // with comments describing what backwards-incompatible features were // introduced. @@ -398,10 +394,6 @@ var versionsSingleton = keyedVersions{ Key: Start22_1, Version: roachpb.Version{Major: 21, Minor: 2, Internal: 2}, }, - { - Key: PebbleFormatBlockPropertyCollector, - Version: roachpb.Version{Major: 21, Minor: 2, Internal: 24}, - }, { Key: ProbeRequest, Version: roachpb.Version{Major: 21, Minor: 2, Internal: 26}, @@ -489,10 +481,6 @@ var versionsSingleton = keyedVersions{ Key: RowLevelTTL, Version: roachpb.Version{Major: 21, Minor: 2, Internal: 88}, }, - { - Key: PebbleFormatSplitUserKeysMarked, - Version: roachpb.Version{Major: 21, Minor: 2, Internal: 90}, - }, { Key: EnableNewStoreRebalancer, Version: roachpb.Version{Major: 21, Minor: 2, Internal: 96}, diff --git a/pkg/clusterversion/key_string.go b/pkg/clusterversion/key_string.go index 3e120cc11c1e..dd7db51cf22f 100644 --- a/pkg/clusterversion/key_string.go +++ b/pkg/clusterversion/key_string.go @@ -10,67 +10,65 @@ func _() { var x [1]struct{} _ = x[V21_2-0] _ = x[Start22_1-1] - _ = x[PebbleFormatBlockPropertyCollector-2] - _ = x[ProbeRequest-3] - _ = x[PublicSchemasWithDescriptors-4] - _ = x[EnsureSpanConfigReconciliation-5] - _ = x[EnsureSpanConfigSubscription-6] - _ = x[EnableSpanConfigStore-7] - _ = x[SCRAMAuthentication-8] - _ = x[UnsafeLossOfQuorumRecoveryRangeLog-9] - _ = x[AlterSystemProtectedTimestampAddColumn-10] - _ = x[EnableProtectedTimestampsForTenant-11] - _ = x[DeleteCommentsWithDroppedIndexes-12] - _ = x[RemoveIncompatibleDatabasePrivileges-13] - _ = x[AddRaftAppliedIndexTermMigration-14] - _ = x[PostAddRaftAppliedIndexTermMigration-15] - _ = x[DontProposeWriteTimestampForLeaseTransfers-16] - _ = x[EnablePebbleFormatVersionBlockProperties-17] - _ = x[MVCCIndexBackfiller-18] - _ = x[EnableLeaseHolderRemoval-19] - _ = x[LooselyCoupledRaftLogTruncation-20] - _ = x[ChangefeedIdleness-21] - _ = x[EnableDeclarativeSchemaChanger-22] - _ = x[RowLevelTTL-23] - _ = x[PebbleFormatSplitUserKeysMarked-24] - _ = x[EnableNewStoreRebalancer-25] - _ = x[ClusterLocksVirtualTable-26] - _ = x[AutoStatsTableSettings-27] - _ = x[SuperRegions-28] - _ = x[EnableNewChangefeedOptions-29] - _ = x[SpanCountTable-30] - _ = x[PreSeedSpanCountTable-31] - _ = x[SeedSpanCountTable-32] - _ = x[V22_1-33] - _ = x[Start22_2-34] - _ = x[LocalTimestamps-35] - _ = x[PebbleFormatSplitUserKeysMarkedCompacted-36] - _ = x[EnsurePebbleFormatVersionRangeKeys-37] - _ = x[EnablePebbleFormatVersionRangeKeys-38] - _ = x[TrigramInvertedIndexes-39] - _ = x[RemoveGrantPrivilege-40] - _ = x[MVCCRangeTombstones-41] - _ = x[UpgradeSequenceToBeReferencedByID-42] - _ = x[SampledStmtDiagReqs-43] - _ = x[AddSSTableTombstones-44] - _ = x[SystemPrivilegesTable-45] - _ = x[EnablePredicateProjectionChangefeed-46] - _ = x[AlterSystemSQLInstancesAddLocality-47] - _ = x[SystemExternalConnectionsTable-48] - _ = x[AlterSystemStatementStatisticsAddIndexRecommendations-49] - _ = x[RoleIDSequence-50] - _ = x[AddSystemUserIDColumn-51] - _ = x[UsersHaveIDs-52] - _ = x[SetUserIDNotNull-53] - _ = x[SQLSchemaTelemetryScheduledJobs-54] - _ = x[SchemaChangeSupportsCreateFunction-55] - _ = x[DeleteRequestReturnKey-56] - _ = x[PebbleFormatPrePebblev1Marked-57] + _ = x[ProbeRequest-2] + _ = x[PublicSchemasWithDescriptors-3] + _ = x[EnsureSpanConfigReconciliation-4] + _ = x[EnsureSpanConfigSubscription-5] + _ = x[EnableSpanConfigStore-6] + _ = x[SCRAMAuthentication-7] + _ = x[UnsafeLossOfQuorumRecoveryRangeLog-8] + _ = x[AlterSystemProtectedTimestampAddColumn-9] + _ = x[EnableProtectedTimestampsForTenant-10] + _ = x[DeleteCommentsWithDroppedIndexes-11] + _ = x[RemoveIncompatibleDatabasePrivileges-12] + _ = x[AddRaftAppliedIndexTermMigration-13] + _ = x[PostAddRaftAppliedIndexTermMigration-14] + _ = x[DontProposeWriteTimestampForLeaseTransfers-15] + _ = x[EnablePebbleFormatVersionBlockProperties-16] + _ = x[MVCCIndexBackfiller-17] + _ = x[EnableLeaseHolderRemoval-18] + _ = x[LooselyCoupledRaftLogTruncation-19] + _ = x[ChangefeedIdleness-20] + _ = x[EnableDeclarativeSchemaChanger-21] + _ = x[RowLevelTTL-22] + _ = x[EnableNewStoreRebalancer-23] + _ = x[ClusterLocksVirtualTable-24] + _ = x[AutoStatsTableSettings-25] + _ = x[SuperRegions-26] + _ = x[EnableNewChangefeedOptions-27] + _ = x[SpanCountTable-28] + _ = x[PreSeedSpanCountTable-29] + _ = x[SeedSpanCountTable-30] + _ = x[V22_1-31] + _ = x[Start22_2-32] + _ = x[LocalTimestamps-33] + _ = x[PebbleFormatSplitUserKeysMarkedCompacted-34] + _ = x[EnsurePebbleFormatVersionRangeKeys-35] + _ = x[EnablePebbleFormatVersionRangeKeys-36] + _ = x[TrigramInvertedIndexes-37] + _ = x[RemoveGrantPrivilege-38] + _ = x[MVCCRangeTombstones-39] + _ = x[UpgradeSequenceToBeReferencedByID-40] + _ = x[SampledStmtDiagReqs-41] + _ = x[AddSSTableTombstones-42] + _ = x[SystemPrivilegesTable-43] + _ = x[EnablePredicateProjectionChangefeed-44] + _ = x[AlterSystemSQLInstancesAddLocality-45] + _ = x[SystemExternalConnectionsTable-46] + _ = x[AlterSystemStatementStatisticsAddIndexRecommendations-47] + _ = x[RoleIDSequence-48] + _ = x[AddSystemUserIDColumn-49] + _ = x[UsersHaveIDs-50] + _ = x[SetUserIDNotNull-51] + _ = x[SQLSchemaTelemetryScheduledJobs-52] + _ = x[SchemaChangeSupportsCreateFunction-53] + _ = x[DeleteRequestReturnKey-54] + _ = x[PebbleFormatPrePebblev1Marked-55] } -const _Key_name = "V21_2Start22_1PebbleFormatBlockPropertyCollectorProbeRequestPublicSchemasWithDescriptorsEnsureSpanConfigReconciliationEnsureSpanConfigSubscriptionEnableSpanConfigStoreSCRAMAuthenticationUnsafeLossOfQuorumRecoveryRangeLogAlterSystemProtectedTimestampAddColumnEnableProtectedTimestampsForTenantDeleteCommentsWithDroppedIndexesRemoveIncompatibleDatabasePrivilegesAddRaftAppliedIndexTermMigrationPostAddRaftAppliedIndexTermMigrationDontProposeWriteTimestampForLeaseTransfersEnablePebbleFormatVersionBlockPropertiesMVCCIndexBackfillerEnableLeaseHolderRemovalLooselyCoupledRaftLogTruncationChangefeedIdlenessEnableDeclarativeSchemaChangerRowLevelTTLPebbleFormatSplitUserKeysMarkedEnableNewStoreRebalancerClusterLocksVirtualTableAutoStatsTableSettingsSuperRegionsEnableNewChangefeedOptionsSpanCountTablePreSeedSpanCountTableSeedSpanCountTableV22_1Start22_2LocalTimestampsPebbleFormatSplitUserKeysMarkedCompactedEnsurePebbleFormatVersionRangeKeysEnablePebbleFormatVersionRangeKeysTrigramInvertedIndexesRemoveGrantPrivilegeMVCCRangeTombstonesUpgradeSequenceToBeReferencedByIDSampledStmtDiagReqsAddSSTableTombstonesSystemPrivilegesTableEnablePredicateProjectionChangefeedAlterSystemSQLInstancesAddLocalitySystemExternalConnectionsTableAlterSystemStatementStatisticsAddIndexRecommendationsRoleIDSequenceAddSystemUserIDColumnUsersHaveIDsSetUserIDNotNullSQLSchemaTelemetryScheduledJobsSchemaChangeSupportsCreateFunctionDeleteRequestReturnKeyPebbleFormatPrePebblev1Marked" +const _Key_name = "V21_2Start22_1ProbeRequestPublicSchemasWithDescriptorsEnsureSpanConfigReconciliationEnsureSpanConfigSubscriptionEnableSpanConfigStoreSCRAMAuthenticationUnsafeLossOfQuorumRecoveryRangeLogAlterSystemProtectedTimestampAddColumnEnableProtectedTimestampsForTenantDeleteCommentsWithDroppedIndexesRemoveIncompatibleDatabasePrivilegesAddRaftAppliedIndexTermMigrationPostAddRaftAppliedIndexTermMigrationDontProposeWriteTimestampForLeaseTransfersEnablePebbleFormatVersionBlockPropertiesMVCCIndexBackfillerEnableLeaseHolderRemovalLooselyCoupledRaftLogTruncationChangefeedIdlenessEnableDeclarativeSchemaChangerRowLevelTTLEnableNewStoreRebalancerClusterLocksVirtualTableAutoStatsTableSettingsSuperRegionsEnableNewChangefeedOptionsSpanCountTablePreSeedSpanCountTableSeedSpanCountTableV22_1Start22_2LocalTimestampsPebbleFormatSplitUserKeysMarkedCompactedEnsurePebbleFormatVersionRangeKeysEnablePebbleFormatVersionRangeKeysTrigramInvertedIndexesRemoveGrantPrivilegeMVCCRangeTombstonesUpgradeSequenceToBeReferencedByIDSampledStmtDiagReqsAddSSTableTombstonesSystemPrivilegesTableEnablePredicateProjectionChangefeedAlterSystemSQLInstancesAddLocalitySystemExternalConnectionsTableAlterSystemStatementStatisticsAddIndexRecommendationsRoleIDSequenceAddSystemUserIDColumnUsersHaveIDsSetUserIDNotNullSQLSchemaTelemetryScheduledJobsSchemaChangeSupportsCreateFunctionDeleteRequestReturnKeyPebbleFormatPrePebblev1Marked" -var _Key_index = [...]uint16{0, 5, 14, 48, 60, 88, 118, 146, 167, 186, 220, 258, 292, 324, 360, 392, 428, 470, 510, 529, 553, 584, 602, 632, 643, 674, 698, 722, 744, 756, 782, 796, 817, 835, 840, 849, 864, 904, 938, 972, 994, 1014, 1033, 1066, 1085, 1105, 1126, 1161, 1195, 1225, 1278, 1292, 1313, 1325, 1341, 1372, 1406, 1428, 1457} +var _Key_index = [...]uint16{0, 5, 14, 26, 54, 84, 112, 133, 152, 186, 224, 258, 290, 326, 358, 394, 436, 476, 495, 519, 550, 568, 598, 609, 633, 657, 679, 691, 717, 731, 752, 770, 775, 784, 799, 839, 873, 907, 929, 949, 968, 1001, 1020, 1040, 1061, 1096, 1130, 1160, 1213, 1227, 1248, 1260, 1276, 1307, 1341, 1363, 1392} func (i Key) String() string { if i < 0 || i >= Key(len(_Key_index)-1) { diff --git a/pkg/storage/min_version_test.go b/pkg/storage/min_version_test.go index a92e2fcb0924..1881d094bd85 100644 --- a/pkg/storage/min_version_test.go +++ b/pkg/storage/min_version_test.go @@ -113,11 +113,11 @@ func TestSetMinVersion(t *testing.T) { require.NoError(t, err) require.Equal(t, pebble.FormatSetWithDelete, p.db.FormatMajorVersion()) - // Advancing the store cluster version to PebbleFormatBlockPropertyCollector + // Advancing the store cluster version to TODOPreV22_1 // should also advance the store's format major version. - err = p.SetMinVersion(clusterversion.ByKey(clusterversion.PebbleFormatBlockPropertyCollector)) + err = p.SetMinVersion(clusterversion.ByKey(clusterversion.TODOPreV22_1)) require.NoError(t, err) - require.Equal(t, pebble.FormatBlockPropertyCollector, p.db.FormatMajorVersion()) + require.Equal(t, pebble.FormatSplitUserKeysMarked, p.db.FormatMajorVersion()) } diff --git a/pkg/storage/pebble.go b/pkg/storage/pebble.go index 96fb97f2cd7e..188c34fc52f6 100644 --- a/pkg/storage/pebble.go +++ b/pkg/storage/pebble.go @@ -1824,14 +1824,10 @@ func (p *Pebble) SetMinVersion(version roachpb.Version) error { if formatVers < pebble.FormatSplitUserKeysMarkedCompacted { formatVers = pebble.FormatSplitUserKeysMarkedCompacted } - case !version.Less(clusterversion.ByKey(clusterversion.PebbleFormatSplitUserKeysMarked)): + case !version.Less(clusterversion.ByKey(clusterversion.TODOPreV22_1)): if formatVers < pebble.FormatSplitUserKeysMarked { formatVers = pebble.FormatSplitUserKeysMarked } - case !version.Less(clusterversion.ByKey(clusterversion.PebbleFormatBlockPropertyCollector)): - if formatVers < pebble.FormatBlockPropertyCollector { - formatVers = pebble.FormatBlockPropertyCollector - } case !version.Less(clusterversion.ByKey(clusterversion.TODOPreV21_2)): if formatVers < pebble.FormatSetWithDelete { formatVers = pebble.FormatSetWithDelete From bf99114024db505e8745b18857a84adcd949657b Mon Sep 17 00:00:00 2001 From: Pavel Kalinnikov Date: Wed, 10 Aug 2022 18:36:15 +0100 Subject: [PATCH 5/6] kvserver: instrument RaftTransport workers with pprof labels The unused arguments in the method signature were used to identify goroutines in traces. This no longer works after Go 1.17 started passing arguments via registers. This commit adds pprof labels when starting these goroutines, to have a cleaner code, more readable traces, and to work around the new Go convention. Release note: None --- pkg/kv/kvserver/raft_transport.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/kv/kvserver/raft_transport.go b/pkg/kv/kvserver/raft_transport.go index 1d22b5124413..566a834715e0 100644 --- a/pkg/kv/kvserver/raft_transport.go +++ b/pkg/kv/kvserver/raft_transport.go @@ -13,6 +13,7 @@ package kvserver import ( "context" "net" + "runtime/pprof" "time" "unsafe" @@ -373,10 +374,7 @@ func (t *RaftTransport) Stop(storeID roachpb.StoreID) { // lost and a new instance of processQueue will be started by the next message // to be sent. func (t *RaftTransport) processQueue( - nodeID roachpb.NodeID, - ch chan *kvserverpb.RaftMessageRequest, - stream MultiRaft_RaftMessageBatchClient, - class rpc.ConnectionClass, + ch chan *kvserverpb.RaftMessageRequest, stream MultiRaft_RaftMessageBatchClient, ) error { errCh := make(chan error, 1) @@ -566,11 +564,14 @@ func (t *RaftTransport) startProcessNewQueue( return } - if err := t.processQueue(toNodeID, ch, stream, class); err != nil { + if err := t.processQueue(ch, stream); err != nil { log.Warningf(ctx, "while processing outgoing Raft queue to node %d: %s:", toNodeID, err) } } - err := t.stopper.RunAsyncTask(ctx, "storage.RaftTransport: sending messages", worker) + err := t.stopper.RunAsyncTask(ctx, "storage.RaftTransport: sending/receiving messages", + func(ctx context.Context) { + pprof.Do(ctx, pprof.Labels("remote_node_id", toNodeID.String()), worker) + }) if err != nil { t.queues[class].Delete(int64(toNodeID)) return false From de478e1fea199de49bd7305f3a54ccce50f5424a Mon Sep 17 00:00:00 2001 From: Marylia Gutierrez Date: Thu, 11 Aug 2022 15:12:22 -0400 Subject: [PATCH 6/6] sql: fix aggregation of statistics Previously, because we were using a join, we were double counting statistics when we had the same fingerprint in memory and persisted that had more than one index recommendation. This commit adds a `DISTINCT` so we only count them once. Fixes #85958 Release note: None --- pkg/sql/crdb_internal.go | 2 +- .../testdata/logic_test/create_statements | 4 +- .../sqlstats/persistedsqlstats/reader_test.go | 61 +++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 129a456edbea..9827b26634f1 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -5571,7 +5571,7 @@ SELECT plan_hash, app_name, max(metadata) as metadata, - crdb_internal.merge_statement_stats(array_agg(statistics)), + crdb_internal.merge_statement_stats(array_agg(DISTINCT statistics)), max(sampled_plan), aggregation_interval, array_remove(array_agg(index_rec), NULL) AS index_recommendations diff --git a/pkg/sql/logictest/testdata/logic_test/create_statements b/pkg/sql/logictest/testdata/logic_test/create_statements index 543ad9394a3e..ab9260c2eb0a 100644 --- a/pkg/sql/logictest/testdata/logic_test/create_statements +++ b/pkg/sql/logictest/testdata/logic_test/create_statements @@ -1512,7 +1512,7 @@ CREATE VIEW crdb_internal.statement_statistics ( plan_hash, app_name, max(metadata) AS metadata, - crdb_internal.merge_statement_stats(array_agg(statistics)), + crdb_internal.merge_statement_stats(array_agg(DISTINCT statistics)), max(sampled_plan), aggregation_interval, array_remove(array_agg(index_rec), NULL) AS index_recommendations @@ -1571,7 +1571,7 @@ CREATE VIEW crdb_internal.statement_statistics ( plan_hash, app_name, max(metadata) AS metadata, - crdb_internal.merge_statement_stats(array_agg(statistics)), + crdb_internal.merge_statement_stats(array_agg(DISTINCT statistics)), max(sampled_plan), aggregation_interval, array_remove(array_agg(index_rec), NULL) AS index_recommendations diff --git a/pkg/sql/sqlstats/persistedsqlstats/reader_test.go b/pkg/sql/sqlstats/persistedsqlstats/reader_test.go index 1580d542c5c6..75bd15527702 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/reader_test.go +++ b/pkg/sql/sqlstats/persistedsqlstats/reader_test.go @@ -24,6 +24,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sqlstats" "github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" + "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -143,6 +144,66 @@ func TestPersistedSQLStatsRead(t *testing.T) { }) } +// Testing same fingerprint having more than one index recommendation and +// checking the aggregation on the crdb_internal.statement_statistics table. +// Testing for issue #85958. +func TestSQLStatsWithMultipleIdxRec(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + skip.UnderStressRace(t, "expensive tests") + + fakeTime := stubTime{ + aggInterval: time.Hour, + } + fakeTime.setTime(timeutil.Now()) + + testCluster := serverutils.StartNewTestCluster(t, 3 /* numNodes */, base.TestClusterArgs{ + ServerArgs: base.TestServerArgs{ + Knobs: base.TestingKnobs{ + SQLStatsKnobs: &sqlstats.TestingKnobs{ + StubTimeNow: fakeTime.Now, + AOSTClause: "AS OF SYSTEM TIME '-1us'", + }, + }, + }, + }) + ctx := context.Background() + defer testCluster.Stopper().Stop(ctx) + + sqlConn := sqlutils.MakeSQLRunner(testCluster.ServerConn(0 /* idx */)) + + sqlConn.Exec(t, "CREATE TABLE t1 (k INT, i INT, f FLOAT, s STRING)") + sqlConn.Exec(t, "CREATE TABLE t2 (k INT, i INT, s STRING)") + + query := "SELECT t1.k FROM t1 JOIN t2 ON t1.k = t2.k WHERE t1.i > 3 AND t2.i > 3" + memorySelect := "SELECT statistics -> 'statistics' ->> 'cnt' as count, " + + "array_length(index_recommendations, 1) FROM " + + "crdb_internal.cluster_statement_statistics WHERE metadata ->> 'query' = " + + "'SELECT t1.k FROM t1 JOIN t2 ON t1.k = t2.k WHERE (t1.i > _) AND (t2.i > _)'" + combinedSelect := "SELECT statistics -> 'statistics' ->> 'cnt' as count, " + + "array_length(index_recommendations, 1) FROM " + + "crdb_internal.statement_statistics WHERE metadata ->> 'query' = " + + "'SELECT t1.k FROM t1 JOIN t2 ON t1.k = t2.k WHERE (t1.i > _) AND (t2.i > _)'" + + // Execute enough times to have index recommendations generated. + // This will generate two recommendations. + for i := 0; i < 6; i++ { + sqlConn.Exec(t, query) + } + var cnt int64 + var recs int64 + // It must have the same count 6 on both in-memory and combined tables. + // This test when there are more than one recommendation, so adding this + // example that has 2 recommendations; + sqlConn.QueryRow(t, memorySelect).Scan(&cnt, &recs) + require.Equal(t, int64(6), cnt) + require.Equal(t, int64(2), recs) + sqlConn.QueryRow(t, combinedSelect).Scan(&cnt, &recs) + require.Equal(t, int64(6), cnt) + require.Equal(t, int64(2), recs) +} + func verifyStoredStmtFingerprints( t *testing.T, expectedStmtFingerprints map[string]int64,