Merge #118289 #118330 #118479

118289: sql/tests: remove stale skipped error r=yuzefovich a=yuzefovich

We now have key-encoding of JSONs.

Epic: None

Release note: None

118330: ttl: use better defaults for some session variables r=yuzefovich a=yuzefovich

This commit fixes some problems that we've seen around internally-executed queries issued by the TTL jobs, present before 23.2. In particular, on 23.1 and prior releases we used Go defaults for the session data parameters for internally-executed queries, which can differ from the defaults set by CRDB. As a result, query plans could be suboptimal as we've seen in a couple of recent escalations. This bug has been fixed on 23.2 proper (we now use CRDB defaults for all session variables except for one), and on 23.1 and prior this commit applies a targeted fix only to the TTL queries. In particular, the following overrides are now set:
- `reorder_joins_limit` to the default value of 8
- `optimizer_use_histograms` and `optimizer_use_multi_col_stats` are both set to `true`.

On 23.2 and later only `optimizer_use_histograms` needs to be updated since it's the only exception mentioned above. However, I chose to keep all 3 in the same commit so that it can be backported more easily to 23.1, and the following commit will revert the other 2 on 23.2 and later.

Touches: #102954.
Fixes: #118129.

Release note (bug fix): Internal queries issued by the TTL jobs should now use optimal plans. The bug has been present since at least 22.2 version.

118479: sql: enable read committed cluster setting by default r=rafiss a=rafiss

Epic CRDB-34172

Release note (sql change): The sql.txn.read_committed_isolation.enabled cluster setting is now true by default. This means that any syntax and settings that configure the READ COMMITTED isolation level will now cause the transaction to use that isolation level, rather than automatically upgrading the transaction to SERIALIZABLE.

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>

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

