From 0e45b5fd5c59073d9e53a9b6d1dbce980db32da9 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Wed, 9 Feb 2022 00:26:58 -0500 Subject: [PATCH] sql: remove sql.catalog.unsafe_skip_system_config_trigger.enabled This cluster setting is no longer useful. Release note: None --- pkg/sql/catalog/descs/txn.go | 23 +++------------------- pkg/sql/gcjob/descriptor_utils.go | 7 +++---- pkg/sql/opt/exec/execbuilder/BUILD.bazel | 1 - pkg/sql/opt/exec/execbuilder/relational.go | 8 +++----- pkg/sql/plan.go | 9 +++------ pkg/sql/schema_changer.go | 2 +- pkg/sql/set_cluster_setting.go | 15 -------------- 7 files changed, 13 insertions(+), 52 deletions(-) diff --git a/pkg/sql/catalog/descs/txn.go b/pkg/sql/catalog/descs/txn.go index cffcaf87e2d9..776770e4e215 100644 --- a/pkg/sql/catalog/descs/txn.go +++ b/pkg/sql/catalog/descs/txn.go @@ -17,7 +17,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/clusterversion" "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" @@ -28,21 +27,6 @@ import ( var errTwoVersionInvariantViolated = errors.Errorf("two version invariant violated") -// UnsafeSkipSystemConfigTrigger will prevent setting the system config -// trigger for transactions which write to tables in the system config. The -// implication of setting this to true is that various subsystems which -// rely on that trigger, such as zone configs and replication reports, will -// not work. This can be used to accelerate high-frequency schema changes -// like during an ORM test suite. -var UnsafeSkipSystemConfigTrigger = settings.RegisterBoolSetting( - settings.SystemOnly, - "sql.catalog.unsafe_skip_system_config_trigger.enabled", - "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 @@ -89,10 +73,9 @@ func (cf *CollectionFactory) Txn( deletedDescs = catalog.DescriptorIDSet{} descsCol = cf.MakeCollection(ctx, nil /* temporarySchemaProvider */) defer descsCol.ReleaseAll(ctx) - if !UnsafeSkipSystemConfigTrigger.Get(&cf.settings.SV) && - !cf.settings.Version.IsActive( - ctx, clusterversion.DisableSystemConfigGossipTrigger, - ) { + if !cf.settings.Version.IsActive( + ctx, clusterversion.DisableSystemConfigGossipTrigger, + ) { if err := txn.DeprecatedSetSystemConfigTrigger( cf.leaseMgr.Codec().ForSystemTenant(), ); err != nil { diff --git a/pkg/sql/gcjob/descriptor_utils.go b/pkg/sql/gcjob/descriptor_utils.go index bc717fa19c0f..63dc4171f798 100644 --- a/pkg/sql/gcjob/descriptor_utils.go +++ b/pkg/sql/gcjob/descriptor_utils.go @@ -75,10 +75,9 @@ func deleteDatabaseZoneConfig( return nil } return db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error { - if !descs.UnsafeSkipSystemConfigTrigger.Get(&settings.SV) && - !settings.Version.IsActive( - ctx, clusterversion.DisableSystemConfigGossipTrigger, - ) { + if !settings.Version.IsActive( + ctx, clusterversion.DisableSystemConfigGossipTrigger, + ) { if err := txn.DeprecatedSetSystemConfigTrigger(codec.ForSystemTenant()); err != nil { return err } diff --git a/pkg/sql/opt/exec/execbuilder/BUILD.bazel b/pkg/sql/opt/exec/execbuilder/BUILD.bazel index bb7c3dcf0233..1598212e5e37 100644 --- a/pkg/sql/opt/exec/execbuilder/BUILD.bazel +++ b/pkg/sql/opt/exec/execbuilder/BUILD.bazel @@ -18,7 +18,6 @@ go_library( "//pkg/server/telemetry", "//pkg/sql/catalog/colinfo", "//pkg/sql/catalog/descpb", - "//pkg/sql/catalog/descs", "//pkg/sql/lexbase", "//pkg/sql/mutations", "//pkg/sql/opt", diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go index 32da9f4fc16b..ebc82db8a255 100644 --- a/pkg/sql/opt/exec/execbuilder/relational.go +++ b/pkg/sql/opt/exec/execbuilder/relational.go @@ -20,7 +20,6 @@ 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" @@ -160,10 +159,9 @@ 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 !descs.UnsafeSkipSystemConfigTrigger.Get(&b.evalCtx.Settings.SV) && - !b.evalCtx.Settings.Version.IsActive( - b.evalCtx.Ctx(), clusterversion.DisableSystemConfigGossipTrigger, - ) { + if !b.evalCtx.Settings.Version.IsActive( + b.evalCtx.Ctx(), clusterversion.DisableSystemConfigGossipTrigger, + ) { if err := b.evalCtx.Txn.DeprecatedSetSystemConfigTrigger(b.evalCtx.Codec.ForSystemTenant()); err != nil { return execPlan{}, errors.WithSecondaryError( unimplemented.NewWithIssuef(26508, diff --git a/pkg/sql/plan.go b/pkg/sql/plan.go index 4ea4b61c90a2..b3ac29313fc7 100644 --- a/pkg/sql/plan.go +++ b/pkg/sql/plan.go @@ -17,7 +17,6 @@ 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" @@ -542,11 +541,9 @@ 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) || - descs.UnsafeSkipSystemConfigTrigger.Get(&p.EvalContext().Settings.SV) || - p.execCfg.Settings.Version.IsActive( - p.EvalContext().Ctx(), clusterversion.DisableSystemConfigGossipTrigger, - ) { + if !descpb.IsSystemConfigID(id) || p.execCfg.Settings.Version.IsActive( + p.EvalContext().Ctx(), clusterversion.DisableSystemConfigGossipTrigger, + ) { 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 1a007e2b657a..69c9037dff82 100644 --- a/pkg/sql/schema_changer.go +++ b/pkg/sql/schema_changer.go @@ -2645,7 +2645,7 @@ func DeleteTableDescAndZoneConfig( ) 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 !descs.UnsafeSkipSystemConfigTrigger.Get(&settings.SV) && !settings.Version.IsActive( + if !settings.Version.IsActive( ctx, clusterversion.DisableSystemConfigGossipTrigger, ) { if err := txn.DeprecatedSetSystemConfigTrigger(codec.ForSystemTenant()); err != nil { diff --git a/pkg/sql/set_cluster_setting.go b/pkg/sql/set_cluster_setting.go index f92bb47aa456..09459f84ddda 100644 --- a/pkg/sql/set_cluster_setting.go +++ b/pkg/sql/set_cluster_setting.go @@ -171,21 +171,6 @@ func (n *setClusterSettingNode) startExec(params runParams) error { 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.enabled cluster setting. - // The usage of gossip to propagate cluster settings in the system tenant - // will be fixed in an upcoming PR with #70566. - if !params.EvalContext().Settings.Version.IsActive( - params.ctx, clusterversion.DisableSystemConfigGossipTrigger, - ) { - if err := params.p.EvalContext().Txn.DeprecatedSetSystemConfigTrigger( - 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 {