diff --git a/pkg/kv/kvserver/tenantrate/BUILD.bazel b/pkg/kv/kvserver/tenantrate/BUILD.bazel index 3c7b9a477dd7..997aff4e0a74 100644 --- a/pkg/kv/kvserver/tenantrate/BUILD.bazel +++ b/pkg/kv/kvserver/tenantrate/BUILD.bazel @@ -14,13 +14,13 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/multitenant", + "//pkg/multitenant/tenantcostmodel", "//pkg/roachpb:with-mocks", "//pkg/settings", "//pkg/util/metric", "//pkg/util/metric/aggmetric", "//pkg/util/quotapool", "//pkg/util/syncutil", - "//pkg/util/tenantcostmodel", "//pkg/util/timeutil", "@com_github_cockroachdb_errors//:errors", ], @@ -33,13 +33,13 @@ go_test( data = glob(["testdata/**"]), deps = [ ":tenantrate", + "//pkg/multitenant/tenantcostmodel", "//pkg/roachpb:with-mocks", "//pkg/settings/cluster", "//pkg/testutils", "//pkg/testutils/metrictestutils", "//pkg/util/leaktest", "//pkg/util/metric", - "//pkg/util/tenantcostmodel", "//pkg/util/timeutil", "@com_github_cockroachdb_datadriven//:datadriven", "@com_github_cockroachdb_errors//:errors", diff --git a/pkg/kv/kvserver/tenantrate/factory.go b/pkg/kv/kvserver/tenantrate/factory.go index 427b7e689fd9..5170687f7d5b 100644 --- a/pkg/kv/kvserver/tenantrate/factory.go +++ b/pkg/kv/kvserver/tenantrate/factory.go @@ -13,11 +13,11 @@ package tenantrate import ( "context" + "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcostmodel" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/util/quotapool" "github.com/cockroachdb/cockroach/pkg/util/syncutil" - "github.com/cockroachdb/cockroach/pkg/util/tenantcostmodel" "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/errors" ) diff --git a/pkg/kv/kvserver/tenantrate/limiter.go b/pkg/kv/kvserver/tenantrate/limiter.go index 74a9cebc6d7a..8b98b339ff61 100644 --- a/pkg/kv/kvserver/tenantrate/limiter.go +++ b/pkg/kv/kvserver/tenantrate/limiter.go @@ -15,9 +15,9 @@ import ( "sync" "time" + "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcostmodel" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/util/quotapool" - "github.com/cockroachdb/cockroach/pkg/util/tenantcostmodel" "github.com/cockroachdb/cockroach/pkg/util/timeutil" ) diff --git a/pkg/kv/kvserver/tenantrate/limiter_test.go b/pkg/kv/kvserver/tenantrate/limiter_test.go index 4271eee0e39a..83511e9096c0 100644 --- a/pkg/kv/kvserver/tenantrate/limiter_test.go +++ b/pkg/kv/kvserver/tenantrate/limiter_test.go @@ -20,13 +20,13 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/tenantrate" + "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcostmodel" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/metrictestutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/metric" - "github.com/cockroachdb/cockroach/pkg/util/tenantcostmodel" "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/datadriven" "github.com/cockroachdb/errors" diff --git a/pkg/kv/kvserver/tenantrate/settings.go b/pkg/kv/kvserver/tenantrate/settings.go index ee3d155987c2..471b67e3bf6c 100644 --- a/pkg/kv/kvserver/tenantrate/settings.go +++ b/pkg/kv/kvserver/tenantrate/settings.go @@ -11,8 +11,8 @@ package tenantrate import ( + "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcostmodel" "github.com/cockroachdb/cockroach/pkg/settings" - "github.com/cockroachdb/cockroach/pkg/util/tenantcostmodel" ) // Config contains the configuration of the rate limiter. diff --git a/pkg/util/tenantcostmodel/BUILD.bazel b/pkg/multitenant/tenantcostmodel/BUILD.bazel similarity index 73% rename from pkg/util/tenantcostmodel/BUILD.bazel rename to pkg/multitenant/tenantcostmodel/BUILD.bazel index 820fa767643e..511f4312fb73 100644 --- a/pkg/util/tenantcostmodel/BUILD.bazel +++ b/pkg/multitenant/tenantcostmodel/BUILD.bazel @@ -6,7 +6,7 @@ go_library( "model.go", "settings.go", ], - importpath = "github.com/cockroachdb/cockroach/pkg/util/tenantcostmodel", + importpath = "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcostmodel", visibility = ["//visibility:public"], deps = ["//pkg/settings"], ) diff --git a/pkg/util/tenantcostmodel/model.go b/pkg/multitenant/tenantcostmodel/model.go similarity index 100% rename from pkg/util/tenantcostmodel/model.go rename to pkg/multitenant/tenantcostmodel/model.go diff --git a/pkg/util/tenantcostmodel/settings.go b/pkg/multitenant/tenantcostmodel/settings.go similarity index 100% rename from pkg/util/tenantcostmodel/settings.go rename to pkg/multitenant/tenantcostmodel/settings.go diff --git a/pkg/sql/create_stats.go b/pkg/sql/create_stats.go index 116452113199..3da365b71df2 100644 --- a/pkg/sql/create_stats.go +++ b/pkg/sql/create_stats.go @@ -335,12 +335,25 @@ func createStatsDefaultColumns( // addIndexColumnStatsIfNotExists appends column stats for the given column // ID if they have not already been added. Histogram stats are collected for // every indexed column. - addIndexColumnStatsIfNotExists := func(colID descpb.ColumnID, isInverted bool) { + addIndexColumnStatsIfNotExists := func(colID descpb.ColumnID, isInverted bool) error { + col, err := desc.FindColumnWithID(colID) + if err != nil { + return err + } + + // Do not collect stats for virtual computed columns. DistSQLPlanner + // cannot currently collect stats for these columns because it plans + // table readers on the table's primary index which does not include + // virtual computed columns. + if col.IsVirtual() { + return nil + } + colList := []descpb.ColumnID{colID} // Check for existing stats and remember the requested stats. if !trackStatsIfNotExists(colList) { - return + return nil } colStat := jobspb.CreateStatsDetails_ColStat{ @@ -359,12 +372,18 @@ func createStatsDefaultColumns( colStat.HasHistogram = true colStats = append(colStats, colStat) } + + return nil } // Add column stats for the primary key. - for i := 0; i < desc.GetPrimaryIndex().NumKeyColumns(); i++ { + primaryIdx := desc.GetPrimaryIndex() + for i := 0; i < primaryIdx.NumKeyColumns(); i++ { // Generate stats for each column in the primary key. - addIndexColumnStatsIfNotExists(desc.GetPrimaryIndex().GetKeyColumnID(i), false /* isInverted */) + err := addIndexColumnStatsIfNotExists(primaryIdx.GetKeyColumnID(i), false /* isInverted */) + if err != nil { + return nil, err + } // Only collect multi-column stats if enabled. if i == 0 || !multiColEnabled { @@ -393,7 +412,9 @@ func createStatsDefaultColumns( isInverted := idx.GetType() == descpb.IndexDescriptor_INVERTED && colID == idx.InvertedColumnID() // Generate stats for each indexed column. - addIndexColumnStatsIfNotExists(colID, isInverted) + if err := addIndexColumnStatsIfNotExists(colID, isInverted); err != nil { + return nil, err + } // Only collect multi-column stats if enabled. if j == 0 || !multiColEnabled { @@ -437,7 +458,9 @@ func createStatsDefaultColumns( return nil, err } isInverted := colinfo.ColumnTypeIsInvertedIndexable(col.GetType()) - addIndexColumnStatsIfNotExists(colID, isInverted) + if err := addIndexColumnStatsIfNotExists(colID, isInverted); err != nil { + return nil, err + } } } } @@ -446,6 +469,12 @@ func createStatsDefaultColumns( nonIdxCols := 0 for i := 0; i < len(desc.PublicColumns()) && nonIdxCols < maxNonIndexCols; i++ { col := desc.PublicColumns()[i] + + // Do not collect stats for virtual computed columns. + if col.IsVirtual() { + continue + } + colList := []descpb.ColumnID{col.GetID()} if !trackStatsIfNotExists(colList) { diff --git a/pkg/sql/logictest/testdata/logic_test/distsql_stats b/pkg/sql/logictest/testdata/logic_test/distsql_stats index 6a4c8157c4a2..3d6b707f29b0 100644 --- a/pkg/sql/logictest/testdata/logic_test/distsql_stats +++ b/pkg/sql/logictest/testdata/logic_test/distsql_stats @@ -826,8 +826,38 @@ upper_bound range_rows distinct_range_rows equal_rows statement ok SET experimental_enable_expression_indexes=true -# Test that inaccessible columns that represent expression indexes have -# histograms collected for them. +# Test that stats are not collected for virtual columns. +statement ok +CREATE TABLE virt ( + a INT, + v INT AS (a + 10) VIRTUAL, + INDEX (v) +) + +statement ok +INSERT INTO virt VALUES (1), (2), (3) + +statement ok +CREATE STATISTICS s FROM virt + +query TTIIB colnames,rowsort +SELECT + statistics_name, + column_names, + row_count, + null_count, + histogram_id IS NOT NULL AS has_histogram +FROM + [SHOW STATISTICS FOR TABLE virt] +ORDER BY + column_names::STRING, created +---- +statistics_name column_names row_count null_count has_histogram +s {a} 3 0 true +s {rowid} 3 0 true + +# Test that stats are not collect for inaccessible virtual columns that +# represent expression indexes. statement ok CREATE TABLE expression ( a INT, @@ -853,13 +883,11 @@ FROM ORDER BY column_names::STRING, created ---- -statistics_name column_names row_count null_count has_histogram -s {a} 3 0 true -s {b} 3 0 true -s {crdb_internal_idx_expr_1} 3 3 false -s {crdb_internal_idx_expr} 3 3 true -s {j} 3 0 false -s {rowid} 3 0 true +statistics_name column_names row_count null_count has_histogram +s {a} 3 0 true +s {b} 3 0 true +s {j} 3 0 false +s {rowid} 3 0 true # Test that non-index columns have histograms collected for them, with # up to 2 buckets.