Merge #80724 #83375

80724: roachtest: add cluster stats utility r=kvoli a=kvoli This patch introduces cluster_stats, a utility wrapper for prometheus collected metrics. The motivation for this patch is to enable additional classes of roachtests to generate run statistics, on relevant cluster metrics. These run statistics are exported into a roachperf parseable stats.json. The utilities provided are: (1) collect and export metrics, per-node and in aggregate over a run; parseable by roachperf. (2) register and stream metrics for test processing at a fixed interval. (1) enables a arbitrary roachtests to declare and export relevant roachperf outputs. Where there is otherwise a binary outcome, we may now track performance over time. (2) reduces friction for testing assertions on cluster metrics, e.g. qps balance. This also makes dynamic workload testing easier, where workload application changes based on predicates of cluster statistics, e.g. switch key access distribution when qps becomes balanced. The allocator related roachtests do not currently export benchmark related statistics. This patch introduces stat collection on replicate/up/1to3, replicate/rebalance/3to5, rebalance/by-load/leases and rebalance/by-load/ranges. The benchmark metric for the nightly run is the time taken to reach balance, from starting the test. In addition, cluster statistics are exported for detailed view into: cpu usage, io bandwidth, qps and range counts. related: https://github.com/cockroachdb/roachperf/pull/94 Release note: None 83375: sql: allow foreign key to reference crdb_region column in REGIONAL BY ROW r=rytaft a=rytaft This commit loosens the restrictions on foreign key references to allow referencing `crdb_region` and other implicit partitioning columns used by `PARTITION ALL BY` tables. There is no technical reason why such references should be disallowed, so this commit simply removes the restriction. Fixes #74244 Release note (sql change): Foreign keys can now reference the `crdb_region` column in `REGIONAL BY ROW` tables even if `crdb_region` is not explicitly part of a `UNIQUE` constraint. This is possible since `crdb_region` is implicitly included in every index on `REGIONAL BY ROW` tables as the partitioning key. (This applies to whichever column is used as the partitioning column, in case a different name is used via `REGIONAL BY ROW AS`.) Co-authored-by: Austen McClernon <[email protected]> Co-authored-by: Rebecca Taft <[email protected]>
cockroachdb · Jul 5, 2022 · f6d0d84 · f6d0d84
3 parents c897707 + 4ce98a4 + a783362
commit f6d0d84
Show file tree

Hide file tree

Showing 24 changed files with 2,006 additions and 21 deletions.
diff --git a/build/bazelutil/check.sh b/build/bazelutil/check.sh
@@ -12,6 +12,7 @@ pkg/roachprod/vm/aws/config.go://go:generate gofmt -s -w embedded.go
 pkg/roachprod/vm/aws/config.go://go:generate goimports -w embedded.go
 pkg/roachprod/vm/aws/config.go://go:generate terraformgen -o terraform/main.tf
 pkg/roachprod/prometheus/prometheus.go://go:generate mockgen -package=prometheus -destination=mocks_generated_test.go . Cluster
+pkg/cmd/roachtest/clusterstats/collector.go://go:generate mockgen -package=clusterstats -destination mocks_generated_test.go github.com/cockroachdb/cockroach/pkg/roachprod/prometheus Client
 pkg/cmd/roachtest/tests/drt.go://go:generate mockgen -package tests -destination drt_generated_test.go github.com/cockroachdb/cockroach/pkg/roachprod/prometheus Client
 pkg/kv/kvclient/kvcoord/transport.go://go:generate mockgen -package=kvcoord -destination=mocks_generated_test.go . Transport
 pkg/kv/kvclient/rangecache/range_cache.go://go:generate mockgen -package=rangecachemock -destination=rangecachemock/mocks_generated.go . RangeDescriptorDB

diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel
@@ -102,6 +102,7 @@ ALL_TESTS = [
     "//pkg/cmd/reduce/reduce/reducesql:reducesql_test",
     "//pkg/cmd/reduce/reduce:reduce_test",
     "//pkg/cmd/release:release_test",
+    "//pkg/cmd/roachtest/clusterstats:clusterstats_test",
     "//pkg/cmd/roachtest/tests:tests_test",
     "//pkg/cmd/roachtest:roachtest_test",
     "//pkg/cmd/teamcity-trigger:teamcity-trigger_test",

diff --git a/pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit b/pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit
@@ -305,8 +305,8 @@ INSERT INTO fk_using_implicit_columns_against_t VALUES (1, 1, 4)
 statement error Key \(ref_t_pk\)=\(2\) is not present in table "t"
 INSERT INTO fk_using_implicit_columns_against_t VALUES (2, 2, 4)
 
-# Error if we reference the implicit partitioning in the foreign key.
-statement error there is no unique constraint matching given keys for referenced table t
+# We are allowed to reference the implicit partitioning in the foreign key.
+statement ok
 CREATE TABLE fk_including_implicit_columns_against_t (
   pk INT NOT NULL PRIMARY KEY,
   ref_t_a INT,

diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row
@@ -1799,6 +1799,56 @@ SELECT pk, a, b, crdb_region_col, crdb_region_col1 FROM regional_by_row_table_as
 2  NULL  NULL  us-east-1     ca-central-1
 3  NULL  NULL  us-east-1     ca-central-1
 
+# Permit foreign key constraint on full primary index of RBR table (See #74244).
+statement ok
+CREATE TABLE users (
+    id         UUID   PRIMARY KEY DEFAULT gen_random_uuid(),
+    username   STRING NOT NULL
+) LOCALITY REGIONAL BY ROW;
+CREATE TABLE user_settings (
+    id      UUID   PRIMARY KEY DEFAULT gen_random_uuid(),
+    user_id UUID   NOT NULL,
+    value   STRING NOT NULL,
+    FOREIGN KEY (user_id, crdb_region) REFERENCES users (id, crdb_region) ON DELETE CASCADE ON UPDATE CASCADE
+) LOCALITY REGIONAL BY ROW;
+
+statement ok
+INSERT INTO users (crdb_region, id, username) VALUES ('ap-southeast-2', '5ebfedee-0dcf-41e6-a315-5fa0b51b9882', 'artem')
+
+query error violates foreign key constraint
+INSERT INTO user_settings (crdb_region, id, user_id, value)
+  VALUES ('ca-central-1', DEFAULT, '5ebfedee-0dcf-41e6-a315-5fa0b51b9882', 'this should work')
+
+statement ok
+INSERT INTO user_settings (crdb_region, id, user_id, value)
+  VALUES ('ap-southeast-2', 'f1867e8b-3414-425c-b3f8-d7e237f848e6', '5ebfedee-0dcf-41e6-a315-5fa0b51b9882', 'this should work')
+
+query TTT
+SELECT crdb_region, * FROM users
+----
+ap-southeast-2  5ebfedee-0dcf-41e6-a315-5fa0b51b9882  artem
+
+query TTTT
+SELECT crdb_region, * FROM user_settings
+----
+ap-southeast-2  f1867e8b-3414-425c-b3f8-d7e237f848e6  5ebfedee-0dcf-41e6-a315-5fa0b51b9882  this should work
+
+query error violates foreign key constraint
+UPDATE user_settings SET crdb_region = 'ca-central-1' WHERE id = 'f1867e8b-3414-425c-b3f8-d7e237f848e6';
+
+statement ok
+UPDATE users SET crdb_region = 'ca-central-1' WHERE id = '5ebfedee-0dcf-41e6-a315-5fa0b51b9882';
+
+query TTT
+SELECT crdb_region, * FROM users
+----
+ca-central-1  5ebfedee-0dcf-41e6-a315-5fa0b51b9882  artem
+
+query TTTT
+SELECT crdb_region, * FROM user_settings
+----
+ca-central-1  f1867e8b-3414-425c-b3f8-d7e237f848e6  5ebfedee-0dcf-41e6-a315-5fa0b51b9882  this should work
+
 # Regression for https://github.com/cockroachdb/cockroach/issues/83076
 # It tests that we should be able to create a unique, expression index
 # on a REGIONAL BY ROW table.

diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_hash_sharded_index b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_hash_sharded_index
@@ -149,3 +149,38 @@ CREATE TABLE public.t_regional_pk_hashed_2 (
                             INDEX t_regional_pk_hashed_2_b_idx (b ASC) USING HASH WITH (bucket_count=16),
                             FAMILY fam_0_b_pk_crdb_region (b, pk, crdb_region)
 ) LOCALITY REGIONAL BY ROW
+
+# Permit foreign key constraint on full primary index of RBR table excluding
+# hash column (See #74244).
+statement ok
+CREATE TABLE users (
+    id         UUID   NOT NULL DEFAULT gen_random_uuid(),
+    username   STRING NOT NULL,
+    PRIMARY KEY (id) USING HASH
+) LOCALITY REGIONAL BY ROW
+
+statement error virtual column "crdb_internal_user_id_shard_16" cannot reference a foreign key
+CREATE TABLE user_settings (
+    id      UUID   PRIMARY KEY DEFAULT gen_random_uuid(),
+    user_id UUID   NOT NULL,
+    value   STRING NOT NULL,
+    INDEX (user_id) USING HASH,
+    FOREIGN KEY (crdb_internal_user_id_shard_16, user_id, crdb_region) REFERENCES users (crdb_internal_id_shard_16, id, crdb_region) ON DELETE CASCADE ON UPDATE CASCADE
+) LOCALITY REGIONAL BY ROW
+
+statement ok
+CREATE TABLE user_settings (
+    id      UUID   PRIMARY KEY DEFAULT gen_random_uuid(),
+    user_id UUID   NOT NULL,
+    value   STRING NOT NULL,
+    INDEX (user_id) USING HASH,
+    FOREIGN KEY (user_id, crdb_region) REFERENCES users (id, crdb_region) ON DELETE CASCADE ON UPDATE CASCADE
+) LOCALITY REGIONAL BY ROW
+
+statement error virtual column "crdb_internal_user_id_shard_16" cannot reference a foreign key
+ALTER TABLE user_settings ADD FOREIGN KEY (crdb_internal_user_id_shard_16, user_id, crdb_region)
+  REFERENCES users (crdb_internal_id_shard_16, id, crdb_region) ON DELETE CASCADE ON UPDATE CASCADE
+
+statement ok
+ALTER TABLE user_settings ADD FOREIGN KEY (user_id, crdb_region)
+  REFERENCES users (id, crdb_region) ON DELETE CASCADE ON UPDATE CASCADE
diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_query_behavior b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_query_behavior
@@ -2951,3 +2951,136 @@ vectorized: true
 statement ok
 SET index_recommendations_enabled = false;
 RESET vectorize
+
+subtest foreign_keys
+
+statement ok
+DROP TABLE users;
+CREATE TABLE users (
+    id         UUID   PRIMARY KEY DEFAULT gen_random_uuid(),
+    username   STRING NOT NULL
+) LOCALITY REGIONAL BY ROW;
+
+statement ok
+CREATE TABLE user_settings (
+    id      UUID   PRIMARY KEY DEFAULT gen_random_uuid(),
+    user_id UUID   NOT NULL,
+    value   STRING NOT NULL,
+    INDEX(user_id),
+    FOREIGN KEY (user_id, crdb_region) REFERENCES users (id, crdb_region)
+) LOCALITY REGIONAL BY ROW;
+
+statement ok
+CREATE TABLE user_settings_cascades (
+    id      UUID   PRIMARY KEY DEFAULT gen_random_uuid(),
+    user_id UUID   NOT NULL,
+    value   STRING NOT NULL,
+    INDEX(user_id),
+    FOREIGN KEY (user_id, crdb_region) REFERENCES users (id, crdb_region) ON DELETE CASCADE ON UPDATE CASCADE
+) LOCALITY REGIONAL BY ROW;
+
+# TODO(rytaft): The following query should be able to infer that the join
+# condition
+#   users.id = user_settings.user_id
+# is equivalent to
+#   users.id = user_settings.user_id AND users.crdb_region = user_settings.crdb_region
+# This would allow the optimizer to plan a lookup join between users and user_settings
+# and avoid visiting all regions. See #69617.
+query T
+EXPLAIN SELECT users.crdb_region AS user_region, user_settings.crdb_region AS user_settings_region, *
+  FROM users JOIN user_settings ON users.id = user_settings.user_id AND users.id = '5ebfedee-0dcf-41e6-a315-5fa0b51b9882';
+----
+distribution: local
+vectorized: true
+·
+• merge join
+│ equality: (user_id) = (id)
+│ right cols are key
+│
+├── • index join
+│   │ table: user_settings@user_settings_pkey
+│   │
+│   └── • scan
+│         missing stats
+│         table: user_settings@user_settings_user_id_idx
+│         spans: [/'ap-southeast-2'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882' - /'ap-southeast-2'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882'] [/'ca-central-1'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882' - /'ca-central-1'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882'] [/'us-east-1'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882' - /'us-east-1'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882']
+│
+└── • union all
+    │ limit: 1
+    │
+    ├── • scan
+    │     missing stats
+    │     table: users@users_pkey
+    │     spans: [/'ap-southeast-2'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882' - /'ap-southeast-2'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882']
+    │
+    └── • scan
+          missing stats
+          table: users@users_pkey
+          spans: [/'ca-central-1'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882' - /'ca-central-1'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882'] [/'us-east-1'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882' - /'us-east-1'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882']
+
+# Ensure that the FK checks and cascades are efficient.
+query T
+EXPLAIN INSERT INTO user_settings (user_id, value) VALUES ('5ebfedee-0dcf-41e6-a315-5fa0b51b9882', 'foo')
+----
+distribution: local
+vectorized: true
+·
+• insert fast path
+  into: user_settings(id, user_id, value, crdb_region)
+  auto commit
+  FK check: users@users_pkey
+  size: 5 columns, 1 row
+
+query T
+EXPLAIN INSERT INTO user_settings_cascades (user_id, value) VALUES ('5ebfedee-0dcf-41e6-a315-5fa0b51b9882', 'foo')
+----
+distribution: local
+vectorized: true
+·
+• insert fast path
+  into: user_settings_cascades(id, user_id, value, crdb_region)
+  auto commit
+  FK check: users@users_pkey
+  size: 5 columns, 1 row
+
+query T
+EXPLAIN DELETE FROM users WHERE id = '5ebfedee-0dcf-41e6-a315-5fa0b51b9882'
+----
+distribution: local
+vectorized: true
+·
+• root
+│
+├── • delete
+│   │ from: users
+│   │
+│   └── • buffer
+│       │ label: buffer 1
+│       │
+│       └── • union all
+│           │ limit: 1
+│           │
+│           ├── • scan
+│           │     missing stats
+│           │     table: users@users_pkey
+│           │     spans: [/'ap-southeast-2'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882' - /'ap-southeast-2'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882']
+│           │
+│           └── • scan
+│                 missing stats
+│                 table: users@users_pkey
+│                 spans: [/'ca-central-1'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882' - /'ca-central-1'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882'] [/'us-east-1'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882' - /'us-east-1'/'5ebfedee-0dcf-41e6-a315-5fa0b51b9882']
+│
+├── • fk-cascade
+│     fk: user_settings_cascades_user_id_crdb_region_fkey
+│     input: buffer 1
+│
+└── • constraint-check
+    │
+    └── • error if rows
+        │
+        └── • lookup join (semi)
+            │ table: user_settings@user_settings_user_id_idx
+            │ equality: (crdb_region, id) = (crdb_region,user_id)
+            │
+            └── • scan buffer
+                  label: buffer 1
diff --git a/pkg/cmd/roachtest/clusterstats/BUILD.bazel b/pkg/cmd/roachtest/clusterstats/BUILD.bazel
@@ -0,0 +1,57 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+load("@bazel_gomock//:gomock.bzl", "gomock")
+
+go_library(
+    name = "clusterstats",
+    srcs = [
+        "collector.go",
+        "doc.go",
+        "exporter.go",
+        "helpers.go",
+        "streamer.go",
+    ],
+    importpath = "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/clusterstats",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//pkg/cmd/roachtest/cluster",
+        "//pkg/cmd/roachtest/test",
+        "//pkg/roachprod/logger",
+        "//pkg/roachprod/prometheus",
+        "//pkg/util/search",
+        "//pkg/util/timeutil",
+        "@com_github_cockroachdb_errors//:errors",
+        "@com_github_prometheus_client_golang//api",
+        "@com_github_prometheus_client_golang//api/prometheus/v1:prometheus",
+        "@com_github_prometheus_common//model",
+    ],
+)
+
+go_test(
+    name = "clusterstats_test",
+    srcs = [
+        "exporter_test.go",
+        "streamer_test.go",
+        ":clusterstats_mock",  # keep
+    ],
+    embed = [":clusterstats"],
+    deps = [
+        "//pkg/roachprod/logger",
+        "//pkg/roachprod/prometheus",
+        "@com_github_golang_mock//gomock",
+        "@com_github_prometheus_client_golang//api/prometheus/v1:prometheus",
+        "@com_github_prometheus_common//model",
+        "@com_github_stretchr_testify//require",
+    ],
+)
+
+gomock(
+    name = "clusterstats_mock",
+    out = "mocks_generated_test.go",
+    interfaces = ["Client"],
+    library = "//pkg/roachprod/prometheus",
+    package = "clusterstats",
+    visibility = [
+        ":__pkg__",
+        "//pkg/gen:__pkg__",
+    ],
+)