Merge #85772 #86195 #86216 #86221 #86235 #86258

85772: ui/cluster-ui: fix polling in fingerprints pages r=xinhaoz a=xinhaoz

Fixes: #85236

Previously, SQL statement and transaction stats would be refreshed
every 5 minutes via sagas in CC and the cached api reducer in
db-console. This method relied on a refresh data call that resided
in `shouldComponentUpdate`, which was ignored by the respective
polling managers when the time interval was not complete. This
pattern was hacky as (unguarded calls in `shouldComponentUpdate`
are typically avoided in React. Polling in these pages were removed
with the introduciton of persisted stats, however we would like to
reintroduce polling when the selected time interval is `Latest xx..'
(i.e. not a custom interval). The removal of this polling introduced
a bug in the CC fingerprint pages, as the saga effects for when the
data was received would invalidate the data after the polling interval.
Now that the data was never refreshed, the page would get stuck on
the 'loading data' page.

This commit reintroduces polling via a `setTimeout` in the page
components, rather than through cached data reducer and sagasn for CC.
Data in the fingerprints overview pages is  now refreshed every
5 minutes for non-custom time ranges. The data invalidation in
CC is also cleaned up such that a call to invalidate data only
happens right before a request to fetch data (to signify new data
is being loaded).

Release note (bug fix): the statements and transaction fingerprint
will no longer get stuck on the loading page in CC after 5 minutes
idling on the page

Release note (ui change): the statements and transaction fingerprint
now refresh data every 5 minutes for non-custom time ranges

Release justification: bug fix

86195: sql/schemachanger: implement ALTER TABLE ... ADD PRIMARY KEY r=ajwerner a=ajwerner

This commit extends #86071 to support `ALTER TABLE ... ADD PRIMARY KEY` in
implicit transactions in the case that the existing primary key uses the
default `rowid` primary key. It adopts the ability of #86071 to drop the
`rowid` column along the way.

Release justification: minor improvement to new functionality needed to unblock DMS support.

Release note (sql change): When running `ALTER TABLE ... ADD PRIMARY KEY`
or `ALTER TABLE ... ADD CONSTRAINT ... PRIMARY KEY` in a single-statement,
implicit transaction, when no primary key had been added to the table, the
old `rowid` column which had been automatically created as the table's
`PRIMARY KEY` will now be dropped.

86216: insights: enable the fixed-threshold detector r=matthewtodd a=matthewtodd

This commit effectively enables the new "insights" subsystem, wherein we
observe in-flight statement executions and gather information about
statements that ran longer than a fixed threshold, with the intent of
supporting developers and operators tuning their workloads.

We are currently building a UI in DB console to surface these insights;
in the meantime, they are also available in the
`crdb_internal.cluster_execution_insights` table.

A local 5-minute run of the kv95 workload with the
`sql.insights.latency_threshold` cluster setting set to 0ms (disabled)
and 100ms shows an unappreciable impact on execution latencies:

| `latency_threshold` | ops(total) | ops/sec(cum) | avg(ms) | p50(ms) | p95(ms) | p99(ms) | pMax(ms) |
|---------------------|------------|--------------|---------|---------|---------|---------|----------|
| 0ms                 | 825873     | 2752.9       | 11.6    | 0.6     | 96.5    | 268.4   | 939.5    |
| 100ms               | 853175     | 2843.9       | 11.2    | 0.7     | 79.7    | 260.0   | 1208.0   |

Note that we have another, hopefully more useful, heuristic-based "slow"
detector in the works, but it will need more performance tuning before
we can enable it by default.

Release justification: Category 2: Bug fixes and low-risk updates to new
functionality

Release note (sql change): A new subsystem, "insights," has been enabled
by default, gathering slow statement executions in the
`crdb_internal.cluster_execution_insights` table along with possible
reasons for the slowness: full scans / missing indexes, contention, plan
changes, retries, etc. This system may be tuned by a handful of new
cluster settings and monitored with a handful of new metrics, all in the
`sql.insights` namespace.

86221: sql/gcjob/gcjobnotifier: fix race r=ajwerner a=ajwerner

The callback which I had assumed could only be called once was being
called multiple times concurrently.

Fixes #86121

Release justification: minor change to new functionality to fix flakey tests

Release note: None

86235: sqlsmith: do not attempt to drop system columns r=yuzefovich a=yuzefovich

Previously, sqlsmith could attempt to drop a system column which
would fail, now it will skip system columns for the drop command.

Fixes: #86076.

Release justification: test-only change.

Release note: None

86258: logictest: attempt to deflake inflight_trace_spans r=ajwerner a=ajwerner

Maybe there's some background tasks popping up.

Fixes #85812

Release justification: testing only change

Release note: None

Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Matthew Todd <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

docs/generated/settings/settings-for-tenants.txt

@@ -232,6 +232,7 @@ sql.distsql.max_running_flows	integer	-128	the value - when positive - used as i
 sql.distsql.temp_storage.workmem	byte size	64 MiB	maximum amount of memory in bytes a processor can use before falling back to temp storage
 sql.guardrails.max_row_size_err	byte size	512 MiB	maximum size of row (or column family if multiple column families are in use) that SQL can write to the database, above which an error is returned; use 0 to disable
 sql.guardrails.max_row_size_log	byte size	64 MiB	maximum size of row (or column family if multiple column families are in use) that SQL can write to the database, above which an event is logged to SQL_PERF (or SQL_INTERNAL_PERF if the mutating statement was internal); use 0 to disable
+sql.insights.latency_threshold	duration	100ms	amount of time after which an executing statement is considered slow. Use 0 to disable.
 sql.log.slow_query.experimental_full_table_scans.enabled	boolean	false	when set to true, statements that perform a full table/index scan will be logged to the slow query log even if they do not meet the latency threshold. Must have the slow query log enabled for this setting to have any effect.
 sql.log.slow_query.internal_queries.enabled	boolean	false	when set to true, internal queries which exceed the slow query log threshold are logged to a separate log. Must have the slow query log enabled for this setting to have any effect.
 sql.log.slow_query.latency_threshold	duration	0s	when set to non-zero, log statements whose service latency exceeds the threshold to a secondary logger on each node

docs/generated/settings/settings.html

@@ -163,6 +163,7 @@
 <tr><td><code>sql.guardrails.max_row_size_err</code></td><td>byte size</td><td><code>512 MiB</code></td><td>maximum size of row (or column family if multiple column families are in use) that SQL can write to the database, above which an error is returned; use 0 to disable</td></tr>
 <tr><td><code>sql.guardrails.max_row_size_log</code></td><td>byte size</td><td><code>64 MiB</code></td><td>maximum size of row (or column family if multiple column families are in use) that SQL can write to the database, above which an event is logged to SQL_PERF (or SQL_INTERNAL_PERF if the mutating statement was internal); use 0 to disable</td></tr>
 <tr><td><code>sql.hash_sharded_range_pre_split.max</code></td><td>integer</td><td><code>16</code></td><td>max pre-split ranges to have when adding hash sharded index to an existing table</td></tr>
+<tr><td><code>sql.insights.latency_threshold</code></td><td>duration</td><td><code>100ms</code></td><td>amount of time after which an executing statement is considered slow. Use 0 to disable.</td></tr>
 <tr><td><code>sql.log.slow_query.experimental_full_table_scans.enabled</code></td><td>boolean</td><td><code>false</code></td><td>when set to true, statements that perform a full table/index scan will be logged to the slow query log even if they do not meet the latency threshold. Must have the slow query log enabled for this setting to have any effect.</td></tr>
 <tr><td><code>sql.log.slow_query.internal_queries.enabled</code></td><td>boolean</td><td><code>false</code></td><td>when set to true, internal queries which exceed the slow query log threshold are logged to a separate log. Must have the slow query log enabled for this setting to have any effect.</td></tr>
 <tr><td><code>sql.log.slow_query.latency_threshold</code></td><td>duration</td><td><code>0s</code></td><td>when set to non-zero, log statements whose service latency exceeds the threshold to a secondary logger on each node</td></tr>

pkg/internal/sqlsmith/alter.go

@@ -254,7 +254,19 @@ func makeDropColumn(s *Smither) (tree.Statement, bool) {
 	if !ok {
 		return nil, false
 	}
-	col := tableRef.Columns[s.rnd.Intn(len(tableRef.Columns))]
+	// Pick a random column to drop while ignoring the system columns since
+	// those cannot be dropped.
+	var col *tree.ColumnTableDef
+	for {
+		col = tableRef.Columns[s.rnd.Intn(len(tableRef.Columns))]
+		var isSystemCol bool
+		for _, systemColDesc := range colinfo.AllSystemColumnDescs {
+			isSystemCol = isSystemCol || string(col.Name) == systemColDesc.Name
+		}
+		if !isSystemCol {
+			break
+		}
+	}
 
 	return &tree.AlterTable{
 		Table: tableRef.TableName.ToUnresolvedObjectName(),

pkg/sql/gcjob/gcjobnotifier/notifier.go

@@ -144,12 +144,13 @@ func (n *Notifier) Start(ctx context.Context) {
 func (n *Notifier) run(_ context.Context) {
 	defer n.markStopped()
 	systemConfigUpdateCh, _ := n.provider.RegisterSystemConfigChannel()
-	var haveNotified bool
+	var haveNotified syncutil.AtomicBool
 	versionSettingChanged := make(chan struct{}, 1)
 	versionBeingWaited := clusterversion.ByKey(clusterversion.UseDelRangeInGCJob)
 	n.settings.Version.SetOnChange(func(ctx context.Context, newVersion clusterversion.ClusterVersion) {
-		if !haveNotified && versionBeingWaited.LessEq(newVersion.Version) {
-			haveNotified = true
+		if !haveNotified.Get() &&
+			versionBeingWaited.LessEq(newVersion.Version) &&
+			!haveNotified.Swap(true) {
 			versionSettingChanged <- struct{}{}
 		}
 	})

pkg/sql/logictest/testdata/logic_test/alter_primary_key

@@ -616,7 +616,6 @@ SHOW CREATE t
 ----
 t  CREATE TABLE public.t (
    x INT8 NOT NULL,
-   rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
    CONSTRAINT t_pkey PRIMARY KEY (x ASC)
 )
 
@@ -653,7 +652,6 @@ SHOW CREATE t
 ----
 t  CREATE TABLE public.t (
    x INT8 NOT NULL,
-   rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
    CONSTRAINT my_pk PRIMARY KEY (x ASC)
 )
 

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -2071,7 +2071,6 @@ SHOW CREATE TABLE t
 ----
 t  CREATE TABLE public.t (
    id INT8 NOT NULL,
-   rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
    CONSTRAINT t_pkey PRIMARY KEY (id ASC)
 )
 

pkg/sql/logictest/testdata/logic_test/inflight_trace_spans

@@ -49,7 +49,7 @@ statement ok
 SET TRACING = off
 
 # Confirm that the trace and its associated spans are no longer tracked.
-query B
+query B retry
 SELECT count(*) = 0
   FROM current_trace_spans
 ----

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel

@@ -6,6 +6,7 @@ go_library(
     srcs = [
         "alter_table.go",
         "alter_table_add_column.go",
+        "alter_table_add_constraint.go",
         "alter_table_alter_primary_key.go",
         "alter_table_drop_column.go",
         "comment_on.go",

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go

@@ -24,16 +24,37 @@ import (
 	"github.com/cockroachdb/errors"
 )
 
+// supportedAlterTableCommand tracks metadata for ALTER TABLE commands that are
+// implemented by the new schema changer.
+type supportedAlterTableCommand struct {
+	// fn is a function to perform a schema change.
+	fn interface{}
+	// on indicates that this statement is on by default.
+	on bool
+	// extraChecks contains a function to determine whether the command is
+	// supported. These extraChecks are important to be able to avoid any
+	// extra round-trips to resolve the descriptor and its elements if we know
+	// that we cannot process the command.
+	extraChecks interface{}
+}
+
 // supportedAlterTableStatements tracks alter table operations fully supported by
 // declarative schema  changer. Operations marked as non-fully supported can
 // only be with the use_declarative_schema_changer session variable.
-var supportedAlterTableStatements = map[reflect.Type]supportedStatement{
-	reflect.TypeOf((*tree.AlterTableAddColumn)(nil)):       {alterTableAddColumn, true},
-	reflect.TypeOf((*tree.AlterTableDropColumn)(nil)):      {alterTableDropColumn, true},
-	reflect.TypeOf((*tree.AlterTableAlterPrimaryKey)(nil)): {alterTableAlterPrimaryKey, true},
+var supportedAlterTableStatements = map[reflect.Type]supportedAlterTableCommand{
+	reflect.TypeOf((*tree.AlterTableAddColumn)(nil)):       {fn: alterTableAddColumn, on: true},
+	reflect.TypeOf((*tree.AlterTableDropColumn)(nil)):      {fn: alterTableDropColumn, on: true},
+	reflect.TypeOf((*tree.AlterTableAlterPrimaryKey)(nil)): {fn: alterTableAlterPrimaryKey, on: true},
+	reflect.TypeOf((*tree.AlterTableAddConstraint)(nil)): {fn: alterTableAddConstraint, on: true, extraChecks: func(
+		t *tree.AlterTableAddConstraint,
+	) bool {
+		d, ok := t.ConstraintDef.(*tree.UniqueConstraintTableDef)
+		return ok && d.PrimaryKey && t.ValidationBehavior == tree.ValidationDefault
+	}},
 }
 
 func init() {
+	boolType := reflect.TypeOf((*bool)(nil)).Elem()
 	// Check function signatures inside the supportedAlterTableStatements map.
 	for statementType, statementEntry := range supportedAlterTableStatements {
 		callBackType := reflect.TypeOf(statementEntry.fn)
@@ -49,6 +70,20 @@ func init() {
 			panic(errors.AssertionFailedf("%v entry for alter table statement "+
 				"does not have a valid signature got %v", statementType, callBackType))
 		}
+		if statementEntry.extraChecks != nil {
+			extraChecks := reflect.TypeOf(statementEntry.extraChecks)
+			if extraChecks.Kind() != reflect.Func {
+				panic(errors.AssertionFailedf("%v extra checks for statement is "+
+					"not a function", statementType))
+			}
+			if extraChecks.NumIn() != 1 ||
+				extraChecks.In(0) != statementType ||
+				extraChecks.NumOut() != 1 ||
+				extraChecks.Out(0) != boolType {
+				panic(errors.AssertionFailedf("%v extra checks for alter table statement "+
+					"does not have a valid signature got %v", statementType, callBackType))
+			}
+		}
 	}
 }
 
@@ -71,7 +106,16 @@ func AlterTable(b BuildCtx, n *tree.AlterTable) {
 		// Check if partially supported operations are allowed next. If an
 		// operation is not fully supported will not allow it to be run in
 		// the declarative schema changer until its fully supported.
-		if !info.IsFullySupported(b.EvalCtx().SessionData().NewSchemaChangerMode) {
+		if !isFullySupported(
+			info.on, b.EvalCtx().SessionData().NewSchemaChangerMode,
+		) {
+			panic(scerrors.NotImplementedError(cmd))
+		}
+
+		// Run the extraChecks to see if we should avoid even resolving the
+		// descriptor.
+		if info.extraChecks != nil && !reflect.ValueOf(info.extraChecks).
+			Call([]reflect.Value{reflect.ValueOf(cmd)})[0].Bool() {
 			panic(scerrors.NotImplementedError(cmd))
 		}
 	}

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go

@@ -0,0 +1,40 @@
+// 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 scbuildstmt
+
+import (
+	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors"
+	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+)
+
+func alterTableAddConstraint(
+	b BuildCtx, tn *tree.TableName, tbl *scpb.Table, t *tree.AlterTableAddConstraint,
+) {
+	// Extra checks before reaching this point ensured that the command is
+	// an ALTER TABLE ... ADD PRIMARY KEY command.
+	d := t.ConstraintDef.(*tree.UniqueConstraintTableDef)
+
+	// Ensure that there is a default rowid column.
+	oldPrimaryIndex := mustRetrievePrimaryIndexElement(b, tbl.TableID)
+	if getPrimaryIndexDefaultRowIDColumn(
+		b, tbl.TableID, oldPrimaryIndex.IndexID,
+	) == nil {
+		panic(scerrors.NotImplementedError(t))
+	}
+	alterPrimaryKey(b, tn, tbl, alterPrimaryKeySpec{
+		n:             t,
+		Columns:       d.Columns,
+		Sharded:       d.Sharded,
+		Name:          d.Name,
+		StorageParams: d.StorageParams,
+	})
+}

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go

@@ -36,6 +36,25 @@ import (
 func alterTableAlterPrimaryKey(
 	b BuildCtx, tn *tree.TableName, tbl *scpb.Table, t *tree.AlterTableAlterPrimaryKey,
 ) {
+	alterPrimaryKey(b, tn, tbl, alterPrimaryKeySpec{
+		n:             t,
+		Columns:       t.Columns,
+		Sharded:       t.Sharded,
+		Name:          t.Name,
+		StorageParams: t.StorageParams,
+	})
+}
+
+type alterPrimaryKeySpec struct {
+	n             tree.NodeFormatter
+	Columns       tree.IndexElemList
+	Sharded       *tree.ShardedIndexDef
+	Name          tree.Name
+	StorageParams tree.StorageParams
+}
+
+func alterPrimaryKey(b BuildCtx, tn *tree.TableName, tbl *scpb.Table, t alterPrimaryKeySpec) {
+
 	// Panic on certain forbidden `ALTER PRIMARY KEY` cases (e.g. one of
 	// the new primary key column is a virtual column). See the comments
 	// for a full list of preconditions we check.
@@ -145,6 +164,9 @@ func alterTableAlterPrimaryKey(
 		// We're NOT dropping the rowid column => do one primary index swap.
 		in, tempIn := makeSwapPrimaryIndexSpec(b, out, inColumns)
 		in.idx.Sharding = sharding
+		if t.Name != "" {
+			in.name.Name = string(t.Name)
+		}
 		in.apply(b.Add)
 		tempIn.apply(b.AddTransient)
 		newPrimaryIndexElem = in.idx
@@ -161,13 +183,16 @@ func alterTableAlterPrimaryKey(
 		tempUnion.apply(b.AddTransient)
 		// Swap again to the final primary index: same PK but NOT storing rowid.
 		in, tempIn := makeSwapPrimaryIndexSpec(b, union, inColumns)
+		if t.Name != "" {
+			in.name.Name = string(t.Name)
+		}
 		in.idx.Sharding = sharding
 		in.apply(b.Add)
 		tempIn.apply(b.AddTransient)
 		newPrimaryIndexElem = in.idx
 		// Drop the rowid column
 		elts := b.QueryByID(rowidToDrop.TableID).Filter(hasColumnIDAttrFilter(rowidToDrop.ColumnID))
-		dropColumn(b, tn, tbl, t, rowidToDrop, elts, tree.DropRestrict)
+		dropColumn(b, tn, tbl, t.n, rowidToDrop, elts, tree.DropRestrict)
 	}
 
 	// Construct and add elements for a unique secondary index created on
@@ -185,7 +210,7 @@ func alterTableAlterPrimaryKey(
 //   5. no virtual columns (starting from v22.1);
 //   6. add more here
 // Panic if any precondition is found unmet.
-func checkForEarlyExit(b BuildCtx, tbl *scpb.Table, t *tree.AlterTableAlterPrimaryKey) {
+func checkForEarlyExit(b BuildCtx, tbl *scpb.Table, t alterPrimaryKeySpec) {
 	if err := paramparse.ValidateUniqueConstraintParams(
 		t.StorageParams,
 		paramparse.UniqueConstraintParamContext{
@@ -247,9 +272,7 @@ func checkForEarlyExit(b BuildCtx, tbl *scpb.Table, t *tree.AlterTableAlterPrima
 
 // isNewPrimaryKeySameAsOldPrimaryKey returns whether the requested new
 // primary key is the same as the old primary key.
-func isNewPrimaryKeySameAsOldPrimaryKey(
-	b BuildCtx, tbl *scpb.Table, t *tree.AlterTableAlterPrimaryKey,
-) bool {
+func isNewPrimaryKeySameAsOldPrimaryKey(b BuildCtx, tbl *scpb.Table, t alterPrimaryKeySpec) bool {
 	oldPrimaryIndexElem := mustRetrievePrimaryIndexElement(b, tbl.TableID)
 	oldPrimaryIndexKeyColumns := mustRetrieveKeyIndexColumns(b, tbl.TableID, oldPrimaryIndexElem.IndexID)
 
@@ -312,7 +335,7 @@ func fallBackIfConcurrentSchemaChange(b BuildCtx, tableID catid.DescID) {
 
 // fallBackIfRequestToBeSharded panics with an unimplemented error
 // if it is requested to be hash-sharded.
-func fallBackIfRequestToBeSharded(t *tree.AlterTableAlterPrimaryKey) {
+func fallBackIfRequestToBeSharded(t alterPrimaryKeySpec) {
 	if t.Sharded != nil {
 		panic(scerrors.NotImplementedErrorf(nil, "ALTER PRIMARY KEY USING HASH is not yet supported."))
 	}
@@ -344,9 +367,7 @@ func fallBackIfRegionalByRowTable(b BuildCtx, tableID catid.DescID) {
 // fallBackIfDescColInRowLevelTTLTables panics with an unimplemented
 // error if the table is a (row-level-ttl table && (it has a descending
 // key column || it has any inbound/outbound FK constraint)).
-func fallBackIfDescColInRowLevelTTLTables(
-	b BuildCtx, tableID catid.DescID, t *tree.AlterTableAlterPrimaryKey,
-) {
+func fallBackIfDescColInRowLevelTTLTables(b BuildCtx, tableID catid.DescID, t alterPrimaryKeySpec) {
 	_, _, rowLevelTTLElem := scpb.FindRowLevelTTL(b.QueryByID(tableID))
 	if rowLevelTTLElem == nil {
 		return
@@ -482,7 +503,7 @@ func mustRetrieveKeyIndexColumns(
 
 // makeShardedDescriptor construct a sharded descriptor for the new primary key.
 // Return nil if the new primary key is not hash-sharded.
-func makeShardedDescriptor(b BuildCtx, t *tree.AlterTableAlterPrimaryKey) *catpb.ShardedDescriptor {
+func makeShardedDescriptor(b BuildCtx, t alterPrimaryKeySpec) *catpb.ShardedDescriptor {
 	if t.Sharded == nil {
 		return nil
 	}
@@ -520,7 +541,7 @@ func maybeAddUniqueIndexForOldPrimaryKey(
 	b BuildCtx,
 	tn *tree.TableName,
 	tbl *scpb.Table,
-	t *tree.AlterTableAlterPrimaryKey,
+	t alterPrimaryKeySpec,
 	oldPrimaryIndex, newPrimaryIndex *scpb.PrimaryIndex,
 	rowidToDrop *scpb.Column,
 ) {
@@ -571,7 +592,7 @@ func addIndexColumnsForNewUniqueSecondaryIndexAndTempIndex(
 	b BuildCtx,
 	tn *tree.TableName,
 	tbl *scpb.Table,
-	t *tree.AlterTableAlterPrimaryKey,
+	t alterPrimaryKeySpec,
 	oldPrimaryIndexID catid.IndexID,
 	newUniqueSecondaryIndexID catid.IndexID,
 	temporaryIndexIDForNewUniqueSecondaryIndex catid.IndexID,