From b4385cac38e19bf097294c91d195bd339071b8cd Mon Sep 17 00:00:00 2001 From: Faizan Qazi Date: Wed, 26 Jul 2023 11:37:44 -0400 Subject: [PATCH 1/3] sql/schemachanger: DROP INDEX could drop unrelated foreign keys Previously, when DROP INDEX was resolving and iterating over foreign keys, it did not validate that these foreign keys were related to the index we were dropping. As a result, if any table referred back to the target table with the index, we would analyze its foreign keys. If cascade wasn't specified this could incorrectly end up blocking the DROP INDEX on unrelated foreign key references assuming they need our index. Or worse with cascade we could remove foreign key constraints in other tables. To address this, this patch filters the back references to only look at ones related to the target table, which causes the correct set to be analuzed / dropped. Fixes: #107576 Release note (bug fix): Dropping an index could end up failing or cleaning foreign keys (when CASCADE is specified) on other tables referencing the target table with this index. --- .../logictest/testdata/logic_test/drop_index | 37 +++++++++++++++++++ .../internal/scbuildstmt/drop_index.go | 13 ++++--- .../scbuild/internal/scbuildstmt/helpers.go | 16 ++++++++ 3 files changed, 61 insertions(+), 5 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/drop_index b/pkg/sql/logictest/testdata/logic_test/drop_index index d52210848d46..021880eccc05 100644 --- a/pkg/sql/logictest/testdata/logic_test/drop_index +++ b/pkg/sql/logictest/testdata/logic_test/drop_index @@ -464,3 +464,40 @@ t1_96731 t1_96731_pkey PRIMARY KEY PRIMARY KEY (i ASC) true statement ok DROP TABLE t1_96731, t2_96731; + + +# In #107576 we had a bug where we were not correctly resolving foreign key +# references by filtering down to the target table for DROP INDEX. As a side effect +# we could end up cleaning foreign key references in other unrelated tables. In +# the example below fk_drop_other has foreign key back references which are not +# relevant. +subtest drop_index_fk_check + +statement ok +CREATE TABLE fk_drop_target + ( k int primary key, + j int); +CREATE TABLE fk_drop_ref_src( + k int, + j int primary key); +CREATE UNIQUE INDEX target_j + ON fk_drop_target(j); + CREATE TABLE fk_ref_dst( + k int primary key, + j int, + m int, + CONSTRAINT "j_fk" FOREIGN KEY (j) REFERENCES + fk_drop_target(k), + CONSTRAINT m_fk FOREIGN KEY (m) REFERENCES + fk_drop_ref_src(j) + ); + +statement ok +DROP INDEX fk_drop_target@target_j CASCADE; + +query T rowsort +SELECT constraint_name from [SHOW CONSTRAINTS FROM fk_ref_dst]; +---- +fk_ref_dst_pkey +j_fk +m_fk diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go index 433e164ac66f..588678acae88 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go @@ -269,8 +269,8 @@ func maybeDropDependentFKConstraints( // dropDependentFKConstraint is a helper function that drops a dependent // FK constraint with ID `fkConstraintID`. - dropDependentFKConstraint := func(fkConstraintID catid.ConstraintID) { - b.BackReferences(tableID).Filter(hasConstraintIDAttrFilter(fkConstraintID)). + dropDependentFKConstraint := func(fkTableID catid.DescID, fkConstraintID catid.ConstraintID) { + b.BackReferences(tableID).Filter(hasTableID(fkTableID)).Filter(hasConstraintIDAttrFilter(fkConstraintID)). ForEach(func( current scpb.Status, target scpb.TargetStatus, e scpb.Element, ) { @@ -278,7 +278,10 @@ func maybeDropDependentFKConstraints( }) } - b.BackReferences(tableID).ForEach(func( + // Iterate over all FKs inbound to this table and decide whether any other + // unique constraints will satisfy them if we were to drop the current unique + // constraint. + b.BackReferences(tableID).Filter(containsDescIDFilter(tableID)).ForEach(func( current scpb.Status, target scpb.TargetStatus, e scpb.Element, ) { switch t := e.(type) { @@ -287,13 +290,13 @@ func maybeDropDependentFKConstraints( return } ensureCascadeBehavior(t.TableID) - dropDependentFKConstraint(t.ConstraintID) + dropDependentFKConstraint(t.TableID, t.ConstraintID) case *scpb.ForeignKeyConstraintUnvalidated: if !shouldDropFK(t.ReferencedColumnIDs) { return } ensureCascadeBehavior(t.TableID) - dropDependentFKConstraint(t.ConstraintID) + dropDependentFKConstraint(t.TableID, t.ConstraintID) } }) } diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go index f4ea0e53f37c..9a5232fa46e2 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go @@ -454,6 +454,22 @@ func notReachedTargetYetFilter(status scpb.Status, target scpb.TargetStatus, _ s return status != target.Status() } +func containsDescIDFilter( + descID catid.DescID, +) func(_ scpb.Status, _ scpb.TargetStatus, _ scpb.Element) bool { + return func(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) (included bool) { + return screl.ContainsDescID(e, descID) + } +} + +func hasTableID( + tableID catid.DescID, +) func(_ scpb.Status, _ scpb.TargetStatus, _ scpb.Element) bool { + return func(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) (included bool) { + return screl.GetDescID(e) == tableID + } +} + func hasIndexIDAttrFilter( indexID catid.IndexID, ) func(_ scpb.Status, _ scpb.TargetStatus, _ scpb.Element) bool { From a12a2ad8ebfdb3e8d1413f3db7091e33afce0bd0 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Wed, 26 Jul 2023 13:53:57 -0400 Subject: [PATCH 2/3] scheduledjobs: move MaybeRewriteCronExpr into package This was moved from the schematelemetrycontroller package. There are no code changes in this commit. Release note: None --- pkg/BUILD.bazel | 4 +- pkg/scheduledjobs/BUILD.bazel | 23 +++++++- pkg/scheduledjobs/rewrite_cron_expr.go | 53 +++++++++++++++++++ .../rewrite_cron_expr_test.go | 2 +- .../testdata/cron_rewrites | 0 pkg/sql/catalog/schematelemetry/BUILD.bazel | 1 + .../schematelemetry/schema_telemetry_test.go | 3 +- .../schematelemetrycontroller/BUILD.bazel | 17 +----- .../schematelemetrycontroller/controller.go | 41 +------------- pkg/upgrade/upgrades/BUILD.bazel | 1 + ...sure_sql_schema_telemetry_schedule_test.go | 5 +- 11 files changed, 87 insertions(+), 63 deletions(-) create mode 100644 pkg/scheduledjobs/rewrite_cron_expr.go rename pkg/{sql/catalog/schematelemetry/schematelemetrycontroller => scheduledjobs}/rewrite_cron_expr_test.go (98%) rename pkg/{sql/catalog/schematelemetry/schematelemetrycontroller => scheduledjobs}/testdata/cron_rewrites (100%) diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index 7e8f78f7aeb9..bd682f7a1445 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -279,6 +279,7 @@ ALL_TESTS = [ "//pkg/rpc/nodedialer:nodedialer_test", "//pkg/rpc:rpc_test", "//pkg/scheduledjobs/schedulebase:schedulebase_test", + "//pkg/scheduledjobs:scheduledjobs_test", "//pkg/security/certmgr:certmgr_test", "//pkg/security/password:password_test", "//pkg/security/sessionrevival:sessionrevival_test", @@ -352,7 +353,6 @@ ALL_TESTS = [ "//pkg/sql/catalog/resolver:resolver_test", "//pkg/sql/catalog/schemadesc:schemadesc_test", "//pkg/sql/catalog/schemaexpr:schemaexpr_test", - "//pkg/sql/catalog/schematelemetry/schematelemetrycontroller:schematelemetrycontroller_test", "//pkg/sql/catalog/schematelemetry:schematelemetry_test", "//pkg/sql/catalog/seqexpr:seqexpr_disallowed_imports_test", "//pkg/sql/catalog/seqexpr:seqexpr_test", @@ -1495,6 +1495,7 @@ GO_TARGETS = [ "//pkg/scheduledjobs/schedulebase:schedulebase", "//pkg/scheduledjobs/schedulebase:schedulebase_test", "//pkg/scheduledjobs:scheduledjobs", + "//pkg/scheduledjobs:scheduledjobs_test", "//pkg/security/certmgr:certmgr", "//pkg/security/certmgr:certmgr_test", "//pkg/security/certnames:certnames", @@ -1664,7 +1665,6 @@ GO_TARGETS = [ "//pkg/sql/catalog/schemaexpr:schemaexpr", "//pkg/sql/catalog/schemaexpr:schemaexpr_test", "//pkg/sql/catalog/schematelemetry/schematelemetrycontroller:schematelemetrycontroller", - "//pkg/sql/catalog/schematelemetry/schematelemetrycontroller:schematelemetrycontroller_test", "//pkg/sql/catalog/schematelemetry:schematelemetry", "//pkg/sql/catalog/schematelemetry:schematelemetry_test", "//pkg/sql/catalog/seqexpr:seqexpr", diff --git a/pkg/scheduledjobs/BUILD.bazel b/pkg/scheduledjobs/BUILD.bazel index 087be1bfc932..4031c0f45fd4 100644 --- a/pkg/scheduledjobs/BUILD.bazel +++ b/pkg/scheduledjobs/BUILD.bazel @@ -1,8 +1,11 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "scheduledjobs", - srcs = ["env.go"], + srcs = [ + "env.go", + "rewrite_cron_expr.go", + ], importpath = "github.com/cockroachdb/cockroach/pkg/scheduledjobs", visibility = ["//visibility:public"], deps = [ @@ -14,5 +17,21 @@ go_library( "//pkg/sql/isql", "//pkg/util/hlc", "//pkg/util/timeutil", + "//pkg/util/uuid", + ], +) + +go_test( + name = "scheduledjobs_test", + srcs = ["rewrite_cron_expr_test.go"], + args = ["-test.timeout=295s"], + data = glob(["testdata/**"]), + embed = [":scheduledjobs"], + deps = [ + "//pkg/testutils/datapathutils", + "//pkg/util/uuid", + "@com_github_cockroachdb_datadriven//:datadriven", + "@com_github_robfig_cron_v3//:cron", + "@com_github_stretchr_testify//require", ], ) diff --git a/pkg/scheduledjobs/rewrite_cron_expr.go b/pkg/scheduledjobs/rewrite_cron_expr.go new file mode 100644 index 000000000000..c45a4fcfabd1 --- /dev/null +++ b/pkg/scheduledjobs/rewrite_cron_expr.go @@ -0,0 +1,53 @@ +// Copyright 2023 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 scheduledjobs + +import ( + "fmt" + "hash/fnv" + "math/rand" + + "github.com/cockroachdb/cockroach/pkg/util/uuid" +) + +const ( + cronWeekly = "@weekly" + cronDaily = "@daily" + cronHourly = "@hourly" +) + +// MaybeRewriteCronExpr is used to rewrite the interval-oriented cron exprs +// into an equivalent frequency interval but with an offset derived from the +// uuid. For a given pair of inputs, the output of this function will always +// be the same. If the input cronExpr is not a special form as denoted by +// the keys of cronExprRewrites, it will be returned unmodified. This rewrite +// occurs in order to uniformly distribute the production of telemetry logs +// over the intended time interval to avoid bursts. +func MaybeRewriteCronExpr(id uuid.UUID, cronExpr string) string { + if f, ok := cronExprRewrites[cronExpr]; ok { + hash := fnv.New64a() // arbitrary hash function + _, _ = hash.Write(id.GetBytes()) + return f(rand.New(rand.NewSource(int64(hash.Sum64())))) + } + return cronExpr +} + +var cronExprRewrites = map[string]func(r *rand.Rand) string{ + cronWeekly: func(r *rand.Rand) string { + return fmt.Sprintf("%d %d * * %d", r.Intn(60), r.Intn(23), r.Intn(7)) + }, + cronDaily: func(r *rand.Rand) string { + return fmt.Sprintf("%d %d * * *", r.Intn(60), r.Intn(23)) + }, + cronHourly: func(r *rand.Rand) string { + return fmt.Sprintf("%d * * * *", r.Intn(60)) + }, +} diff --git a/pkg/sql/catalog/schematelemetry/schematelemetrycontroller/rewrite_cron_expr_test.go b/pkg/scheduledjobs/rewrite_cron_expr_test.go similarity index 98% rename from pkg/sql/catalog/schematelemetry/schematelemetrycontroller/rewrite_cron_expr_test.go rename to pkg/scheduledjobs/rewrite_cron_expr_test.go index 4f36dfb29189..987b7cfc4daa 100644 --- a/pkg/sql/catalog/schematelemetry/schematelemetrycontroller/rewrite_cron_expr_test.go +++ b/pkg/scheduledjobs/rewrite_cron_expr_test.go @@ -8,7 +8,7 @@ // by the Apache License, Version 2.0, included in the file // licenses/APL.txt. -package schematelemetrycontroller +package scheduledjobs import ( "bufio" diff --git a/pkg/sql/catalog/schematelemetry/schematelemetrycontroller/testdata/cron_rewrites b/pkg/scheduledjobs/testdata/cron_rewrites similarity index 100% rename from pkg/sql/catalog/schematelemetry/schematelemetrycontroller/testdata/cron_rewrites rename to pkg/scheduledjobs/testdata/cron_rewrites diff --git a/pkg/sql/catalog/schematelemetry/BUILD.bazel b/pkg/sql/catalog/schematelemetry/BUILD.bazel index 27c6ff239b98..09275272a093 100644 --- a/pkg/sql/catalog/schematelemetry/BUILD.bazel +++ b/pkg/sql/catalog/schematelemetry/BUILD.bazel @@ -49,6 +49,7 @@ go_test( "//pkg/base", "//pkg/jobs", "//pkg/jobs/jobstest", + "//pkg/scheduledjobs", "//pkg/security/securityassets", "//pkg/security/securitytest", "//pkg/server", diff --git a/pkg/sql/catalog/schematelemetry/schema_telemetry_test.go b/pkg/sql/catalog/schematelemetry/schema_telemetry_test.go index 893f52a08a49..46d1b271c904 100644 --- a/pkg/sql/catalog/schematelemetry/schema_telemetry_test.go +++ b/pkg/sql/catalog/schematelemetry/schema_telemetry_test.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/jobs" "github.com/cockroachdb/cockroach/pkg/jobs/jobstest" + "github.com/cockroachdb/cockroach/pkg/scheduledjobs" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schematelemetry/schematelemetrycontroller" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinconstants" @@ -82,7 +83,7 @@ func TestSchemaTelemetrySchedule(t *testing.T) { clusterID := s.ExecutorConfig().(sql.ExecutorConfig).NodeInfo. LogicalClusterID() - exp := schematelemetrycontroller.MaybeRewriteCronExpr(clusterID, "@weekly") + exp := scheduledjobs.MaybeRewriteCronExpr(clusterID, "@weekly") tdb.CheckQueryResultsRetry(t, qExists, [][]string{{exp, "1"}}) tdb.ExecSucceedsSoon(t, qSet) tdb.CheckQueryResultsRetry(t, qExists, [][]string{{"* * * * *", "1"}}) diff --git a/pkg/sql/catalog/schematelemetry/schematelemetrycontroller/BUILD.bazel b/pkg/sql/catalog/schematelemetry/schematelemetrycontroller/BUILD.bazel index abf48baed4aa..ae0b8588a98a 100644 --- a/pkg/sql/catalog/schematelemetry/schematelemetrycontroller/BUILD.bazel +++ b/pkg/sql/catalog/schematelemetry/schematelemetrycontroller/BUILD.bazel @@ -1,5 +1,5 @@ load("@rules_proto//proto:defs.bzl", "proto_library") -load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") +load("@io_bazel_rules_go//go:def.bzl", "go_library") load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library") proto_library( @@ -43,18 +43,3 @@ go_library( "@com_github_robfig_cron_v3//:cron", ], ) - -go_test( - name = "schematelemetrycontroller_test", - srcs = ["rewrite_cron_expr_test.go"], - args = ["-test.timeout=295s"], - data = glob(["testdata/**"]), - embed = [":schematelemetrycontroller"], - deps = [ - "//pkg/testutils/datapathutils", - "//pkg/util/uuid", - "@com_github_cockroachdb_datadriven//:datadriven", - "@com_github_robfig_cron_v3//:cron", - "@com_github_stretchr_testify//require", - ], -) diff --git a/pkg/sql/catalog/schematelemetry/schematelemetrycontroller/controller.go b/pkg/sql/catalog/schematelemetry/schematelemetrycontroller/controller.go index ab4a4317811c..ffbd13b7cfa8 100644 --- a/pkg/sql/catalog/schematelemetry/schematelemetrycontroller/controller.go +++ b/pkg/sql/catalog/schematelemetry/schematelemetrycontroller/controller.go @@ -12,9 +12,6 @@ package schematelemetrycontroller import ( "context" - "fmt" - "hash/fnv" - "math/rand" "time" "github.com/cockroachdb/cockroach/pkg/jobs" @@ -39,19 +36,13 @@ import ( // SchemaTelemetryScheduleName is the name of the schema telemetry schedule. const SchemaTelemetryScheduleName = "sql-schema-telemetry" -const ( - cronWeekly = "@weekly" - cronDaily = "@daily" - cronHourly = "@hourly" -) - // SchemaTelemetryRecurrence is the cron-tab string specifying the recurrence // for schema telemetry job. var SchemaTelemetryRecurrence = settings.RegisterValidatedStringSetting( settings.TenantReadOnly, "sql.schema.telemetry.recurrence", "cron-tab recurrence for SQL schema telemetry job", - cronWeekly, /* defaultValue */ + "@weekly", /* defaultValue */ func(_ *settings.Values, s string) error { if _, err := cron.ParseStandard(s); err != nil { return errors.Wrap(err, "invalid cron expression") @@ -166,7 +157,7 @@ func updateSchedule(ctx context.Context, db isql.DB, st *cluster.Settings, clust } } // Update schedule with new recurrence, if different. - cronExpr := MaybeRewriteCronExpr( + cronExpr := scheduledjobs.MaybeRewriteCronExpr( clusterID, SchemaTelemetryRecurrence.Get(&st.SV), ) if sj.ScheduleExpr() == cronExpr { @@ -185,34 +176,6 @@ func updateSchedule(ctx context.Context, db isql.DB, st *cluster.Settings, clust } } -// MaybeRewriteCronExpr is used to rewrite the interval-oriented cron exprs -// into an equivalent frequency interval but with an offset derived from the -// uuid. For a given pair of inputs, the output of this function will always -// be the same. If the input cronExpr is not a special form as denoted by -// the keys of cronExprRewrites, it will be returned unmodified. This rewrite -// occurs in order to uniformly distribute the production of telemetry logs -// over the intended time interval to avoid bursts. -func MaybeRewriteCronExpr(id uuid.UUID, cronExpr string) string { - if f, ok := cronExprRewrites[cronExpr]; ok { - hash := fnv.New64a() // arbitrary hash function - _, _ = hash.Write(id.GetBytes()) - return f(rand.New(rand.NewSource(int64(hash.Sum64())))) - } - return cronExpr -} - -var cronExprRewrites = map[string]func(r *rand.Rand) string{ - cronWeekly: func(r *rand.Rand) string { - return fmt.Sprintf("%d %d * * %d", r.Intn(60), r.Intn(23), r.Intn(7)) - }, - cronDaily: func(r *rand.Rand) string { - return fmt.Sprintf("%d %d * * *", r.Intn(60), r.Intn(23)) - }, - cronHourly: func(r *rand.Rand) string { - return fmt.Sprintf("%d * * * *", r.Intn(60)) - }, -} - // CreateSchemaTelemetryJob is part of the eval.SchemaTelemetryController // interface. func (c *Controller) CreateSchemaTelemetryJob( diff --git a/pkg/upgrade/upgrades/BUILD.bazel b/pkg/upgrade/upgrades/BUILD.bazel index 09ea7d2f7867..576db99dfbf1 100644 --- a/pkg/upgrade/upgrades/BUILD.bazel +++ b/pkg/upgrade/upgrades/BUILD.bazel @@ -148,6 +148,7 @@ go_test( "//pkg/keys", "//pkg/kv", "//pkg/roachpb", + "//pkg/scheduledjobs", "//pkg/security/securityassets", "//pkg/security/securitytest", "//pkg/security/username", diff --git a/pkg/upgrade/upgrades/ensure_sql_schema_telemetry_schedule_test.go b/pkg/upgrade/upgrades/ensure_sql_schema_telemetry_schedule_test.go index 6389981d8332..52a8652adf2e 100644 --- a/pkg/upgrade/upgrades/ensure_sql_schema_telemetry_schedule_test.go +++ b/pkg/upgrade/upgrades/ensure_sql_schema_telemetry_schedule_test.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/jobs" "github.com/cockroachdb/cockroach/pkg/jobs/jobstest" "github.com/cockroachdb/cockroach/pkg/kv" + "github.com/cockroachdb/cockroach/pkg/scheduledjobs" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schematelemetry/schematelemetrycontroller" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinconstants" @@ -90,14 +91,14 @@ func TestSchemaTelemetrySchedule(t *testing.T) { // Check that the schedule exists and that jobs can be created. tdb.Exec(t, qJob) - exp := schematelemetrycontroller.MaybeRewriteCronExpr(clusterID, "@weekly") + exp := scheduledjobs.MaybeRewriteCronExpr(clusterID, "@weekly") tdb.CheckQueryResultsRetry(t, qExists, [][]string{{exp, "1"}}) // Check that the schedule can have its recurrence altered. tdb.Exec(t, fmt.Sprintf(`SET CLUSTER SETTING %s = '* * * * *'`, schematelemetrycontroller.SchemaTelemetryRecurrence.Key())) tdb.CheckQueryResultsRetry(t, qExists, [][]string{{"* * * * *", "1"}}) - exp = schematelemetrycontroller.MaybeRewriteCronExpr(clusterID, "@daily") + exp = scheduledjobs.MaybeRewriteCronExpr(clusterID, "@daily") tdb.Exec(t, fmt.Sprintf(`SET CLUSTER SETTING %s = '@daily'`, schematelemetrycontroller.SchemaTelemetryRecurrence.Key())) tdb.CheckQueryResultsRetry(t, qExists, [][]string{{exp, "1"}}) From 67af0f5ff37451d5ef465c5e834a86c95358060d Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Wed, 26 Jul 2023 14:09:43 -0400 Subject: [PATCH 3/3] sql: use a random minute for the sql-stats-compaction job default recurrence Now, the sql-stats-compaction job that is created during cluster initialization will be scheduled on a random minute in the hour, rather than at the top of the hour. This will only affect clusters that are initialized after this change is released. Any existing clusters will continue to keep whatever recurrence they had before, which defaulted to @hourly. This change was made because we have observed that this job can cause CPU spikes on the serverless host clusters, since different tenants all had this job scheduled for the same time. Release note: None --- pkg/sql/conn_executor.go | 1 + pkg/sql/sqlstats/persistedsqlstats/BUILD.bazel | 1 + .../sqlstats/persistedsqlstats/compaction_scheduling.go | 7 +++++-- pkg/sql/sqlstats/persistedsqlstats/controller.go | 9 ++++++--- pkg/sql/sqlstats/persistedsqlstats/provider.go | 3 +++ .../sqlstats/persistedsqlstats/scheduled_job_monitor.go | 4 +++- 6 files changed, 19 insertions(+), 6 deletions(-) diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index ea0d782e16b7..789365444d8a 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -471,6 +471,7 @@ func NewServer(cfg *ExecutorConfig, pool *mon.BytesMonitor) *Server { DB: NewInternalDB( s, MemoryMetrics{}, sqlStatsInternalExecutorMonitor, ), + ClusterID: s.cfg.NodeInfo.LogicalClusterID, SQLIDContainer: cfg.NodeInfo.NodeID, JobRegistry: s.cfg.JobRegistry, Knobs: cfg.SQLStatsTestingKnobs, diff --git a/pkg/sql/sqlstats/persistedsqlstats/BUILD.bazel b/pkg/sql/sqlstats/persistedsqlstats/BUILD.bazel index bebb211be831..68ce48da1869 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/BUILD.bazel +++ b/pkg/sql/sqlstats/persistedsqlstats/BUILD.bazel @@ -49,6 +49,7 @@ go_library( "//pkg/util/stop", "//pkg/util/syncutil", "//pkg/util/timeutil", + "//pkg/util/uuid", "@com_github_cockroachdb_errors//:errors", "@com_github_gogo_protobuf//types", "@com_github_robfig_cron_v3//:cron", diff --git a/pkg/sql/sqlstats/persistedsqlstats/compaction_scheduling.go b/pkg/sql/sqlstats/persistedsqlstats/compaction_scheduling.go index b747e5baf568..802efee8ad41 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/compaction_scheduling.go +++ b/pkg/sql/sqlstats/persistedsqlstats/compaction_scheduling.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/isql" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/util/uuid" "github.com/cockroachdb/errors" pbtypes "github.com/gogo/protobuf/types" ) @@ -35,7 +36,7 @@ var ErrDuplicatedSchedules = errors.New("creating multiple sql stats compaction // scheduled job subsystem so the compaction job can be run periodically. This // is done during the cluster startup upgrade. func CreateSQLStatsCompactionScheduleIfNotYetExist( - ctx context.Context, txn isql.Txn, st *cluster.Settings, + ctx context.Context, txn isql.Txn, st *cluster.Settings, clusterID uuid.UUID, ) (*jobs.ScheduledJob, error) { scheduleExists, err := checkExistingCompactionSchedule(ctx, txn) if err != nil { @@ -48,7 +49,9 @@ func CreateSQLStatsCompactionScheduleIfNotYetExist( compactionSchedule := jobs.NewScheduledJob(scheduledjobs.ProdJobSchedulerEnv) - schedule := SQLStatsCleanupRecurrence.Get(&st.SV) + schedule := scheduledjobs.MaybeRewriteCronExpr( + clusterID, SQLStatsCleanupRecurrence.Get(&st.SV), + ) if err := compactionSchedule.SetSchedule(schedule); err != nil { return nil, err } diff --git a/pkg/sql/sqlstats/persistedsqlstats/controller.go b/pkg/sql/sqlstats/persistedsqlstats/controller.go index ec6b7e961eb0..6a8a2f18aaa6 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/controller.go +++ b/pkg/sql/sqlstats/persistedsqlstats/controller.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/isql" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqlstats/sslocal" + "github.com/cockroachdb/cockroach/pkg/util/uuid" ) // Controller implements the SQL Stats subsystem control plane. This exposes @@ -27,8 +28,9 @@ import ( // subsystem. type Controller struct { *sslocal.Controller - db isql.DB - st *cluster.Settings + db isql.DB + st *cluster.Settings + clusterID func() uuid.UUID } // NewController returns a new instance of sqlstats.Controller. @@ -39,6 +41,7 @@ func NewController( Controller: sslocal.NewController(sqlStats.SQLStats, status), db: db, st: sqlStats.cfg.Settings, + clusterID: sqlStats.cfg.ClusterID, } } @@ -46,7 +49,7 @@ func NewController( // interface. func (s *Controller) CreateSQLStatsCompactionSchedule(ctx context.Context) error { return s.db.Txn(ctx, func(ctx context.Context, txn isql.Txn) error { - _, err := CreateSQLStatsCompactionScheduleIfNotYetExist(ctx, txn, s.st) + _, err := CreateSQLStatsCompactionScheduleIfNotYetExist(ctx, txn, s.st, s.clusterID()) return err }) } diff --git a/pkg/sql/sqlstats/persistedsqlstats/provider.go b/pkg/sql/sqlstats/persistedsqlstats/provider.go index 574fdc275f12..80f8eb4fb3c2 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/provider.go +++ b/pkg/sql/sqlstats/persistedsqlstats/provider.go @@ -33,6 +33,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/stop" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/cockroach/pkg/util/timeutil" + "github.com/cockroachdb/cockroach/pkg/util/uuid" ) // Config is a configuration struct for the persisted SQL stats subsystem. @@ -40,6 +41,7 @@ type Config struct { Settings *cluster.Settings InternalExecutorMonitor *mon.BytesMonitor DB isql.DB + ClusterID func() uuid.UUID SQLIDContainer *base.SQLIDContainer JobRegistry *jobs.Registry @@ -100,6 +102,7 @@ func New(cfg *Config, memSQLStats *sslocal.SQLStats) *PersistedSQLStats { p.jobMonitor = jobMonitor{ st: cfg.Settings, + clusterID: cfg.ClusterID, db: cfg.DB, scanInterval: defaultScanInterval, jitterFn: p.jitterInterval, diff --git a/pkg/sql/sqlstats/persistedsqlstats/scheduled_job_monitor.go b/pkg/sql/sqlstats/persistedsqlstats/scheduled_job_monitor.go index cf5ba9f8e91e..692c518ecb6d 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/scheduled_job_monitor.go +++ b/pkg/sql/sqlstats/persistedsqlstats/scheduled_job_monitor.go @@ -25,6 +25,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/retry" "github.com/cockroachdb/cockroach/pkg/util/stop" "github.com/cockroachdb/cockroach/pkg/util/timeutil" + "github.com/cockroachdb/cockroach/pkg/util/uuid" "github.com/cockroachdb/errors" ) @@ -60,6 +61,7 @@ var longIntervalWarningThreshold = time.Hour * 24 // periodically every scanInterval (subject to jittering). type jobMonitor struct { st *cluster.Settings + clusterID func() uuid.UUID db isql.DB scanInterval time.Duration jitterFn func(time.Duration) time.Duration @@ -171,7 +173,7 @@ func (j *jobMonitor) updateSchedule(ctx context.Context, cronExpr string) { if !jobs.HasScheduledJobNotFoundError(err) && !errors.Is(err, errScheduleNotFound) { return err } - sj, err = CreateSQLStatsCompactionScheduleIfNotYetExist(ctx, txn, j.st) + sj, err = CreateSQLStatsCompactionScheduleIfNotYetExist(ctx, txn, j.st, j.clusterID()) if err != nil { return err }