sql: add a cluster setting to avoid system config triggers

This is intended as a short-term workaround to improve performance in
situations of repeated schema changes, like ORM tests.

We have a plan to disable the system config trigger altogether in 22.1 with

This PR provides a cluster setting which allows schema change transactions
to bypass triggerring an update to the system config span. These updates
currently drive only the propagation of zone configs to KV and cluster
settings. The cluster setting behavior is retained until we address cockroachdb#70566.

Release note: None

pkg/sql/catalog/descs/txn.go

@@ -16,6 +16,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/kv"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
@@ -26,6 +27,14 @@ import (
 
 var errTwoVersionInvariantViolated = errors.Errorf("two version invariant violated")
 
+var UnsafeSkipSystemConfigTrigger = settings.RegisterBoolSetting(
+	"sql.catalog.unsafe_skip_system_config_trigger",
+	"avoid setting the system config trigger in transactions which write to "+
+		"the system config. This will unlock performance at the cost of breaking "+
+		"table splits, zone configuration propagation, and cluster settings",
+	false,
+)
+
 // Txn enables callers to run transactions with a *Collection such that all
 // retrieved immutable descriptors are properly leased and all mutable
 // descriptors are handled. The function deals with verifying the two version
@@ -78,8 +87,10 @@ func (cf *CollectionFactory) Txn(
 			deletedDescs = nil
 			descsCol = cf.MakeCollection(nil)
 			defer descsCol.ReleaseAll(ctx)
-			if err := txn.SetSystemConfigTrigger(cf.leaseMgr.Codec().ForSystemTenant()); err != nil {
-				return err
+			if !UnsafeSkipSystemConfigTrigger.Get(&cf.settings.SV) {
+				if err := txn.SetSystemConfigTrigger(cf.leaseMgr.Codec().ForSystemTenant()); err != nil {
+					return err
+				}
 			}
 			if err := f(ctx, txn, &descsCol); err != nil {
 				return err

pkg/sql/gcjob/descriptor_utils.go

@@ -73,8 +73,10 @@ func deleteDatabaseZoneConfig(
 		return nil
 	}
 	return db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
-		if err := txn.SetSystemConfigTrigger(codec.ForSystemTenant()); err != nil {
-			return err
+		if !descs.UnsafeSkipSystemConfigTrigger.Get(&settings.SV) {
+			if err := txn.SetSystemConfigTrigger(codec.ForSystemTenant()); err != nil {
+				return err
+			}
 		}
 		b := &kv.Batch{}
 

pkg/sql/gcjob/table_garbage_collection.go

@@ -73,7 +73,9 @@ func gcTables(
 		}
 
 		// Finished deleting all the table data, now delete the table meta data.
-		if err := sql.DeleteTableDescAndZoneConfig(ctx, execCfg.DB, execCfg.Codec, table); err != nil {
+		if err := sql.DeleteTableDescAndZoneConfig(
+			ctx, execCfg.DB, execCfg.Settings, execCfg.Codec, table,
+		); err != nil {
 			return errors.Wrapf(err, "dropping table descriptor for table %d", table.GetID())
 		}
 

pkg/sql/opt/exec/execbuilder/relational.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/cat"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/constraint"
@@ -156,12 +157,15 @@ func (b *Builder) buildRelational(e memo.RelExpr) (execPlan, error) {
 		// `BEGIN; INSERT INTO ...; CREATE TABLE IF NOT EXISTS ...; COMMIT;`
 		// where the table already exists. This will generate some false schema
 		// cache refreshes, but that's expected to be quite rare in practice.
-		if err := b.evalCtx.Txn.SetSystemConfigTrigger(b.evalCtx.Codec.ForSystemTenant()); err != nil {
-			return execPlan{}, errors.WithSecondaryError(
-				unimplemented.NewWithIssuef(26508,
-					"the first schema change statement in a transaction must precede any writes"),
-				err)
+		if !descs.UnsafeSkipSystemConfigTrigger.Get(&b.evalCtx.Settings.SV) {
+			if err := b.evalCtx.Txn.SetSystemConfigTrigger(b.evalCtx.Codec.ForSystemTenant()); err != nil {
+				return execPlan{}, errors.WithSecondaryError(
+					unimplemented.NewWithIssuef(26508,
+						"the first schema change statement in a transaction must precede any writes"),
+					err)
+			}
 		}
+
 	}
 
 	if opt.IsMutationOp(e) {

pkg/sql/plan.go

@@ -16,6 +16,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
 	"github.com/cockroachdb/cockroach/pkg/sql/execstats"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec"
@@ -539,7 +540,8 @@ func (p *planner) maybePlanHook(ctx context.Context, stmt tree.Statement) (planN
 // Mark transaction as operating on the system DB if the descriptor id
 // is within the SystemConfig range.
 func (p *planner) maybeSetSystemConfig(id descpb.ID) error {
-	if !descpb.IsSystemConfigID(id) {
+	if !descpb.IsSystemConfigID(id) ||
+		descs.UnsafeSkipSystemConfigTrigger.Get(&p.EvalContext().Settings.SV) {
 		return nil
 	}
 	// Mark transaction as operating on the system DB.

pkg/sql/schema_changer.go

@@ -667,7 +667,9 @@ func (sc *SchemaChanger) exec(ctx context.Context) error {
 		} else {
 			// We've dropped a non-physical table, no need for a GC job, let's delete
 			// its descriptor and zone config immediately.
-			if err := DeleteTableDescAndZoneConfig(ctx, sc.db, sc.execCfg.Codec, tableDesc); err != nil {
+			if err := DeleteTableDescAndZoneConfig(
+				ctx, sc.db, sc.settings, sc.execCfg.Codec, tableDesc,
+			); err != nil {
 				return err
 			}
 		}
@@ -2536,12 +2538,18 @@ func (sc *SchemaChanger) applyZoneConfigChangeForMutation(
 
 // DeleteTableDescAndZoneConfig removes a table's descriptor and zone config from the KV database.
 func DeleteTableDescAndZoneConfig(
-	ctx context.Context, db *kv.DB, codec keys.SQLCodec, tableDesc catalog.TableDescriptor,
+	ctx context.Context,
+	db *kv.DB,
+	settings *cluster.Settings,
+	codec keys.SQLCodec,
+	tableDesc catalog.TableDescriptor,
 ) error {
 	log.Infof(ctx, "removing table descriptor and zone config for table %d", tableDesc.GetID())
 	return db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
-		if err := txn.SetSystemConfigTrigger(codec.ForSystemTenant()); err != nil {
-			return err
+		if !descs.UnsafeSkipSystemConfigTrigger.Get(&settings.SV) {
+			if err := txn.SetSystemConfigTrigger(codec.ForSystemTenant()); err != nil {
+				return err
+			}
 		}
 		b := &kv.Batch{}
 

pkg/sql/set_cluster_setting.go

@@ -170,6 +170,18 @@ func (n *setClusterSettingNode) startExec(params runParams) error {
 	if !params.p.ExtendedEvalContext().TxnImplicit {
 		return errors.Errorf("SET CLUSTER SETTING cannot be used inside a transaction")
 	}
+
+	// Set the system config trigger explicitly here as it might not happen
+	// implicitly due to the setting of the
+	// sql.catalog.unsafe_skip_system_config_trigger cluster setting.
+	// The usage of gossip to propagate cluster settings in the system tenant
+	// will be fixed in an upcoming PR with #70566.
+	if err := params.p.EvalContext().Txn.SetSystemConfigTrigger(
+		params.EvalContext().Codec.ForSystemTenant(),
+	); err != nil {
+		return err
+	}
+
 	execCfg := params.extendedEvalCtx.ExecCfg
 	var expectedEncodedValue string
 	if err := execCfg.DB.Txn(params.ctx, func(ctx context.Context, txn *kv.Txn) error {