Merge cockroachdb#107633 cockroachdb#107646

107633: sql/schemachanger: DROP INDEX could drop unrelated foreign keys r=fqazi a=fqazi

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: cockroachdb#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.

107646: sql: use a random minute for the sql-stats-compaction job default recurrence r=maryliag a=rafiss

### scheduledjobs: move MaybeRewriteCronExpr into package

This was moved from the schematelemetrycontroller package.

There are no code changes in this commit.

----

### 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.

---

see: https://cockroachlabs.slack.com/archives/C04U1BTF8/p1688829944578639
refs: cockroachdb#54537
Epic: None
Release note: None

Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>

Author: 3 people

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",

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",
     ],
 )

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))
+	},
+}

pkg/sql/catalog/schematelemetry/schematelemetrycontroller/rewrite_cron_expr_test.go renamed to 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"

pkg/sql/catalog/schematelemetry/schematelemetrycontroller/testdata/cron_rewrites renamed to pkg/scheduledjobs/testdata/cron_rewrites

@@ -49,6 +49,7 @@ go_test(
         "//pkg/base",
         "//pkg/jobs",
         "//pkg/jobs/jobstest",
+        "//pkg/scheduledjobs",
         "//pkg/security/securityassets",
         "//pkg/security/securitytest",
         "//pkg/server",

pkg/sql/catalog/schematelemetry/BUILD.bazel

@@ -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"}})

pkg/sql/catalog/schematelemetry/schema_telemetry_test.go

@@ -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",
-    ],
-)

pkg/sql/catalog/schematelemetry/schematelemetrycontroller/BUILD.bazel

@@ -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(

pkg/sql/catalog/schematelemetry/schematelemetrycontroller/controller.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,