From e87a74ecded5a8519b15e22d01db7e8fdaf2b9ca Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Wed, 29 Sep 2021 16:14:35 -0400 Subject: [PATCH] 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 #70566. Release note: None --- pkg/sql/catalog/descs/txn.go | 15 +++++++++++++-- pkg/sql/gcjob/descriptor_utils.go | 6 ++++-- pkg/sql/gcjob/table_garbage_collection.go | 4 +++- pkg/sql/opt/exec/execbuilder/relational.go | 14 +++++++++----- pkg/sql/plan.go | 4 +++- pkg/sql/schema_changer.go | 16 ++++++++++++---- pkg/sql/set_cluster_setting.go | 12 ++++++++++++ 7 files changed, 56 insertions(+), 15 deletions(-) diff --git a/pkg/sql/catalog/descs/txn.go b/pkg/sql/catalog/descs/txn.go index 0b11f69c99f3..a142d60e9f53 100644 --- a/pkg/sql/catalog/descs/txn.go +++ b/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 diff --git a/pkg/sql/gcjob/descriptor_utils.go b/pkg/sql/gcjob/descriptor_utils.go index 6e4830c3d344..33864a4e7d6f 100644 --- a/pkg/sql/gcjob/descriptor_utils.go +++ b/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{} diff --git a/pkg/sql/gcjob/table_garbage_collection.go b/pkg/sql/gcjob/table_garbage_collection.go index 410da3c5073f..782fc22afe91 100644 --- a/pkg/sql/gcjob/table_garbage_collection.go +++ b/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()) } diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go index 4a807483fa26..d694eee050a8 100644 --- a/pkg/sql/opt/exec/execbuilder/relational.go +++ b/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) { diff --git a/pkg/sql/plan.go b/pkg/sql/plan.go index 32c1f486606c..4efe69199d16 100644 --- a/pkg/sql/plan.go +++ b/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. diff --git a/pkg/sql/schema_changer.go b/pkg/sql/schema_changer.go index 6561b8bffd71..8ede456f7c86 100644 --- a/pkg/sql/schema_changer.go +++ b/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{} diff --git a/pkg/sql/set_cluster_setting.go b/pkg/sql/set_cluster_setting.go index 68f73b1bb1ef..d176f3f09fc7 100644 --- a/pkg/sql/set_cluster_setting.go +++ b/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 {