Merge #104620 #105482 #105579 #105581 #105596

104620: concurrency: use lock modes to find conflicts during lock table scans r=nvanbenschoten a=arulajmani

Previous attempt over at: #104261

First commit from: #104537
Second commit from: #104600

This patch majorly refactors the lock table scan codepath, all in the
name of shared locks. At its core is a desire to use lock modes to
perform conflict resolution between an incoming request and locks held
on one particular key. In doing so, we rip out tryActiveWait.

At a high level (for a particular key), a request's journey looks like
the following:

- It first checks if the transaction already holds a lock at a equal or
higher lock strength (read: It's good enough for its use). If this is
the case, it can proceed without any bookkeeping.
- It then checks if any finalized transactions hold locks on the key.
Such locks do not conflict, but need to be resolved before the
transaction can evaluate. They're thus accumulated for later.
- The request then enqueues itself in the appropriate wait queue.
- It then determines if it needs to actively wait at this lock because
of a conflict. If that's the case, the lock table scan short circuits.
- Otherwise, the request lays a claim (if it can) before proceeding with
its scan.

Closes #102210

Release note: None

105482: sqltelemetry: add missing schema telemetry r=postamar a=rafiss

CREATE [ SCHEMA | INDEX | FUNCTION | TYPE ] and ALTER FUNCTION did not
have any telemetry, but they should.

Epic: None
Release note: None

105579: sql: disallow cross-database type references in CTAS r=chengxiong-ruan a=chengxiong-ruan

Fixes: #105393

Release note (bug fix): reviously, cross-database type references could sneaked in through `CREATE TABLE...AS` statements if the source table is from another database and any of its columns is of a user defined type. This introduced bug where the source table can be dropped and type could not be found for the CTAS table. This commit disallow such CTAS as a fix.

105581: optbuilder: reset annotations when building CREATE FUNCTION r=rafiss a=rafiss

In 22dabb0 we started overriding the annotations for each statement in the UDF body. We should reset them to the original values, so we don't accidentally leave the old reference.

Epic: None
Release note: None

105596: storage: make `storage.max_sync_duration` public and `TenantReadOnly` r=erikgrinaker a=erikgrinaker

Users have asked why this setting is not public, this patch makes it so.

Furthermore, these settings were `TenantWritable`. We do not want these to be writable by tenants, where they can potentially cause problems on SQL nodes, considering e.g. SQL disk spilling uses Pebble.

Epic: none
Release note: None

Co-authored-by: Arul Ajmani <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Chengxiong Ruan <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>

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

@@ -292,6 +292,8 @@ sql.ttl.default_delete_rate_limit	integer	0	default delete rate limit for all TT
 sql.ttl.default_select_batch_size	integer	500	default amount of rows to select in a single query during a TTL job	tenant-rw
 sql.ttl.job.enabled	boolean	true	whether the TTL job is enabled	tenant-rw
 sql.txn_fingerprint_id_cache.capacity	integer	100	the maximum number of txn fingerprint IDs stored	tenant-rw
+storage.max_sync_duration	duration	20s	maximum duration for disk operations; any operations that take longer than this setting trigger a warning log entry or process crash	tenant-ro
+storage.max_sync_duration.fatal.enabled	boolean	true	if true, fatal the process when a disk operation exceeds storage.max_sync_duration	tenant-ro
 timeseries.storage.enabled	boolean	true	if set, periodic timeseries data is stored within the cluster; disabling is not recommended unless you are storing the data elsewhere	tenant-rw
 timeseries.storage.resolution_10s.ttl	duration	240h0m0s	the maximum age of time series data stored at the 10 second resolution. Data older than this is subject to rollup and deletion.	tenant-rw
 timeseries.storage.resolution_30m.ttl	duration	2160h0m0s	the maximum age of time series data stored at the 30 minute resolution. Data older than this is subject to deletion.	tenant-rw

docs/generated/settings/settings.html

@@ -245,6 +245,8 @@
 <tr><td><div id="setting-sql-ttl-default-select-batch-size" class="anchored"><code>sql.ttl.default_select_batch_size</code></div></td><td>integer</td><td><code>500</code></td><td>default amount of rows to select in a single query during a TTL job</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-ttl-job-enabled" class="anchored"><code>sql.ttl.job.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>whether the TTL job is enabled</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-txn-fingerprint-id-cache-capacity" class="anchored"><code>sql.txn_fingerprint_id_cache.capacity</code></div></td><td>integer</td><td><code>100</code></td><td>the maximum number of txn fingerprint IDs stored</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-storage-max-sync-duration" class="anchored"><code>storage.max_sync_duration</code></div></td><td>duration</td><td><code>20s</code></td><td>maximum duration for disk operations; any operations that take longer than this setting trigger a warning log entry or process crash</td><td>Serverless/Dedicated/Self-Hosted (read-only)</td></tr>
+<tr><td><div id="setting-storage-max-sync-duration-fatal-enabled" class="anchored"><code>storage.max_sync_duration.fatal.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if true, fatal the process when a disk operation exceeds storage.max_sync_duration</td><td>Serverless/Dedicated/Self-Hosted (read-only)</td></tr>
 <tr><td><div id="setting-storage-value-blocks-enabled" class="anchored"><code>storage.value_blocks.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>set to true to enable writing of value blocks in sstables</td><td>Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-timeseries-storage-enabled" class="anchored"><code>timeseries.storage.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if set, periodic timeseries data is stored within the cluster; disabling is not recommended unless you are storing the data elsewhere</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-timeseries-storage-resolution-10s-ttl" class="anchored"><code>timeseries.storage.resolution_10s.ttl</code></div></td><td>duration</td><td><code>240h0m0s</code></td><td>the maximum age of time series data stored at the 10 second resolution. Data older than this is subject to rollup and deletion.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>

pkg/ccl/schemachangerccl/testdata/end_to_end/create_index/create_index.side_effects

@@ -13,6 +13,7 @@ CREATE INDEX id1
 begin transaction #1
 # begin StatementPhase
 checking for feature: CREATE INDEX
+increment telemetry for sql.schema.create_index
 write *eventpb.CreateIndex to event log:
   indexName: id1
   mutationId: 1

pkg/kv/kvserver/concurrency/testdata/lock_table/queue_length_exceeded

@@ -241,12 +241,6 @@ scan r=req10
 ----
 start-waiting: true
 
-# Note that the state here changed from waitForDistinguished to waitFor. This
-# is because we're cheating slightly in this test by calling scan on a request
-# that's already actively waiting at a lock, which is something that cannot
-# happen outside of unit tests. tryActiveWait doesn't expect this, and doesn't
-# handle this state transition -- we could teach it, but it would be just for
-# this contrived test scenario.
 guard-state r=req10
 ----
-new: state=waitFor txn=txn6 key="b" held=true guard-strength=Intent
+new: state=waitForDistinguished txn=txn6 key="b" held=true guard-strength=Intent

pkg/sql/alter_function.go

@@ -14,6 +14,7 @@ import (
 	"context"
 
 	"github.com/cockroachdb/cockroach/pkg/security/username"
+	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/funcdesc"
@@ -22,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
@@ -63,6 +65,8 @@ func (p *planner) AlterFunctionOptions(
 }
 
 func (n *alterFunctionOptionsNode) startExec(params runParams) error {
+	telemetry.Inc(sqltelemetry.SchemaChangeAlterCounter("function"))
+
 	fnDesc, err := params.p.mustGetMutableFunctionForAlter(params.ctx, &n.n.Function)
 	if err != nil {
 		return err
@@ -148,6 +152,7 @@ func (p *planner) AlterFunctionRename(
 }
 
 func (n *alterFunctionRenameNode) startExec(params runParams) error {
+	telemetry.Inc(sqltelemetry.SchemaChangeAlterCounter("function"))
 	// TODO(chengxiong): add validation that a function can not be altered if it's
 	// referenced by other objects. This is needed when want to allow function
 	// references.
@@ -221,6 +226,7 @@ func (p *planner) AlterFunctionSetOwner(
 }
 
 func (n *alterFunctionSetOwnerNode) startExec(params runParams) error {
+	telemetry.Inc(sqltelemetry.SchemaChangeAlterCounter("function"))
 	fnDesc, err := params.p.mustGetMutableFunctionForAlter(params.ctx, &n.n.Function)
 	if err != nil {
 		return err
@@ -281,6 +287,7 @@ func (p *planner) AlterFunctionSetSchema(
 }
 
 func (n *alterFunctionSetSchemaNode) startExec(params runParams) error {
+	telemetry.Inc(sqltelemetry.SchemaChangeAlterCounter("function"))
 	// TODO(chengxiong): add validation that a function can not be altered if it's
 	// referenced by other objects. This is needed when want to allow function
 	// references.

pkg/sql/create_function.go

@@ -15,6 +15,7 @@ import (
 	"fmt"
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege"
@@ -28,6 +29,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
@@ -68,6 +70,9 @@ func (n *createFunctionNode) startExec(params runParams) error {
 	if scDesc.SchemaKind() == catalog.SchemaTemporary {
 		return unimplemented.NewWithIssue(104687, "cannot create UDFs under a temporary schema")
 	}
+
+	telemetry.Inc(sqltelemetry.SchemaChangeCreateCounter("function"))
+
 	mutScDesc, err := params.p.descCollection.MutableByName(params.p.Txn()).Schema(params.ctx, n.dbDesc, n.scDesc.GetName())
 	if err != nil {
 		return err

pkg/sql/create_table.go

@@ -39,6 +39,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/paramparse"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
@@ -1223,6 +1224,27 @@ func newTableDescIfAs(
 		}
 	}
 
+	// Check if there is any reference to a user defined type that belongs to
+	// another database which is not allowed.
+	for _, def := range p.Defs {
+		if d, ok := def.(*tree.ColumnTableDef); ok {
+			// In CTAS, ColumnTableDef are generated from resultColumns which are
+			// resolved already. So we may cast it to *types.T directly without
+			// resolving it again.
+			typ := d.Type.(*types.T)
+			if typ.UserDefined() {
+				tn, typDesc, err := params.p.GetTypeDescriptor(params.ctx, typedesc.UserDefinedTypeOIDToID(typ.Oid()))
+				if err != nil {
+					return nil, err
+				}
+				if typDesc.GetParentID() != db.GetID() {
+					return nil, pgerror.Newf(
+						pgcode.FeatureNotSupported, "cross database type references are not supported: %s", tn.String())
+				}
+			}
+		}
+	}
+
 	desc, err = newTableDesc(
 		params,
 		p,

pkg/sql/create_type.go

@@ -17,6 +17,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
+	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
@@ -80,6 +81,7 @@ func (p *planner) CreateType(ctx context.Context, n *tree.CreateType) (planNode,
 }
 
 func (n *createTypeNode) startExec(params runParams) error {
+	telemetry.Inc(sqltelemetry.SchemaChangeCreateCounter("type"))
 	// Check if a type with the same name exists already.
 	g := params.p.Descriptors().ByName(params.p.Txn()).MaybeGet()
 	_, typ, err := descs.PrefixAndType(params.ctx, g, n.typeName)

pkg/sql/logictest/testdata/logic_test/create_as

@@ -382,3 +382,29 @@ query I
 SELECT * FROM tab_from_seq
 ----
 2
+
+# Regression test for #105393
+subtest regression_105393
+
+statement ok
+CREATE DATABASE db105393_1;
+CREATE DATABASE db105393_2;
+USE db105393_1;
+CREATE TYPE e105393 AS ENUM ('a');
+CREATE TABLE t105393 (a INT PRIMARY KEY, b e105393);
+USE db105393_2;
+
+statement error pq: cross database type references are not supported: db105393_1.public.e105393
+CREATE TABLE e105393 AS TABLE db105393_1.public.t105393;
+
+statement error pq: cross database type references are not supported: db105393_1.public.e105393
+CREATE TABLE e105393 AS SELECT * FROM db105393_1.public.t105393;
+
+statement error pq: cross database type references are not supported: db105393_1.public.e105393
+CREATE TABLE e105393 AS SELECT b FROM db105393_1.public.t105393;
+
+statement error pq: cross database type references are not supported: db105393_1.public.e105393
+CREATE TABLE e105393 (a PRIMARY KEY, b) AS TABLE db105393_1.public.t105393;
+
+statement error pq: cross database type references are not supported: db105393_1.public.e105393
+CREATE TABLE e105393 (b) AS SELECT b FROM db105393_1.public.t105393;

pkg/sql/opt/optbuilder/create_function.go

@@ -129,6 +129,10 @@ func (b *Builder) buildCreateFunction(cf *tree.CreateFunction, inScope *scope) (
 		// Reset the tracked dependencies for next statement.
 		b.schemaDeps = nil
 		b.schemaTypeDeps = intsets.Fast{}
+
+		// Reset the annotations to the original values
+		b.evalCtx.Annotations = oldEvalCtxAnn
+		b.semaCtx.Annotations = oldSemaCtxAnn
 	}
 
 	// bodyScope is the base scope for each statement in the body. We add the

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

@@ -28,6 +28,7 @@ func CreateFunction(b BuildCtx, n *tree.CreateFunction) {
 	if n.Replace {
 		panic(scerrors.NotImplementedError(n))
 	}
+	b.IncrementSchemaChangeCreateCounter("function")
 
 	dbElts, scElts := b.ResolvePrefix(n.FuncName.ObjectNamePrefix, privilege.CREATE)
 	_, _, sc := scpb.FindSchema(scElts)

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

@@ -47,6 +47,7 @@ import (
 
 // CreateIndex implements CREATE INDEX.
 func CreateIndex(b BuildCtx, n *tree.CreateIndex) {
+	b.IncrementSchemaChangeCreateCounter("index")
 	// Resolve the table name and start building the new index element.
 	relationElements := b.ResolveRelation(n.Table.ToUnresolvedObjectName(), ResolveParams{
 		IsExistenceOptional: false,

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

@@ -40,6 +40,7 @@ func CreateSchema(b BuildCtx, n *tree.CreateSchema) {
 		}
 	}
 
+	b.IncrementSchemaChangeCreateCounter("schema")
 	sqltelemetry.IncrementUserDefinedSchemaCounter(sqltelemetry.UserDefinedSchemaCreate)
 	dbElts := b.ResolveDatabase(n.Schema.CatalogName, ResolveParams{
 		IsExistenceOptional: false,

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

@@ -148,6 +148,10 @@ type TreeAnnotator interface {
 // Telemetry allows incrementing schema change telemetry counters.
 type Telemetry interface {
 
+	// IncrementSchemaChangeCreateCounter increments the selected CREATE telemetry
+	// counter.
+	IncrementSchemaChangeCreateCounter(counterType string)
+
 	// IncrementSchemaChangeAlterCounter increments the selected ALTER telemetry
 	// counter.
 	IncrementSchemaChangeAlterCounter(counterType string, extra ...string)

pkg/sql/schemachanger/scdeps/build_deps.go

@@ -385,6 +385,12 @@ func (d *buildDeps) IncrementSchemaChangeAlterCounter(counterType string, extra
 	telemetry.Inc(sqltelemetry.SchemaChangeAlterCounterWithExtra(counterType, maybeExtra))
 }
 
+// IncrementSchemaChangeCreateCounter implements the scbuild.Dependencies
+// interface.
+func (d *buildDeps) IncrementSchemaChangeCreateCounter(counterType string) {
+	telemetry.Inc(sqltelemetry.SchemaChangeCreateCounter(counterType))
+}
+
 // IncrementSchemaChangeDropCounter implements the scbuild.Dependencies
 // interface.
 func (d *buildDeps) IncrementSchemaChangeDropCounter(counterType string) {

pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go

@@ -110,6 +110,12 @@ func (s *TestState) IncrementSchemaChangeDropCounter(counterType string) {
 	s.LogSideEffectf("increment telemetry for sql.schema.drop_%s", counterType)
 }
 
+// IncrementSchemaChangeCreateCounter implements the scbuild.Dependencies
+// interface.
+func (s *TestState) IncrementSchemaChangeCreateCounter(counterType string) {
+	s.LogSideEffectf("increment telemetry for sql.schema.create_%s", counterType)
+}
+
 // IncrementSchemaChangeAddColumnTypeCounter  implements the scbuild.Dependencies
 // interface.
 func (s *TestState) IncrementSchemaChangeAddColumnTypeCounter(typeName string) {

pkg/sql/schemachanger/testdata/end_to_end/create_function/create_function.side_effects

@@ -31,6 +31,7 @@ $$;
 begin transaction #1
 # begin StatementPhase
 checking for feature: CREATE FUNCTION
+increment telemetry for sql.schema.create_function
 write *eventpb.CreateFunction to event log:
   functionName: defaultdb.public.f
   sql:

pkg/sql/schemachanger/testdata/end_to_end/create_function_in_txn/create_function_in_txn.side_effects

@@ -11,6 +11,7 @@ CREATE UNIQUE INDEX idx ON t(b);
 begin transaction #1
 # begin StatementPhase
 checking for feature: CREATE FUNCTION
+increment telemetry for sql.schema.create_function
 write *eventpb.CreateFunction to event log:
   functionName: defaultdb.public.t
   sql:
@@ -68,6 +69,7 @@ upsert descriptor #101
   -  version: "1"
   +  version: "2"
 checking for feature: CREATE INDEX
+increment telemetry for sql.schema.create_index
 write *eventpb.CreateIndex to event log:
   indexName: idx
   mutationId: 1

pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.side_effects

@@ -13,6 +13,7 @@ CREATE INDEX idx1 ON t (v) WHERE (v = 'a');
 begin transaction #1
 # begin StatementPhase
 checking for feature: CREATE INDEX
+increment telemetry for sql.schema.create_index
 increment telemetry for sql.schema.partial_index
 write *eventpb.CreateIndex to event log:
   indexName: idx1

pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements.side_effects

@@ -12,6 +12,7 @@ CREATE SCHEMA sc;
 begin transaction #1
 # begin StatementPhase
 checking for feature: CREATE INDEX
+increment telemetry for sql.schema.create_index
 increment telemetry for sql.schema.partial_index
 write *eventpb.CreateIndex to event log:
   indexName: idx
@@ -95,6 +96,7 @@ upsert descriptor #104
   -  version: "1"
   +  version: "2"
 checking for feature: CREATE SCHEMA
+increment telemetry for sql.schema.create_schema
 write *eventpb.CreateSchema to event log:
   owner: root
   schemaName: defaultdb.sc

pkg/sql/schemachanger/testdata/end_to_end/create_schema/create_schema.side_effects

@@ -8,6 +8,7 @@ CREATE SCHEMA sc;
 begin transaction #1
 # begin StatementPhase
 checking for feature: CREATE SCHEMA
+increment telemetry for sql.schema.create_schema
 write *eventpb.CreateSchema to event log:
   owner: root
   schemaName: defaultdb.sc

pkg/sql/schemachanger/testdata/end_to_end/create_schema_drop_schema_separate_statements/create_schema_drop_schema_separate_statements.side_effects

@@ -10,6 +10,7 @@ CREATE SCHEMA sc;
 begin transaction #1
 # begin StatementPhase
 checking for feature: CREATE SCHEMA
+increment telemetry for sql.schema.create_schema
 write *eventpb.CreateSchema to event log:
   owner: root
   schemaName: defaultdb.sc
@@ -74,6 +75,7 @@ upsert descriptor #104
   +  state: DROP
      version: "1"
 checking for feature: CREATE SCHEMA
+increment telemetry for sql.schema.create_schema
 write *eventpb.CreateSchema to event log:
   owner: root
   schemaName: defaultdb.sc

pkg/sql/schemachanger/testdata/end_to_end/drop_column_create_index_separate_statements/drop_column_create_index_separate_statements.side_effects

@@ -207,6 +207,7 @@ upsert descriptor #104
   -  version: "1"
   +  version: "2"
 checking for feature: CREATE INDEX
+increment telemetry for sql.schema.create_index
 write *eventpb.CreateIndex to event log:
   indexName: idx
   mutationId: 1