@@ -317,7 +317,7 @@ sql.ttl.default_delete_rate_limit	integer	0	default delete rate limit (rows per
 sql.ttl.default_select_batch_size	integer	500	default amount of rows to select in a single query during a TTL job	application
 sql.ttl.default_select_rate_limit	integer	0	default select rate limit (rows per second) per node for each TTL job. Use 0 to signify no rate limit.	application
 sql.ttl.job.enabled	boolean	true	whether the TTL job is enabled	application
-sql.txn.read_committed_isolation.enabled	boolean	false	set to true to allow transactions to use the READ COMMITTED isolation level if specified by BEGIN/SET commands	application
+sql.txn.read_committed_isolation.enabled	boolean	true	set to true to allow transactions to use the READ COMMITTED isolation level if specified by BEGIN/SET commands	application
 sql.txn_fingerprint_id_cache.capacity	integer	100	the maximum number of txn fingerprint IDs stored	application
 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	system-visible
 storage.max_sync_duration.fatal.enabled	boolean	true	if true, fatal the process when a disk operation exceeds storage.max_sync_duration	application

docs/generated/settings/settings.html

@@ -267,7 +267,7 @@
 <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-default-select-rate-limit" class="anchored"><code>sql.ttl.default_select_rate_limit</code></div></td><td>integer</td><td><code>0</code></td><td>default select rate limit (rows per second) per node for each TTL job. Use 0 to signify no rate limit.</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-read-committed-isolation-enabled" class="anchored"><code>sql.txn.read_committed_isolation.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>set to true to allow transactions to use the READ COMMITTED isolation level if specified by BEGIN/SET commands</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-sql-txn-read-committed-isolation-enabled" class="anchored"><code>sql.txn.read_committed_isolation.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>set to true to allow transactions to use the READ COMMITTED isolation level if specified by BEGIN/SET commands</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-experimental-eventually-file-only-snapshots-enabled" class="anchored"><code>storage.experimental.eventually_file_only_snapshots.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>set to false to disable eventually-file-only-snapshots (kv.snapshot_receiver.excise.enabled must also be false)</td><td>Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-storage-ingest-split-enabled" class="anchored"><code>storage.ingest_split.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>set to false to disable ingest-time splitting that lowers write-amplification</td><td>Dedicated/Self-Hosted</td></tr>

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_read_committed

@@ -1,9 +1,6 @@
 # tenant-cluster-setting-override-opt: sql.zone_configs.allow_for_secondary_tenant.enabled=true sql.multi_region.allow_abstractions_for_secondary_tenants.enabled=true
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-vec-off multiregion-9node-3region-3azs-tenant multiregion-9node-3region-3azs-no-los
 
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 statement ok
 SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ COMMITTED
 

pkg/cmd/roachtest/tests/orm_helpers.go

@@ -71,7 +71,6 @@ func alterZoneConfigAndClusterSettings(
 
 		// Enable experimental/preview/compatibility features.
 		`SET CLUSTER SETTING sql.defaults.experimental_temporary_tables.enabled = 'true';`,
-		`SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = 'true';`,
 		`ALTER ROLE ALL SET multiple_active_portals_enabled = 'true';`,
 		`ALTER ROLE ALL SET serial_normalization = 'sql_sequence_cached'`,
 		`ALTER ROLE ALL SET statement_timeout = '60s'`,

pkg/cmd/roachtest/tests/ycsb.go

@@ -168,7 +168,6 @@ func registerYCSB(r registry.Registry) {
 
 func enableIsolationLevels(ctx context.Context, t test.Test, db *gosql.DB) error {
 	for _, cmd := range []string{
-		`SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = 'true';`,
 		`SET CLUSTER SETTING sql.txn.snapshot_isolation.enabled = 'true';`,
 	} {
 		if _, err := db.ExecContext(ctx, cmd); err != nil {

pkg/sql/conn_executor.go

@@ -3426,7 +3426,7 @@ var allowReadCommittedIsolation = settings.RegisterBoolSetting(
 	"sql.txn.read_committed_isolation.enabled",
 	"set to true to allow transactions to use the READ COMMITTED isolation "+
 		"level if specified by BEGIN/SET commands",
-	false,
+	true,
 	settings.WithPublic,
 )
 

pkg/sql/conn_executor_test.go

@@ -427,7 +427,6 @@ func TestHalloweenProblemAvoidance(t *testing.T) {
 	defer s.Stopper().Stop(context.Background())
 
 	for _, s := range []string{
-		`SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true;`,
 		`SET CLUSTER SETTING sql.txn.snapshot_isolation.enabled = true;`,
 		`CREATE DATABASE t;`,
 		`CREATE TABLE t.test (x FLOAT);`,
@@ -1470,9 +1469,7 @@ func TestInjectRetryErrors(t *testing.T) {
 	defer s.Stopper().Stop(ctx)
 	defer db.Close()
 
-	_, err := db.Exec(`SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true`)
-	require.NoError(t, err)
-	_, err = db.Exec("SET inject_retry_errors_enabled = 'true'")
+	_, err := db.Exec("SET inject_retry_errors_enabled = 'true'")
 	require.NoError(t, err)
 
 	t.Run("with_savepoints", func(t *testing.T) {

pkg/sql/internal.go

@@ -863,6 +863,9 @@ func applyOverrides(o sessiondata.InternalExecutorOverride, sd *sessiondata.Sess
 	}
 	// We always override the injection knob based on the override struct.
 	sd.InjectRetryErrorsEnabled = o.InjectRetryErrorsEnabled
+	if o.OptimizerUseHistograms {
+		sd.OptimizerUseHistograms = true
+	}
 }
 
 func (ie *InternalExecutor) maybeRootSessionDataOverride(

pkg/sql/logictest/testdata/logic_test/cluster_locks

@@ -350,9 +350,6 @@ SELECT count(*) FROM crdb_internal.cluster_locks WHERE table_name IN ('t','t2')
 
 # Test with different isolation levels.
 
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 statement ok
 SET CLUSTER SETTING sql.txn.snapshot_isolation.enabled = true
 

pkg/sql/logictest/testdata/logic_test/fk_read_committed

@@ -1,6 +1,3 @@
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 statement ok
 CREATE TABLE jars (j INT PRIMARY KEY)
 

pkg/sql/logictest/testdata/logic_test/hash_sharded_index_read_committed

@@ -1,6 +1,3 @@
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 statement ok
 SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ COMMITTED
 

pkg/sql/logictest/testdata/logic_test/read_committed

@@ -1,8 +1,5 @@
 # LogicTest: !local-mixed-23.1
 
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 subtest select_for_update
 
 statement ok

pkg/sql/logictest/testdata/logic_test/select_for_update_read_committed

@@ -1,8 +1,5 @@
 # LogicTest: !local-mixed-23.1
 
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 statement ok
 CREATE TABLE abc (a INT PRIMARY KEY, b INT, c INT, INDEX (b), FAMILY (a, b, c))
 

pkg/sql/logictest/testdata/logic_test/txn

@@ -207,9 +207,6 @@ BEGIN TRANSACTION; SET TRANSACTION ISOLATION LEVEL READ COMMITTED; COMMIT
 
 # It is an error to change the isolation level of a running transaction.
 
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 statement ok
 BEGIN TRANSACTION
 
@@ -237,9 +234,9 @@ statement ok
 ROLLBACK
 
 statement ok
-RESET CLUSTER SETTING sql.txn.read_committed_isolation.enabled
+SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = false
 
-# Transactions default to serializable.
+# Transactions default to serializable when the read_committed cluster setting is off.
 
 statement ok
 BEGIN TRANSACTION

pkg/sql/logictest/testdata/logic_test/unique_read_committed

@@ -1,8 +1,5 @@
 # LogicTest: !local-mixed-23.1
 
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 statement ok
 SET experimental_enable_unique_without_index_constraints = true
 

pkg/sql/opt/exec/execbuilder/testdata/explain_analyze

@@ -1,8 +1,5 @@
 # LogicTest: local
 
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 statement ok
 CREATE TABLE kv (k INT PRIMARY KEY, v INT, FAMILY (k, v))
 

pkg/sql/opt/exec/execbuilder/testdata/fk_read_committed

@@ -1,8 +1,5 @@
 # LogicTest: local
 
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 statement ok
 CREATE TABLE jars (j INT PRIMARY KEY)
 

pkg/sql/opt/exec/execbuilder/testdata/select_for_update_read_committed

@@ -1,8 +1,5 @@
 # LogicTest: local
 
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 statement ok
 CREATE TABLE supermarket (
   person STRING PRIMARY KEY,

pkg/sql/opt/exec/execbuilder/testdata/unique_read_committed

@@ -1,8 +1,5 @@
 # LogicTest: local
 
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 statement ok
 SET experimental_enable_unique_without_index_constraints = true
 

pkg/sql/opt/exec/execbuilder/testdata/update_read_committed

@@ -1,8 +1,5 @@
 # LogicTest: local
 
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 # Test multi-column-family checks under read committed. If a check constraint
 # contains multiple column families but only some of them are updated, the
 # query should fail under read committed.

pkg/sql/opt/exec/execbuilder/testdata/upsert_read_committed

@@ -1,8 +1,5 @@
 # LogicTest: local
 
-statement ok
-SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true
-
 # Test multi-column-family checks under read committed. If a check constraint
 # contains multiple column families but only some of them are updated, the
 # query should fail under read committed.

pkg/sql/pgwire/conn_test.go

@@ -1767,9 +1767,6 @@ func TestSetSessionArguments(t *testing.T) {
 	defer srv.Stopper().Stop(ctx)
 	s := srv.ApplicationLayer()
 
-	_, err := s.SQLConn(t, serverutils.DBName("defaultdb")).Exec(`SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true`)
-	require.NoError(t, err)
-
 	pgURL, cleanupFunc := s.PGUrl(
 		t, serverutils.CertsDirPrefix("testConnClose"), serverutils.User(username.RootUser),
 	)

pkg/sql/pgwire/testdata/pgtest/read_committed

@@ -2,16 +2,12 @@ only crdb
 ----
 
 send
-Query {"String": "SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = 'true'"}
 Query {"String": "SET default_transaction_isolation = 'read committed'"}
 ----
 
 until
 ReadyForQuery
-ReadyForQuery
 ----
-{"Type":"CommandComplete","CommandTag":"SET CLUSTER SETTING"}
-{"Type":"ReadyForQuery","TxStatus":"I"}
 {"Type":"CommandComplete","CommandTag":"SET"}
 {"Type":"ReadyForQuery","TxStatus":"I"}
 

pkg/sql/sessiondata/internal.go

@@ -47,6 +47,10 @@ type InternalExecutorOverride struct {
 	// does **not** propagate further to "nested" executors that are spawned up
 	// by the "top" executor.
 	InjectRetryErrorsEnabled bool
+	// OptimizerUseHistograms indicates whether we should use histograms for
+	// cardinality estimation in the optimizer.
+	// TODO(#102954): this should be removed when #102954 is fixed.
+	OptimizerUseHistograms bool
 }
 
 // NoSessionDataOverride is the empty InternalExecutorOverride which does not

pkg/sql/tests/read_committed_test.go

@@ -89,12 +89,9 @@ func TestReadCommittedStmtRetry(t *testing.T) {
 	defer s.Stopper().Stop(ctx)
 	codec = s.ApplicationLayer().Codec()
 
-	_, err := sqlDB.Exec(`SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true`)
-	require.NoError(t, err)
-
 	// Create a table with three rows. Note that k is not the primary key,
 	// so locking won't be pushed into the initial scan of the UPDATEs below.
-	_, err = sqlDB.Exec(`CREATE TABLE kv (k TEXT, v INT) WITH (sql_stats_automatic_collection_enabled = false);`)
+	_, err := sqlDB.Exec(`CREATE TABLE kv (k TEXT, v INT) WITH (sql_stats_automatic_collection_enabled = false);`)
 	require.NoError(t, err)
 	_, err = sqlDB.Exec(`INSERT INTO kv VALUES ('a', 1);`)
 	require.NoError(t, err)
@@ -181,9 +178,7 @@ func TestReadCommittedReadTimestampNotSteppedOnCommit(t *testing.T) {
 	s, sqlDB, _ := serverutils.StartServer(t, params)
 	defer s.Stopper().Stop(ctx)
 
-	_, err := sqlDB.Exec(`SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true`)
-	require.NoError(t, err)
-	_, err = sqlDB.Exec(`CREATE TABLE kv (k TEXT, v INT) WITH (sql_stats_automatic_collection_enabled = false);`)
+	_, err := sqlDB.Exec(`CREATE TABLE kv (k TEXT, v INT) WITH (sql_stats_automatic_collection_enabled = false);`)
 	require.NoError(t, err)
 
 	// Create a read committed transaction that writes to three rows in three
@@ -226,10 +221,7 @@ func TestReadCommittedVolatileUDF(t *testing.T) {
 	s, sqlDB, _ := serverutils.StartServer(t, params)
 	defer s.Stopper().Stop(ctx)
 
-	_, err := sqlDB.Exec(`SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = true`)
-	require.NoError(t, err)
-
-	_, err = sqlDB.Exec(`CREATE TABLE kv (k TEXT PRIMARY KEY, v INT) WITH (sql_stats_automatic_collection_enabled = false);`)
+	_, err := sqlDB.Exec(`CREATE TABLE kv (k TEXT PRIMARY KEY, v INT) WITH (sql_stats_automatic_collection_enabled = false);`)
 	require.NoError(t, err)
 	_, err = sqlDB.Exec(`INSERT INTO kv VALUES ('a', 10);`)
 	require.NoError(t, err)

pkg/sql/tests/rsg_test.go

@@ -618,7 +618,6 @@ var ignoredErrorPatterns = []string{
 	"cannot call json_object_keys on an array",
 	"cannot set path in scalar",
 	"cannot delete path in scalar",
-	"unable to encode table key: \\*tree\\.DJSON",
 	"path element at position .* is null",
 	"path element is not an integer",
 	"cannot delete from object using integer index",

pkg/sql/ttl/ttljob/ttljob_metrics.go

@@ -18,11 +18,9 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
-	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
-	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
 	"github.com/cockroachdb/cockroach/pkg/util/metric"
 	"github.com/cockroachdb/cockroach/pkg/util/metric/aggmetric"
@@ -225,10 +223,7 @@ func (m *rowLevelTTLMetrics) fetchStatistics(
 			ctx,
 			c.opName,
 			nil,
-			sessiondata.InternalExecutorOverride{
-				User:             username.RootUserName(),
-				QualityOfService: &qosLevel,
-			},
+			getInternalExecutorOverride(qosLevel),
 			fmt.Sprintf(c.query, details.TableID, aost.String()),
 			c.args...,
 		)

pkg/sql/ttl/ttljob/ttljob_processor.go

@@ -301,6 +301,8 @@ func (t *ttlProcessor) runTTLOnQueryBounds(
 			ctx,
 			"pre-select-delete-statement",
 			nil, /* txn */
+			// This is a test-only knob, so we're ok not specifying custom
+			// InternalExecutorOverride.
 			sessiondata.RootUserSessionDataOverride,
 			preSelectStatement,
 		); err != nil {

pkg/sql/ttl/ttljob/ttljob_query_builder.go

@@ -120,6 +120,16 @@ func (b *SelectQueryBuilder) buildQuery() string {
 
 var qosLevel = sessiondatapb.TTLLow
 
+func getInternalExecutorOverride(
+	qosLevel sessiondatapb.QoSLevel,
+) sessiondata.InternalExecutorOverride {
+	return sessiondata.InternalExecutorOverride{
+		User:                   username.RootUserName(),
+		QualityOfService:       &qosLevel,
+		OptimizerUseHistograms: true,
+	}
+}
+
 func (b *SelectQueryBuilder) Run(
 	ctx context.Context, ie isql.Executor,
 ) (_ []tree.Datums, hasNext bool, _ error) {
@@ -148,10 +158,7 @@ func (b *SelectQueryBuilder) Run(
 		ctx,
 		b.selectOpName,
 		nil, /* txn */
-		sessiondata.InternalExecutorOverride{
-			User:             username.RootUserName(),
-			QualityOfService: &qosLevel,
-		},
+		getInternalExecutorOverride(qosLevel),
 		query,
 		b.cachedArgs...,
 	)
@@ -254,10 +261,7 @@ func (b *DeleteQueryBuilder) Run(
 		ctx,
 		b.deleteOpName,
 		txn.KV(),
-		sessiondata.InternalExecutorOverride{
-			User:             username.RootUserName(),
-			QualityOfService: &qosLevel,
-		},
+		getInternalExecutorOverride(qosLevel),
 		query,
 		deleteArgs...,
 	)