diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index b78738de0ec7..32d64f69d3f3 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -1364,6 +1364,7 @@ GO_TARGETS = [ "//pkg/sql/catalog/catpb:catpb_test", "//pkg/sql/catalog/catprivilege:catprivilege", "//pkg/sql/catalog/catprivilege:catprivilege_test", + "//pkg/sql/catalog/catsessiondata:catsessiondata", "//pkg/sql/catalog/colinfo/colinfotestutils:colinfotestutils", "//pkg/sql/catalog/colinfo:colinfo", "//pkg/sql/catalog/colinfo:colinfo_test", @@ -2599,6 +2600,7 @@ GET_X_DATA_TARGETS = [ "//pkg/sql/catalog/catformat:get_x_data", "//pkg/sql/catalog/catpb:get_x_data", "//pkg/sql/catalog/catprivilege:get_x_data", + "//pkg/sql/catalog/catsessiondata:get_x_data", "//pkg/sql/catalog/colinfo:get_x_data", "//pkg/sql/catalog/colinfo/colinfotestutils:get_x_data", "//pkg/sql/catalog/dbdesc:get_x_data", diff --git a/pkg/ccl/changefeedccl/avro_test.go b/pkg/ccl/changefeedccl/avro_test.go index f7934f36a5de..d4c40f556568 100644 --- a/pkg/ccl/changefeedccl/avro_test.go +++ b/pkg/ccl/changefeedccl/avro_test.go @@ -20,14 +20,13 @@ import ( "github.com/cockroachdb/apd/v3" "github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl/cdcevent" - "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" @@ -88,7 +87,7 @@ func parseTableDesc(createTableStmt string) (catalog.TableDescriptor, error) { mutDesc.Families = []descpb.ColumnFamilyDescriptor{ {ID: primary, Name: "primary", ColumnIDs: mutDesc.PublicColumnIDs(), ColumnNames: columnNames}, } - return mutDesc, descbuilder.ValidateSelf(mutDesc, clusterversion.TestingClusterVersion) + return mutDesc, desctestutils.TestingValidateSelf(mutDesc) } func parseValues(tableDesc catalog.TableDescriptor, values string) ([]rowenc.EncDatumRow, error) { diff --git a/pkg/ccl/changefeedccl/cdcevent/rowfetcher_cache.go b/pkg/ccl/changefeedccl/cdcevent/rowfetcher_cache.go index 61e97a9cc556..230b680c615f 100644 --- a/pkg/ccl/changefeedccl/cdcevent/rowfetcher_cache.go +++ b/pkg/ccl/changefeedccl/cdcevent/rowfetcher_cache.go @@ -80,7 +80,7 @@ func newRowFetcherCache( return &rowFetcherCache{ codec: codec, leaseMgr: leaseMgr, - collection: cf.NewCollection(ctx, nil /* TemporarySchemaProvider */, nil /* monitor */), + collection: cf.NewCollection(ctx), db: db, fetchers: cache.NewUnorderedCache(DefaultCacheConfig), watchedFamilies: watchedFamilies, diff --git a/pkg/ccl/changefeedccl/changefeed_stmt.go b/pkg/ccl/changefeedccl/changefeed_stmt.go index 9e889794e41c..18a813be66bf 100644 --- a/pkg/ccl/changefeedccl/changefeed_stmt.go +++ b/pkg/ccl/changefeedccl/changefeed_stmt.go @@ -1167,7 +1167,7 @@ func getQualifiedTableName( func getQualifiedTableNameObj( ctx context.Context, execCfg *sql.ExecutorConfig, txn *kv.Txn, desc catalog.TableDescriptor, ) (tree.TableName, error) { - col := execCfg.CollectionFactory.NewCollection(ctx, nil /* TemporarySchemaProvider */, nil /* monitor */) + col := execCfg.CollectionFactory.NewCollection(ctx) dbDesc, err := col.Direct().MustGetDatabaseDescByID(ctx, txn, desc.GetParentID()) if err != nil { return tree.TableName{}, err diff --git a/pkg/ccl/partitionccl/BUILD.bazel b/pkg/ccl/partitionccl/BUILD.bazel index be887e428b2c..10ee7cc620ab 100644 --- a/pkg/ccl/partitionccl/BUILD.bazel +++ b/pkg/ccl/partitionccl/BUILD.bazel @@ -46,7 +46,6 @@ go_test( "//pkg/ccl/storageccl", "//pkg/ccl/testutilsccl", "//pkg/ccl/utilccl", - "//pkg/clusterversion", "//pkg/config", "//pkg/config/zonepb", "//pkg/jobs", @@ -62,7 +61,6 @@ go_test( "//pkg/sql/catalog", "//pkg/sql/catalog/bootstrap", "//pkg/sql/catalog/catalogkeys", - "//pkg/sql/catalog/descbuilder", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/desctestutils", "//pkg/sql/catalog/tabledesc", diff --git a/pkg/ccl/partitionccl/partition_test.go b/pkg/ccl/partitionccl/partition_test.go index fa0161c8b5fa..1db2fdb2c8e4 100644 --- a/pkg/ccl/partitionccl/partition_test.go +++ b/pkg/ccl/partitionccl/partition_test.go @@ -21,7 +21,6 @@ import ( "github.com/cockroachdb/cockroach-go/v2/crdb" "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/ccl/utilccl" - "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/config" "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/jobs" @@ -33,7 +32,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" @@ -155,7 +153,7 @@ func (pt *partitioningTest) parse() error { return err } pt.parsed.tableDesc = mutDesc - if err := descbuilder.ValidateSelf(pt.parsed.tableDesc, clusterversion.TestingClusterVersion); err != nil { + if err := desctestutils.TestingValidateSelf(pt.parsed.tableDesc); err != nil { return err } } diff --git a/pkg/jobs/BUILD.bazel b/pkg/jobs/BUILD.bazel index 271834507ba0..aa67a74c2330 100644 --- a/pkg/jobs/BUILD.bazel +++ b/pkg/jobs/BUILD.bazel @@ -116,7 +116,6 @@ go_test( "//pkg/sql", "//pkg/sql/catalog/catalogkeys", "//pkg/sql/catalog/colinfo", - "//pkg/sql/catalog/descbuilder", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/descs", "//pkg/sql/catalog/desctestutils", diff --git a/pkg/jobs/registry_test.go b/pkg/jobs/registry_test.go index acfab16efd1e..fcc91ad8f60a 100644 --- a/pkg/jobs/registry_test.go +++ b/pkg/jobs/registry_test.go @@ -22,7 +22,6 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" @@ -30,7 +29,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/spanconfig" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" @@ -82,7 +80,7 @@ func writeMutation( ) { tableDesc.Mutations = append(tableDesc.Mutations, m) tableDesc.Version++ - if err := descbuilder.ValidateSelf(tableDesc, clusterversion.TestingClusterVersion); err != nil { + if err := desctestutils.TestingValidateSelf(tableDesc); err != nil { t.Fatal(err) } if err := kvDB.Put( diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index e4bf24d7c3f3..760619116966 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -163,6 +163,7 @@ go_library( "//pkg/sql/cacheutil", "//pkg/sql/catalog/bootstrap", "//pkg/sql/catalog/catalogkeys", + "//pkg/sql/catalog/catsessiondata", "//pkg/sql/catalog/colinfo", "//pkg/sql/catalog/descidgen", "//pkg/sql/catalog/descpb", diff --git a/pkg/server/server_sql.go b/pkg/server/server_sql.go index ae83f379e413..300d2f97df92 100644 --- a/pkg/server/server_sql.go +++ b/pkg/server/server_sql.go @@ -65,6 +65,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/cacheutil" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/hydrateddesc" @@ -658,6 +659,7 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) { hydratedDescCache, spanConfig.splitter, spanConfig.limiter, + catsessiondata.DefaultDescriptorSessionDataProvider, ) clusterIDForSQL := cfg.rpcContext.LogicalClusterID diff --git a/pkg/settings/registry.go b/pkg/settings/registry.go index 7677706d068e..e50664666bdc 100644 --- a/pkg/settings/registry.go +++ b/pkg/settings/registry.go @@ -150,6 +150,7 @@ var retiredSettings = map[string]struct{}{ "sql.ttl.default_range_concurrency": {}, // removed as of 23.1. + "sql.catalog.descs.validate_on_write.enabled": {}, "sql.distsql.max_running_flows": {}, "sql.distsql.flow_scheduler_queueing.enabled": {}, } diff --git a/pkg/sql/BUILD.bazel b/pkg/sql/BUILD.bazel index dca9a2186d7b..e82848c50e35 100644 --- a/pkg/sql/BUILD.bazel +++ b/pkg/sql/BUILD.bazel @@ -323,6 +323,7 @@ go_library( "//pkg/sql/catalog/catformat", "//pkg/sql/catalog/catpb", "//pkg/sql/catalog/catprivilege", + "//pkg/sql/catalog/catsessiondata", "//pkg/sql/catalog/colinfo", "//pkg/sql/catalog/dbdesc", "//pkg/sql/catalog/descbuilder", @@ -687,7 +688,6 @@ go_test( "//pkg/sql/catalog/bootstrap", "//pkg/sql/catalog/catalogkeys", "//pkg/sql/catalog/catpb", - "//pkg/sql/catalog/descbuilder", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/descs", "//pkg/sql/catalog/desctestutils", diff --git a/pkg/sql/alter_primary_key.go b/pkg/sql/alter_primary_key.go index 4353362f5800..de448c3aa344 100644 --- a/pkg/sql/alter_primary_key.go +++ b/pkg/sql/alter_primary_key.go @@ -18,8 +18,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/paramparse" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" @@ -544,7 +545,8 @@ func (p *planner) AlterPrimaryKey( } tableDesc.AddPrimaryKeySwapMutation(swapArgs) - if err := descbuilder.ValidateSelf(tableDesc, version); err != nil { + dvmp := catsessiondata.NewDescriptorSessionDataProvider(p.SessionData()) + if err := descs.ValidateSelf(tableDesc, version, dvmp); err != nil { return err } diff --git a/pkg/sql/catalog/catsessiondata/BUILD.bazel b/pkg/sql/catalog/catsessiondata/BUILD.bazel new file mode 100644 index 000000000000..a553ec78b834 --- /dev/null +++ b/pkg/sql/catalog/catsessiondata/BUILD.bazel @@ -0,0 +1,17 @@ +load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data") +load("@io_bazel_rules_go//go:def.bzl", "go_library") + +go_library( + name = "catsessiondata", + srcs = ["descriptor_session_data_provider.go"], + importpath = "github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata", + visibility = ["//visibility:public"], + deps = [ + "//pkg/sql/catalog/descpb", + "//pkg/sql/catalog/descs", + "//pkg/sql/sessiondata", + "//pkg/sql/sessiondatapb", + ], +) + +get_x_data(name = "get_x_data") diff --git a/pkg/sql/catalog/catsessiondata/descriptor_session_data_provider.go b/pkg/sql/catalog/catsessiondata/descriptor_session_data_provider.go new file mode 100644 index 000000000000..18bf5405c8fa --- /dev/null +++ b/pkg/sql/catalog/catsessiondata/descriptor_session_data_provider.go @@ -0,0 +1,130 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package catsessiondata + +import ( + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" +) + +type sessionData sessiondata.SessionData +type sessionDataStack sessiondata.Stack + +func (sds *sessionDataStack) top() *sessionData { + if sds == nil { + return nil + } + return (*sessionData)((*sessiondata.Stack)(sds).Top()) +} + +// DefaultDescriptorSessionDataProvider is the default implementation of +// descs.DescriptorSessionDataProvider +var DefaultDescriptorSessionDataProvider descs.DescriptorSessionDataProvider = (*sessionData)(nil) +var _ descs.DescriptorSessionDataProvider = (*sessionDataStack)(nil) + +// NewDescriptorSessionDataProvider constructs a +// descs.DescriptorSessionDataProvider instance given session data. +// The pointer may be nil. +func NewDescriptorSessionDataProvider( + sd *sessiondata.SessionData, +) descs.DescriptorSessionDataProvider { + return (*sessionData)(sd) +} + +// NewDescriptorSessionDataStackProvider constructs a +// descs.DescriptorSessionDataProvider instance given a pointer to a session +// data stack. The pointer may be nil. +func NewDescriptorSessionDataStackProvider( + sds *sessiondata.Stack, +) descs.DescriptorSessionDataProvider { + return (*sessionDataStack)(sds) +} + +// HasTemporarySchema implements descs.TemporarySchemaProvider. +func (sd *sessionData) HasTemporarySchema() bool { + return sd != nil +} + +// HasTemporarySchema implements descs.TemporarySchemaProvider. +func (sds *sessionDataStack) HasTemporarySchema() bool { + return sds.top() != nil +} + +// GetTemporarySchemaName implements descs.TemporarySchemaProvider. +func (sd *sessionData) GetTemporarySchemaName() string { + if sd == nil { + return "" + } + return (*sessiondata.SessionData)(sd).SearchPath.GetTemporarySchemaName() +} + +// GetTemporarySchemaName implements descs.TemporarySchemaProvider. +func (sds *sessionDataStack) GetTemporarySchemaName() string { + return sds.top().GetTemporarySchemaName() +} + +// GetTemporarySchemaIDForDB implements descs.TemporarySchemaProvider. +func (sd *sessionData) GetTemporarySchemaIDForDB(dbID descpb.ID) descpb.ID { + if sd == nil { + return descpb.InvalidID + } + ret, _ := (*sessiondata.SessionData)(sd).GetTemporarySchemaIDForDB(uint32(dbID)) + return descpb.ID(ret) +} + +// GetTemporarySchemaIDForDB implements descs.TemporarySchemaProvider. +func (sds *sessionDataStack) GetTemporarySchemaIDForDB(dbID descpb.ID) descpb.ID { + return sds.top().GetTemporarySchemaIDForDB(dbID) +} + +// MaybeGetDatabaseForTemporarySchemaID implements descs.TemporarySchemaProvider. +func (sd *sessionData) MaybeGetDatabaseForTemporarySchemaID(id descpb.ID) descpb.ID { + if sd == nil { + return descpb.InvalidID + } + ret, _ := (*sessiondata.SessionData)(sd).MaybeGetDatabaseForTemporarySchemaID(uint32(id)) + return descpb.ID(ret) +} + +// MaybeGetDatabaseForTemporarySchemaID implements descs.TemporarySchemaProvider. +func (sds *sessionDataStack) MaybeGetDatabaseForTemporarySchemaID(id descpb.ID) descpb.ID { + return sds.top().MaybeGetDatabaseForTemporarySchemaID(id) +} + +// ValidateDescriptorsOnRead implements descs.DescriptorValidationModeProvider. +func (sd *sessionData) ValidateDescriptorsOnRead() bool { + var mode sessiondatapb.DescriptorValidationMode + if sd != nil { + mode = sd.DescriptorValidationMode + } + return mode != sessiondatapb.DescriptorValidationOff +} + +// ValidateDescriptorsOnRead implements descs.DescriptorValidationModeProvider. +func (sds *sessionDataStack) ValidateDescriptorsOnRead() bool { + return sds.top().ValidateDescriptorsOnRead() +} + +// ValidateDescriptorsOnWrite implements descs.DescriptorValidationModeProvider. +func (sd *sessionData) ValidateDescriptorsOnWrite() bool { + var mode sessiondatapb.DescriptorValidationMode + if sd != nil { + mode = sd.DescriptorValidationMode + } + return mode == sessiondatapb.DescriptorValidationOn +} + +// ValidateDescriptorsOnWrite implements descs.DescriptorValidationModeProvider. +func (sds *sessionDataStack) ValidateDescriptorsOnWrite() bool { + return sds.top().ValidateDescriptorsOnWrite() +} diff --git a/pkg/sql/catalog/descbuilder/BUILD.bazel b/pkg/sql/catalog/descbuilder/BUILD.bazel index bbfe22ad3dab..ebaf77b05db9 100644 --- a/pkg/sql/catalog/descbuilder/BUILD.bazel +++ b/pkg/sql/catalog/descbuilder/BUILD.bazel @@ -7,13 +7,11 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder", visibility = ["//visibility:public"], deps = [ - "//pkg/clusterversion", "//pkg/roachpb", "//pkg/sql/catalog", "//pkg/sql/catalog/dbdesc", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/funcdesc", - "//pkg/sql/catalog/internal/validate", "//pkg/sql/catalog/schemadesc", "//pkg/sql/catalog/tabledesc", "//pkg/sql/catalog/typedesc", diff --git a/pkg/sql/catalog/descbuilder/desc_builder.go b/pkg/sql/catalog/descbuilder/desc_builder.go index 028a93d3bae2..2f2b3c5668ee 100644 --- a/pkg/sql/catalog/descbuilder/desc_builder.go +++ b/pkg/sql/catalog/descbuilder/desc_builder.go @@ -11,13 +11,11 @@ package descbuilder import ( - "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/funcdesc" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/validate" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" @@ -172,8 +170,3 @@ func BuildMutable( } return ret, nil } - -// ValidateSelf validates that the descriptor is internally consistent. -func ValidateSelf(desc catalog.Descriptor, version clusterversion.ClusterVersion) error { - return validate.Self(version, desc) -} diff --git a/pkg/sql/catalog/descs/BUILD.bazel b/pkg/sql/catalog/descs/BUILD.bazel index c22ade167025..fd16bc1ead9f 100644 --- a/pkg/sql/catalog/descs/BUILD.bazel +++ b/pkg/sql/catalog/descs/BUILD.bazel @@ -15,6 +15,7 @@ go_library( "leased_descriptors.go", "object.go", "schema.go", + "session_data.go", "synthetic_descriptors.go", "system_table.go", "table.go", @@ -34,7 +35,6 @@ go_library( "//pkg/keys", "//pkg/kv", "//pkg/roachpb", - "//pkg/settings", "//pkg/settings/cluster", "//pkg/spanconfig", "//pkg/sql/catalog", diff --git a/pkg/sql/catalog/descs/collection.go b/pkg/sql/catalog/descs/collection.go index e626435861b1..272ac3e7ae80 100644 --- a/pkg/sql/catalog/descs/collection.go +++ b/pkg/sql/catalog/descs/collection.go @@ -18,7 +18,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" - "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" @@ -35,36 +34,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/iterutil" "github.com/cockroachdb/cockroach/pkg/util/log" - "github.com/cockroachdb/cockroach/pkg/util/mon" "github.com/cockroachdb/errors" ) -// newCollection constructs a Collection. -func newCollection( - ctx context.Context, - leaseMgr *lease.Manager, - settings *cluster.Settings, - codec keys.SQLCodec, - hydrated *hydrateddesc.Cache, - systemDatabase *catkv.SystemDatabaseCache, - virtualSchemas catalog.VirtualSchemas, - temporarySchemaProvider TemporarySchemaProvider, - monitor *mon.BytesMonitor, -) *Collection { - v := settings.Version.ActiveVersion(ctx) - cr := catkv.NewCatalogReader(codec, v, systemDatabase) - return &Collection{ - settings: settings, - version: v, - hydrated: hydrated, - virtual: makeVirtualDescriptors(virtualSchemas), - leased: makeLeasedDescriptors(leaseMgr), - uncommitted: makeUncommittedDescriptors(monitor), - stored: catkv.MakeStoredCatalog(cr, monitor), - temporary: makeTemporaryDescriptors(settings, codec, temporarySchemaProvider), - } -} - // Collection is a collection of descriptors held by a single session that // serves SQL requests, or a background job using descriptors. The // collection is cleared using ReleaseAll() which is called at the @@ -111,8 +83,12 @@ type Collection struct { // non-public schema elements during a schema change). synthetic syntheticDescriptors - // temporary contains logic to access temporary schema descriptors. - temporary temporaryDescriptors + // temporarySchemaProvider is used to access temporary schema descriptors. + temporarySchemaProvider TemporarySchemaProvider + + // validationModeProvider is used to access the session var which determines + // the descriptor validation mode: 'on', 'off' or 'read_only'. + validationModeProvider DescriptorValidationModeProvider // hydrated is node-level cache of table descriptors which utilize // user-defined types. @@ -255,22 +231,13 @@ func (tc *Collection) AddUncommittedDescriptor( return tc.uncommitted.upsert(ctx, desc) } -// ValidateOnWriteEnabled is the cluster setting used to enable or disable -// validating descriptors prior to writing. -var ValidateOnWriteEnabled = settings.RegisterBoolSetting( - settings.TenantWritable, - "sql.catalog.descs.validate_on_write.enabled", - "set to true to validate descriptors prior to writing, false to disable; default is true", - true, /* defaultValue */ -) - // WriteDescToBatch calls MaybeIncrementVersion, adds the descriptor to the // collection as an uncommitted descriptor, and writes it into b. func (tc *Collection) WriteDescToBatch( ctx context.Context, kvTrace bool, desc catalog.MutableDescriptor, b *kv.Batch, ) error { desc.MaybeIncrementVersion() - if !tc.skipValidationOnWrite && ValidateOnWriteEnabled.Get(&tc.settings.SV) { + if !tc.skipValidationOnWrite && tc.validationModeProvider.ValidateDescriptorsOnWrite() { if err := validate.Self(tc.version, desc); err != nil { return err } @@ -588,11 +555,12 @@ func (tc *Collection) SetSession(session sqlliveness.Session) { tc.sqlLivenessSession = session } -// SetTemporaryDescriptors is used in the context of the internal executor -// to override the temporary descriptors during temporary object -// cleanup. -func (tc *Collection) SetTemporaryDescriptors(provider TemporarySchemaProvider) { - tc.temporary = makeTemporaryDescriptors(tc.settings, tc.codec(), provider) +// SetDescriptorSessionDataProvider sets a DescriptorSessionDataProvider for +// this Collection. +func (tc *Collection) SetDescriptorSessionDataProvider(dsdp DescriptorSessionDataProvider) { + tc.stored.DescriptorValidationModeProvider = dsdp + tc.temporarySchemaProvider = dsdp + tc.validationModeProvider = dsdp } // Direct exports the catkv.Direct interface. @@ -600,7 +568,7 @@ type Direct = catkv.Direct // Direct provides direct access to the underlying KV-storage. func (tc *Collection) Direct() Direct { - return catkv.MakeDirect(tc.codec(), tc.version) + return catkv.MakeDirect(tc.codec(), tc.version, tc.validationModeProvider) } // MakeTestCollection makes a Collection that can be used for tests. diff --git a/pkg/sql/catalog/descs/collection_test.go b/pkg/sql/catalog/descs/collection_test.go index c9df7e9ecf17..b93f35f08861 100644 --- a/pkg/sql/catalog/descs/collection_test.go +++ b/pkg/sql/catalog/descs/collection_test.go @@ -70,8 +70,7 @@ func TestCollectionWriteDescToBatch(t *testing.T) { tdb.Exec(t, `CREATE TABLE db.schema.table()`) db := s0.DB() - descriptors := s0.ExecutorConfig().(sql.ExecutorConfig).CollectionFactory. - NewCollection(ctx, nil /* TemporarySchemaProvider */, nil /* Monitor */) + descriptors := s0.ExecutorConfig().(sql.ExecutorConfig).CollectionFactory.NewCollection(ctx) // Note this transaction abuses the mechanisms normally required for updating // tables and is just for testing what this test intends to exercise. @@ -653,8 +652,9 @@ func TestCollectionProperlyUsesMemoryMonitoring(t *testing.T) { monitor.Start(ctx, nil, mon.NewStandaloneBudget(math.MaxInt64)) // Create a `Collection` with monitor hooked up. - col := tc.Server(0).ExecutorConfig().(sql.ExecutorConfig).CollectionFactory. - NewCollection(ctx, nil /* temporarySchemaProvider */, monitor) + col := tc.Server(0).ExecutorConfig().(sql.ExecutorConfig).CollectionFactory.NewCollection( + ctx, descs.WithMonitor(monitor), + ) require.Equal(t, int64(0), monitor.AllocBytes()) // Read all the descriptors into `col` and assert this read will finish without error. @@ -672,8 +672,9 @@ func TestCollectionProperlyUsesMemoryMonitoring(t *testing.T) { require.Equal(t, int64(0), monitor.AllocBytes()) // Repeat the process again and assert this time memory allocation will err out. - col = tc.Server(0).ExecutorConfig().(sql.ExecutorConfig).CollectionFactory. - NewCollection(ctx, nil /* temporarySchemaProvider */, monitor) + col = tc.Server(0).ExecutorConfig().(sql.ExecutorConfig).CollectionFactory.NewCollection( + ctx, descs.WithMonitor(monitor), + ) _, err2 := col.GetAllDescriptors(ctx, txn) require.Error(t, err2) diff --git a/pkg/sql/catalog/descs/descriptor.go b/pkg/sql/catalog/descs/descriptor.go index 356e61261251..da33f08cb26b 100644 --- a/pkg/sql/catalog/descs/descriptor.go +++ b/pkg/sql/catalog/descs/descriptor.go @@ -245,7 +245,7 @@ func (q *byIDLookupContext) lookupVirtual( func (q *byIDLookupContext) lookupTemporary( id descpb.ID, ) (catalog.Descriptor, catalog.ValidationLevel, error) { - td := q.tc.temporary.getSchemaByID(id) + td := q.tc.getTemporarySchemaByID(id) if td == nil { return nil, catalog.NoValidation, nil } @@ -434,7 +434,7 @@ func (tc *Collection) getNonVirtualDescriptorID( if !isSchema || !isTemporarySchema(name) { return continueLookups, descpb.InvalidID, nil } - avoidFurtherLookups, td := tc.temporary.getSchemaByName(ctx, parentID, name) + avoidFurtherLookups, td := tc.getTemporarySchemaByName(parentID, name) if td != nil { return haltLookups, td.GetID(), nil } @@ -551,6 +551,9 @@ func (tc *Collection) finalizeDescriptors( } } // Ensure that all descriptors are sufficiently validated. + if !tc.validationModeProvider.ValidateDescriptorsOnRead() { + return nil + } requiredLevel := validate.MutableRead if !flags.RequireMutable && !flags.AvoidLeased { requiredLevel = validate.ImmutableRead diff --git a/pkg/sql/catalog/descs/factory.go b/pkg/sql/catalog/descs/factory.go index 32fb3bf1040e..149b2807ae6b 100644 --- a/pkg/sql/catalog/descs/factory.go +++ b/pkg/sql/catalog/descs/factory.go @@ -28,15 +28,16 @@ import ( // CollectionFactory is used to construct a new Collection. type CollectionFactory struct { - settings *cluster.Settings - codec keys.SQLCodec - leaseMgr *lease.Manager - virtualSchemas catalog.VirtualSchemas - hydrated *hydrateddesc.Cache - systemDatabase *catkv.SystemDatabaseCache - spanConfigSplitter spanconfig.Splitter - spanConfigLimiter spanconfig.Limiter - defaultMonitor *mon.BytesMonitor + settings *cluster.Settings + codec keys.SQLCodec + leaseMgr *lease.Manager + virtualSchemas catalog.VirtualSchemas + hydrated *hydrateddesc.Cache + systemDatabase *catkv.SystemDatabaseCache + spanConfigSplitter spanconfig.Splitter + spanConfigLimiter spanconfig.Limiter + defaultMonitor *mon.BytesMonitor + defaultDescriptorSessionDataProvider DescriptorSessionDataProvider } // GetClusterSettings returns the cluster setting from the collection factory. @@ -89,6 +90,7 @@ func NewCollectionFactory( hydrated *hydrateddesc.Cache, spanConfigSplitter spanconfig.Splitter, spanConfigLimiter spanconfig.Limiter, + defaultDescriptorSessionDataProvider DescriptorSessionDataProvider, ) *CollectionFactory { return &CollectionFactory{ settings: settings, @@ -102,6 +104,7 @@ func NewCollectionFactory( defaultMonitor: mon.NewUnlimitedMonitor(ctx, "CollectionFactoryDefaultUnlimitedMonitor", mon.MemoryResource, nil /* curCount */, nil, /* maxHist */ 0 /* noteworthy */, settings), + defaultDescriptorSessionDataProvider: defaultDescriptorSessionDataProvider, } } @@ -116,15 +119,58 @@ func NewBareBonesCollectionFactory( } } +type constructorConfig struct { + dsdp DescriptorSessionDataProvider + monitor *mon.BytesMonitor +} + +// Option is how optional construction parameters are provided to the +// CollectionFactory construction method. +type Option func(b *constructorConfig) + +// WithDescriptorSessionDataProvider supplies a DescriptorSessionDataProvider +// instance to the Collection constructor. +func WithDescriptorSessionDataProvider( + dsdp DescriptorSessionDataProvider, +) func(cfg *constructorConfig) { + return func(cfg *constructorConfig) { + cfg.dsdp = dsdp + } +} + +// WithMonitor supplies a mon.BytesMonitor instance to the Collection +// constructor. +func WithMonitor(monitor *mon.BytesMonitor) func(b *constructorConfig) { + return func(cfg *constructorConfig) { + cfg.monitor = monitor + } +} + // NewCollection constructs a new Collection. -func (cf *CollectionFactory) NewCollection( - ctx context.Context, temporarySchemaProvider TemporarySchemaProvider, monitor *mon.BytesMonitor, -) *Collection { - if monitor == nil { - // If an upstream monitor is not provided, the default, unlimited monitor will be used. - // All downstream resource allocation/releases on this default monitor will then be no-ops. - monitor = cf.defaultMonitor +// When no DescriptorSessionDataProvider is provided, the factory falls back to +// the default instances which behaves as if the session data stack were empty. +// Whe no mon.BytesMonitor is provided, the factory falls back to a default, +// unlimited monitor for which all downstream resource allocation/releases are +// no-ops. +func (cf *CollectionFactory) NewCollection(ctx context.Context, options ...Option) *Collection { + cfg := constructorConfig{ + dsdp: cf.defaultDescriptorSessionDataProvider, + monitor: cf.defaultMonitor, + } + for _, opt := range options { + opt(&cfg) + } + v := cf.settings.Version.ActiveVersion(ctx) + cr := catkv.NewCatalogReader(cf.codec, v, cf.systemDatabase) + return &Collection{ + settings: cf.settings, + version: v, + hydrated: cf.hydrated, + virtual: makeVirtualDescriptors(cf.virtualSchemas), + leased: makeLeasedDescriptors(cf.leaseMgr), + uncommitted: makeUncommittedDescriptors(cfg.monitor), + stored: catkv.MakeStoredCatalog(cr, cfg.dsdp, cfg.monitor), + temporarySchemaProvider: cfg.dsdp, + validationModeProvider: cfg.dsdp, } - return newCollection(ctx, cf.leaseMgr, cf.settings, cf.codec, cf.hydrated, cf.systemDatabase, - cf.virtualSchemas, temporarySchemaProvider, monitor) } diff --git a/pkg/sql/catalog/descs/session_data.go b/pkg/sql/catalog/descs/session_data.go new file mode 100644 index 000000000000..b7f4c1bb1945 --- /dev/null +++ b/pkg/sql/catalog/descs/session_data.go @@ -0,0 +1,47 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package descs + +import ( + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/catkv" +) + +// DescriptorSessionDataProvider is the union of various interfaces which +// expose descriptor-related state from session data. +type DescriptorSessionDataProvider interface { + TemporarySchemaProvider + DescriptorValidationModeProvider +} + +// DescriptorValidationModeProvider exports the same interface in catkv. +type DescriptorValidationModeProvider = catkv.DescriptorValidationModeProvider + +// TemporarySchemaProvider encapsulates the temporary schema information in +// the current session data. +type TemporarySchemaProvider interface { + + // HasTemporarySchema returns false iff the there is no temporary schemas in + // the underlying session data. + HasTemporarySchema() bool + + // GetTemporarySchemaName returns the name of the temporary schema for this + // session according to the search path, if it exists. + GetTemporarySchemaName() string + + // GetTemporarySchemaIDForDB returns the ID of the temporary schema for this + // session for the given database ID. + GetTemporarySchemaIDForDB(dbID descpb.ID) descpb.ID + + // MaybeGetDatabaseForTemporarySchemaID returns the ID of the parent database + // of the temporary schema with the given ID, if it exists, 0 otherwise. + MaybeGetDatabaseForTemporarySchemaID(id descpb.ID) descpb.ID +} diff --git a/pkg/sql/catalog/descs/temporary_descriptors.go b/pkg/sql/catalog/descs/temporary_descriptors.go index 8e91b6851922..feb318ec0d0a 100644 --- a/pkg/sql/catalog/descs/temporary_descriptors.go +++ b/pkg/sql/catalog/descs/temporary_descriptors.go @@ -11,109 +11,52 @@ package descs import ( - "context" - - "github.com/cockroachdb/cockroach/pkg/keys" - "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" - "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" ) -type temporaryDescriptors struct { - settings *cluster.Settings - codec keys.SQLCodec - tsp TemporarySchemaProvider -} - -func makeTemporaryDescriptors( - settings *cluster.Settings, codec keys.SQLCodec, temporarySchemaProvider TemporarySchemaProvider, -) temporaryDescriptors { - return temporaryDescriptors{ - settings: settings, - codec: codec, - tsp: temporarySchemaProvider, - } -} - -// TemporarySchemaProvider is an interface that provides temporary schema -// details on the current session. -type TemporarySchemaProvider interface { - GetTemporarySchemaName() string - GetTemporarySchemaIDForDB(descpb.ID) (descpb.ID, bool) - MaybeGetDatabaseForTemporarySchemaID(descpb.ID) (descpb.ID, bool) -} - -type temporarySchemaProviderImpl sessiondata.Stack - -var _ TemporarySchemaProvider = (*temporarySchemaProviderImpl)(nil) - -// NewTemporarySchemaProvider creates a TemporarySchemaProvider. -func NewTemporarySchemaProvider(sds *sessiondata.Stack) TemporarySchemaProvider { - return (*temporarySchemaProviderImpl)(sds) -} - -// GetTemporarySchemaName implements the TemporarySchemaProvider interface. -func (impl *temporarySchemaProviderImpl) GetTemporarySchemaName() string { - return (*sessiondata.Stack)(impl).Top().SearchPath.GetTemporarySchemaName() -} - -// GetTemporarySchemaIDForDB implements the TemporarySchemaProvider interface. -func (impl *temporarySchemaProviderImpl) GetTemporarySchemaIDForDB(id descpb.ID) (descpb.ID, bool) { - ret, found := (*sessiondata.Stack)(impl).Top().GetTemporarySchemaIDForDB(uint32(id)) - return descpb.ID(ret), found -} - -// MaybeGetDatabaseForTemporarySchemaID implements the TemporarySchemaProvider interface. -func (impl *temporarySchemaProviderImpl) MaybeGetDatabaseForTemporarySchemaID( - id descpb.ID, -) (descpb.ID, bool) { - ret, found := (*sessiondata.Stack)(impl).Top().MaybeGetDatabaseForTemporarySchemaID(uint32(id)) - return descpb.ID(ret), found -} - -// getSchemaByName assumes that the schema name carries the `pg_temp` prefix. +// getTemporarySchemaByName assumes that the schema name carries the `pg_temp` +// prefix. // It will exhaustively search for the schema, first checking the local session // data and then consulting the namespace table to discover if this schema // exists as a part of another session. // If it did not find a schema, it also returns a boolean flag indicating // whether the search is known to have been exhaustive or not. -func (td *temporaryDescriptors) getSchemaByName( - ctx context.Context, dbID descpb.ID, schemaName string, +func (tc *Collection) getTemporarySchemaByName( + dbID descpb.ID, schemaName string, ) (avoidFurtherLookups bool, _ catalog.SchemaDescriptor) { // If a temp schema is requested, check if it's for the current session, or // else fall back to reading from the store. - if tsp := td.tsp; tsp != nil { - if schemaName == catconstants.PgTempSchemaName || schemaName == tsp.GetTemporarySchemaName() { - schemaID, found := tsp.GetTemporarySchemaIDForDB(dbID) - if !found { - return true, nil - } - return true, schemadesc.NewTemporarySchema( - tsp.GetTemporarySchemaName(), - schemaID, - dbID, - ) - } + if !tc.temporarySchemaProvider.HasTemporarySchema() { + return false, nil } - return false, nil -} - -// getSchemaByID returns the schema descriptor if it is temporary and belongs -// to the current session. -func (td *temporaryDescriptors) getSchemaByID(schemaID descpb.ID) catalog.SchemaDescriptor { - tsp := td.tsp - if tsp == nil { - return nil + tempSchemaName := tc.temporarySchemaProvider.GetTemporarySchemaName() + if schemaName != catconstants.PgTempSchemaName && schemaName != tempSchemaName { + return false, nil } - if dbID, exists := tsp.MaybeGetDatabaseForTemporarySchemaID(schemaID); exists { - return schemadesc.NewTemporarySchema( - tsp.GetTemporarySchemaName(), - schemaID, - dbID, - ) + schemaID := tc.temporarySchemaProvider.GetTemporarySchemaIDForDB(dbID) + if schemaID == descpb.InvalidID { + return true, nil + } + return true, schemadesc.NewTemporarySchema( + tempSchemaName, + schemaID, + dbID, + ) +} + +// getTemporarySchemaByID returns the schema descriptor if it is temporary and +// belongs to the current session. +func (tc *Collection) getTemporarySchemaByID(schemaID descpb.ID) catalog.SchemaDescriptor { + dbID := tc.temporarySchemaProvider.MaybeGetDatabaseForTemporarySchemaID(schemaID) + if dbID == descpb.InvalidID { + return nil } - return nil + return schemadesc.NewTemporarySchema( + tc.temporarySchemaProvider.GetTemporarySchemaName(), + schemaID, + dbID, + ) } diff --git a/pkg/sql/catalog/descs/validate.go b/pkg/sql/catalog/descs/validate.go index 52c9c268a9e5..43db064c9628 100644 --- a/pkg/sql/catalog/descs/validate.go +++ b/pkg/sql/catalog/descs/validate.go @@ -30,6 +30,9 @@ func (tc *Collection) Validate( targetLevel catalog.ValidationLevel, descriptors ...catalog.Descriptor, ) (err error) { + if !tc.validationModeProvider.ValidateDescriptorsOnRead() && !tc.validationModeProvider.ValidateDescriptorsOnWrite() { + return nil + } vd := tc.newValidationDereferencer(txn) version := tc.settings.Version.ActiveVersion(ctx) return validate.Validate( @@ -48,7 +51,7 @@ func (tc *Collection) Validate( // be one version behind, in which case it's possible (and legitimate) that // those are missing back-references which would cause validation to fail. func (tc *Collection) ValidateUncommittedDescriptors(ctx context.Context, txn *kv.Txn) (err error) { - if tc.skipValidationOnWrite || !ValidateOnWriteEnabled.Get(&tc.settings.SV) { + if tc.skipValidationOnWrite || !tc.validationModeProvider.ValidateDescriptorsOnWrite() { return nil } var descs []catalog.Descriptor @@ -115,3 +118,16 @@ func (c collectionBackedDereferencer) DereferenceDescriptorIDs( // TODO(postamar): namespace operations in general should go through Collection return c.sd.DereferenceDescriptorIDs(ctx, reqs) } + +// ValidateSelf validates that the descriptor is internally consistent. +// Validation may be skipped depending on mode. +func ValidateSelf( + desc catalog.Descriptor, + version clusterversion.ClusterVersion, + dvmp DescriptorValidationModeProvider, +) error { + if !dvmp.ValidateDescriptorsOnRead() && !dvmp.ValidateDescriptorsOnWrite() { + return nil + } + return validate.Self(version, desc) +} diff --git a/pkg/sql/catalog/desctestutils/BUILD.bazel b/pkg/sql/catalog/desctestutils/BUILD.bazel index a7a2fcb8b48d..930ff399bb48 100644 --- a/pkg/sql/catalog/desctestutils/BUILD.bazel +++ b/pkg/sql/catalog/desctestutils/BUILD.bazel @@ -13,6 +13,7 @@ go_library( "//pkg/sql/catalog", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/internal/catkv", + "//pkg/sql/catalog/internal/validate", "//pkg/sql/catalog/tabledesc", "@com_github_cockroachdb_errors//:errors", ], diff --git a/pkg/sql/catalog/desctestutils/descriptor_test_utils.go b/pkg/sql/catalog/desctestutils/descriptor_test_utils.go index fb6d7a5d73a8..2af97993796c 100644 --- a/pkg/sql/catalog/desctestutils/descriptor_test_utils.go +++ b/pkg/sql/catalog/desctestutils/descriptor_test_utils.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/catkv" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/validate" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/errors" ) @@ -35,7 +36,9 @@ func TestingGetDatabaseDescriptorWithVersion( ) catalog.DatabaseDescriptor { ctx := context.Background() var desc catalog.Descriptor - direct := catkv.MakeDirect(codec, version) + direct := catkv.MakeDirect( + codec, version, catkv.DefaultDescriptorValidationModeProvider, + ) if err := kvDB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) (err error) { id, err := direct.LookupDescriptorID(ctx, txn, keys.RootNamespaceID, keys.RootNamespaceID, database) if err != nil { @@ -73,7 +76,9 @@ func TestingGetSchemaDescriptorWithVersion( ) catalog.SchemaDescriptor { ctx := context.Background() var desc catalog.Descriptor - direct := catkv.MakeDirect(codec, version) + direct := catkv.MakeDirect( + codec, version, catkv.DefaultDescriptorValidationModeProvider, + ) if err := kvDB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) (err error) { schemaID, err := direct.LookupDescriptorID(ctx, txn, dbID, keys.RootNamespaceID, schemaName) if err != nil { @@ -169,7 +174,9 @@ func testingGetObjectDescriptor( object string, ) (desc catalog.Descriptor) { ctx := context.Background() - direct := catkv.MakeDirect(codec, version) + direct := catkv.MakeDirect( + codec, version, catkv.DefaultDescriptorValidationModeProvider, + ) if err := kvDB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) (err error) { dbID, err := direct.LookupDescriptorID(ctx, txn, keys.RootNamespaceID, keys.RootNamespaceID, database) if err != nil { @@ -199,3 +206,9 @@ func testingGetObjectDescriptor( } return desc } + +// TestingValidateSelf is a convenience function for internal descriptor +// validation. +func TestingValidateSelf(desc catalog.Descriptor) error { + return validate.Self(clusterversion.TestingClusterVersion, desc) +} diff --git a/pkg/sql/catalog/internal/catkv/direct.go b/pkg/sql/catalog/internal/catkv/direct.go index 69c9b98a9b97..d94c7dcbd76f 100644 --- a/pkg/sql/catalog/internal/catkv/direct.go +++ b/pkg/sql/catalog/internal/catkv/direct.go @@ -148,15 +148,34 @@ type direct struct { var _ Direct = &direct{} // MakeDirect returns an implementation of Direct. -func MakeDirect(codec keys.SQLCodec, version clusterversion.ClusterVersion) Direct { +func MakeDirect( + codec keys.SQLCodec, version clusterversion.ClusterVersion, dvmp DescriptorValidationModeProvider, +) Direct { return &direct{ StoredCatalog: StoredCatalog{ - CatalogReader: NewUncachedCatalogReader(codec), + CatalogReader: NewUncachedCatalogReader(codec), + DescriptorValidationModeProvider: dvmp, }, version: version, } } +// DefaultDescriptorValidationModeProvider is the default implementation of +// DescriptorValidationModeProvider. +var DefaultDescriptorValidationModeProvider DescriptorValidationModeProvider = &defaultDescriptorValidationModeProvider{} + +type defaultDescriptorValidationModeProvider struct{} + +// ValidateDescriptorsOnRead implements DescriptorValidationModeProvider. +func (d *defaultDescriptorValidationModeProvider) ValidateDescriptorsOnRead() bool { + return true +} + +// ValidateDescriptorsOnWrite implements DescriptorValidationModeProvider. +func (d *defaultDescriptorValidationModeProvider) ValidateDescriptorsOnWrite() bool { + return true +} + // MaybeGetDescriptorByIDUnvalidated is part of the Direct interface. func (d *direct) MaybeGetDescriptorByIDUnvalidated( ctx context.Context, txn *kv.Txn, id descpb.ID, @@ -178,10 +197,12 @@ func (d *direct) MustGetDescriptorsByID( if err != nil { return nil, err } - vd := d.NewValidationDereferencer(txn) - ve := validate.Validate(ctx, d.version, vd, catalog.ValidationReadTelemetry, validate.ImmutableRead, descs...) - if err := ve.CombinedError(); err != nil { - return nil, err + if d.ValidateDescriptorsOnRead() { + vd := d.NewValidationDereferencer(txn) + ve := validate.Validate(ctx, d.version, vd, catalog.ValidationReadTelemetry, validate.ImmutableRead, descs...) + if err := ve.CombinedError(); err != nil { + return nil, err + } } return descs, nil } diff --git a/pkg/sql/catalog/internal/catkv/stored_catalog.go b/pkg/sql/catalog/internal/catkv/stored_catalog.go index 45af2c41076d..bc25256c7961 100644 --- a/pkg/sql/catalog/internal/catkv/stored_catalog.go +++ b/pkg/sql/catalog/internal/catkv/stored_catalog.go @@ -33,6 +33,7 @@ import ( // a catalogReader and used for direct catalog access, see MakeDirect. type StoredCatalog struct { CatalogReader + DescriptorValidationModeProvider // cache mirrors the descriptors in storage. // This map does not store descriptors by name. @@ -63,14 +64,27 @@ type StoredCatalog struct { memAcc *mon.BoundAccount } +// DescriptorValidationModeProvider encapsulates the descriptor_validation +// session variable state. +type DescriptorValidationModeProvider interface { + + // ValidateDescriptorsOnRead returns true iff 'on' or 'read_only'. + ValidateDescriptorsOnRead() bool + + // ValidateDescriptorsOnWrite returns true iff 'on'. + ValidateDescriptorsOnWrite() bool +} + // MakeStoredCatalog returns a new instance of StoredCatalog. -func MakeStoredCatalog(cr CatalogReader, monitor *mon.BytesMonitor) StoredCatalog { - sd := StoredCatalog{CatalogReader: cr} +func MakeStoredCatalog( + cr CatalogReader, dvmp DescriptorValidationModeProvider, monitor *mon.BytesMonitor, +) StoredCatalog { + sc := StoredCatalog{CatalogReader: cr, DescriptorValidationModeProvider: dvmp} if monitor != nil { memAcc := monitor.MakeBoundAccount() - sd.memAcc = &memAcc + sc.memAcc = &memAcc } - return sd + return sc } // Reset zeroes the object for re-use in a new transaction. @@ -82,10 +96,11 @@ func (sc *StoredCatalog) Reset(ctx context.Context) { } old := *sc *sc = StoredCatalog{ - CatalogReader: old.CatalogReader, - cache: old.cache, - nameIndex: old.nameIndex, - memAcc: old.memAcc, + CatalogReader: old.CatalogReader, + DescriptorValidationModeProvider: old.DescriptorValidationModeProvider, + cache: old.cache, + nameIndex: old.nameIndex, + memAcc: old.memAcc, } } @@ -396,10 +411,12 @@ func (c storedCatalogBackedDereferencer) DereferenceDescriptors( if desc == nil { continue } - if err = validate.Self(version, desc); err != nil { - return nil, err + if c.sc.ValidateDescriptorsOnRead() { + if err = validate.Self(version, desc); err != nil { + return nil, err + } + c.sc.UpdateValidationLevel(desc, catalog.ValidationLevelSelfOnly) } - c.sc.UpdateValidationLevel(desc, catalog.ValidationLevelSelfOnly) ret[fallbackRetIndexes[j]] = desc } } diff --git a/pkg/sql/catalog/lease/BUILD.bazel b/pkg/sql/catalog/lease/BUILD.bazel index 772da24cb559..710eff010d27 100644 --- a/pkg/sql/catalog/lease/BUILD.bazel +++ b/pkg/sql/catalog/lease/BUILD.bazel @@ -87,7 +87,6 @@ go_test( "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/descs", "//pkg/sql/catalog/desctestutils", - "//pkg/sql/catalog/internal/catkv", "//pkg/sql/catalog/tabledesc", "//pkg/sql/pgwire/pgcode", "//pkg/sql/rowenc/keyside", diff --git a/pkg/sql/catalog/lease/helpers_test.go b/pkg/sql/catalog/lease/helpers_test.go index 7b7ee44dceb1..1c9ac93e3314 100644 --- a/pkg/sql/catalog/lease/helpers_test.go +++ b/pkg/sql/catalog/lease/helpers_test.go @@ -19,7 +19,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/catkv" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/retry" @@ -174,8 +173,7 @@ func (m *Manager) PublishMultiple( for _, id := range ids { // Re-read the current versions of the descriptor, this time // transactionally. - direct := catkv.MakeDirect(m.storage.codec, m.storage.settings.Version.ActiveVersion(ctx)) - desc, err := direct.MustGetDescriptorByID(ctx, txn, id, catalog.Any) + desc, err := m.storage.makeDirect(ctx).MustGetDescriptorByID(ctx, txn, id, catalog.Any) // Due to details in #51417, it is possible for a user to request a // descriptor which no longer exists. In that case, just return an error. if err != nil { diff --git a/pkg/sql/catalog/lease/lease.go b/pkg/sql/catalog/lease/lease.go index fde1d75ac02c..8cb4b2457ced 100644 --- a/pkg/sql/catalog/lease/lease.go +++ b/pkg/sql/catalog/lease/lease.go @@ -31,7 +31,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/catkv" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqlutil" @@ -127,7 +126,6 @@ func (m *Manager) WaitForOneVersion( ) (desc catalog.Descriptor, _ error) { for lastCount, r := 0, retry.Start(retryOpts); r.Next(); { if err := m.DB().Txn(ctx, func(ctx context.Context, txn *kv.Txn) (err error) { - sc := catkv.MakeDirect(m.storage.codec, m.storage.settings.Version.ActiveVersion(ctx)) // Use the lower-level MaybeGetDescriptorByIDUnvalidated to avoid // performing validation while waiting for leases to drain. // Validation is somewhat expensive but more importantly, is not @@ -135,7 +133,7 @@ func (m *Manager) WaitForOneVersion( // descriptors can be removed or made invalid. For instance, the // descriptor could be a type or a schema which is dropped by a subsequent // concurrent schema change. - desc, err = sc.MaybeGetDescriptorByIDUnvalidated(ctx, txn, id) + desc, err = m.storage.makeDirect(ctx).MaybeGetDescriptorByIDUnvalidated(ctx, txn, id) if err != nil { return err } @@ -918,8 +916,7 @@ func (m *Manager) resolveName( return err } var err error - direct := catkv.MakeDirect(m.storage.codec, m.storage.settings.Version.ActiveVersion(ctx)) - id, err = direct.LookupDescriptorID(ctx, txn, parentID, parentSchemaID, name) + id, err = m.storage.makeDirect(ctx).LookupDescriptorID(ctx, txn, parentID, parentSchemaID, name) return err }); err != nil { return id, err diff --git a/pkg/sql/catalog/lease/storage.go b/pkg/sql/catalog/lease/storage.go index c686177fdd1d..d78977b88630 100644 --- a/pkg/sql/catalog/lease/storage.go +++ b/pkg/sql/catalog/lease/storage.go @@ -131,9 +131,7 @@ func (s storage) acquire( expiration = minExpiration.Add(int64(time.Millisecond), 0) } - version := s.settings.Version.ActiveVersion(ctx) - direct := catkv.MakeDirect(s.codec, version) - desc, err = direct.MustGetDescriptorByID(ctx, txn, id, catalog.Any) + desc, err = s.makeDirect(ctx).MustGetDescriptorByID(ctx, txn, id, catalog.Any) if err != nil { return err } @@ -258,9 +256,7 @@ func (s storage) getForExpiration( if err != nil { return err } - version := s.settings.Version.ActiveVersion(ctx) - direct := catkv.MakeDirect(s.codec, version) - desc, err = direct.MustGetDescriptorByID(ctx, txn, id, catalog.Any) + desc, err = s.makeDirect(ctx).MustGetDescriptorByID(ctx, txn, id, catalog.Any) if err != nil { return err } @@ -274,3 +270,11 @@ func (s storage) getForExpiration( }) return desc, err } + +func (s storage) makeDirect(ctx context.Context) catkv.Direct { + return catkv.MakeDirect( + s.codec, + s.settings.Version.ActiveVersion(ctx), + catkv.DefaultDescriptorValidationModeProvider, + ) +} diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index c7f6e526a782..68d3f50a9f6f 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -29,6 +29,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/server/telemetry" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" @@ -1006,7 +1007,10 @@ func (s *Server) newConnExecutor( portals: make(map[string]PreparedPortal), } ex.extraTxnState.prepStmtsNamespaceMemAcc = ex.sessionMon.MakeBoundAccount() - ex.extraTxnState.descCollection = s.cfg.CollectionFactory.NewCollection(ctx, descs.NewTemporarySchemaProvider(sdMutIterator.sds), ex.sessionMon) + dsdp := catsessiondata.NewDescriptorSessionDataStackProvider(sdMutIterator.sds) + ex.extraTxnState.descCollection = s.cfg.CollectionFactory.NewCollection( + ctx, descs.WithDescriptorSessionDataProvider(dsdp), descs.WithMonitor(ex.sessionMon), + ) ex.extraTxnState.jobs = new(jobsCollection) ex.extraTxnState.txnRewindPos = -1 ex.extraTxnState.schemaChangeJobRecords = make(map[descpb.ID]*jobs.Record) diff --git a/pkg/sql/create_sequence.go b/pkg/sql/create_sequence.go index a22683c55b4e..df8f1c91b9b7 100644 --- a/pkg/sql/create_sequence.go +++ b/pkg/sql/create_sequence.go @@ -22,8 +22,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" @@ -334,7 +335,12 @@ func NewSequenceTableDesc( } version := settings.Version.ActiveVersion(ctx) - if err := descbuilder.ValidateSelf(&desc, version); err != nil { + var sd *sessiondata.SessionData + if p != nil { + sd = p.SessionData() + } + dvmp := catsessiondata.NewDescriptorSessionDataProvider(sd) + if err := descs.ValidateSelf(&desc, version, dvmp); err != nil { return nil, err } return &desc, nil diff --git a/pkg/sql/descriptor_mutation_test.go b/pkg/sql/descriptor_mutation_test.go index 89c8fa9cb449..8e0a77ae421d 100644 --- a/pkg/sql/descriptor_mutation_test.go +++ b/pkg/sql/descriptor_mutation_test.go @@ -18,12 +18,10 @@ import ( "testing" "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" @@ -87,7 +85,7 @@ func (mt mutationTest) makeMutationsActive(ctx context.Context) { } mt.tableDesc.Mutations = nil mt.tableDesc.Version++ - if err := descbuilder.ValidateSelf(mt.tableDesc, clusterversion.TestingClusterVersion); err != nil { + if err := desctestutils.TestingValidateSelf(mt.tableDesc); err != nil { mt.Fatal(err) } if err := mt.kvDB.Put( @@ -147,7 +145,7 @@ func (mt mutationTest) writeMutation(ctx context.Context, m descpb.DescriptorMut } mt.tableDesc.Mutations = append(mt.tableDesc.Mutations, m) mt.tableDesc.Version++ - if err := descbuilder.ValidateSelf(mt.tableDesc, clusterversion.TestingClusterVersion); err != nil { + if err := desctestutils.TestingValidateSelf(mt.tableDesc); err != nil { mt.Fatal(err) } if err := mt.kvDB.Put( @@ -450,21 +448,21 @@ func TestOperationsWithColumnMutation(t *testing.T) { // Check that a mutation can only be inserted with an explicit mutation state, and direction. tableDesc = mTest.tableDesc tableDesc.Mutations = []descpb.DescriptorMutation{{}} - if err := descbuilder.ValidateSelf(tableDesc, clusterversion.TestingClusterVersion); !testutils.IsError(err, "mutation in state UNKNOWN, direction NONE, and no column/index descriptor") { + if err := desctestutils.TestingValidateSelf(tableDesc); !testutils.IsError(err, "mutation in state UNKNOWN, direction NONE, and no column/index descriptor") { t.Fatal(err) } tableDesc.Mutations = []descpb.DescriptorMutation{{Descriptor_: &descpb.DescriptorMutation_Column{Column: &tableDesc.Columns[len(tableDesc.Columns)-1]}}} tableDesc.Columns = tableDesc.Columns[:len(tableDesc.Columns)-1] - if err := descbuilder.ValidateSelf(tableDesc, clusterversion.TestingClusterVersion); !testutils.IsError(err, `mutation in state UNKNOWN, direction NONE, col "i", id 3`) { + if err := desctestutils.TestingValidateSelf(tableDesc); !testutils.IsError(err, `mutation in state UNKNOWN, direction NONE, col "i", id 3`) { t.Fatal(err) } tableDesc.Mutations[0].State = descpb.DescriptorMutation_DELETE_ONLY - if err := descbuilder.ValidateSelf(tableDesc, clusterversion.TestingClusterVersion); !testutils.IsError(err, `mutation in state DELETE_ONLY, direction NONE, col "i", id 3`) { + if err := desctestutils.TestingValidateSelf(tableDesc); !testutils.IsError(err, `mutation in state DELETE_ONLY, direction NONE, col "i", id 3`) { t.Fatal(err) } tableDesc.Mutations[0].State = descpb.DescriptorMutation_UNKNOWN tableDesc.Mutations[0].Direction = descpb.DescriptorMutation_DROP - if err := descbuilder.ValidateSelf(tableDesc, clusterversion.TestingClusterVersion); !testutils.IsError(err, `mutation in state UNKNOWN, direction DROP, col "i", id 3`) { + if err := desctestutils.TestingValidateSelf(tableDesc); !testutils.IsError(err, `mutation in state UNKNOWN, direction DROP, col "i", id 3`) { t.Fatal(err) } } @@ -651,7 +649,7 @@ func TestOperationsWithIndexMutation(t *testing.T) { index := tableDesc.PublicNonPrimaryIndexes()[len(tableDesc.PublicNonPrimaryIndexes())-1] tableDesc.Mutations = []descpb.DescriptorMutation{{Descriptor_: &descpb.DescriptorMutation_Index{Index: index.IndexDesc()}}} tableDesc.RemovePublicNonPrimaryIndex(index.Ordinal()) - if err := descbuilder.ValidateSelf(tableDesc, clusterversion.TestingClusterVersion); !testutils.IsError(err, "mutation in state UNKNOWN, direction NONE, index foo, id 2") { + if err := desctestutils.TestingValidateSelf(tableDesc); !testutils.IsError(err, "mutation in state UNKNOWN, direction NONE, index foo, id 2") { t.Fatal(err) } } diff --git a/pkg/sql/distsql/BUILD.bazel b/pkg/sql/distsql/BUILD.bazel index 64b7a79c4f1a..493bb185184f 100644 --- a/pkg/sql/distsql/BUILD.bazel +++ b/pkg/sql/distsql/BUILD.bazel @@ -13,6 +13,7 @@ go_library( "//pkg/roachpb", "//pkg/server/telemetry", "//pkg/settings", + "//pkg/sql/catalog/catsessiondata", "//pkg/sql/catalog/descs", "//pkg/sql/colflow", "//pkg/sql/execinfra", diff --git a/pkg/sql/distsql/server.go b/pkg/sql/distsql/server.go index 2fc96ecd04ff..817a88703e88 100644 --- a/pkg/sql/distsql/server.go +++ b/pkg/sql/distsql/server.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/settings" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/colflow" "github.com/cockroachdb/cockroach/pkg/sql/execinfra" @@ -488,7 +489,10 @@ func (ds *ServerImpl) newFlowContext( // If we weren't passed a descs.Collection, then make a new one. We are // responsible for cleaning it up and releasing any accessed descriptors // on flow cleanup. - flowCtx.Descriptors = ds.CollectionFactory.NewCollection(ctx, descs.NewTemporarySchemaProvider(evalCtx.SessionDataStack), nil /* monitor */) + dsdp := catsessiondata.NewDescriptorSessionDataStackProvider(evalCtx.SessionDataStack) + flowCtx.Descriptors = ds.CollectionFactory.NewCollection( + ctx, descs.WithDescriptorSessionDataProvider(dsdp), + ) flowCtx.IsDescriptorsCleanupRequired = true flowCtx.EvalCatalogBuiltins.Init(evalCtx.Codec, evalCtx.Txn, flowCtx.Descriptors) evalCtx.CatalogBuiltins = &flowCtx.EvalCatalogBuiltins diff --git a/pkg/sql/drop_test.go b/pkg/sql/drop_test.go index 5f08dd817add..06986d383055 100644 --- a/pkg/sql/drop_test.go +++ b/pkg/sql/drop_test.go @@ -1374,12 +1374,8 @@ func dropLargeDatabaseGeneric( s, db, _ := serverutils.StartServer(t, base.TestServerArgs{UseDatabase: `test`}) defer s.Stopper().Stop(ctx) sqlDB := sqlutils.MakeSQLRunner(db) - sqlDB.Exec(t, ` -SET CLUSTER SETTING sql.catalog.descs.validate_on_write.enabled=no; -`) - sqlDB.Exec(t, ` -CREATE DATABASE largedb; -`) + sqlDB.Exec(t, `SET descriptor_validation = read_only`) + sqlDB.Exec(t, `CREATE DATABASE largedb`) stmts, err := sqltestutils.GenerateViewBasedGraphSchema(workloadParams) require.NoError(t, err) for _, stmt := range stmts { diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index b6a5c42aad6b..5d2e54161274 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -3182,6 +3182,12 @@ func (m *sessionDataMutator) SetUseNewSchemaChanger(val sessiondatapb.NewSchemaC m.data.NewSchemaChangerMode = val } +func (m *sessionDataMutator) SetDescriptorValidationMode( + val sessiondatapb.DescriptorValidationMode, +) { + m.data.DescriptorValidationMode = val +} + func (m *sessionDataMutator) SetQualityOfService(val sessiondatapb.QoSLevel) { m.data.DefaultTxnQualityOfService = val.Validate() } diff --git a/pkg/sql/internal.go b/pkg/sql/internal.go index d60528929c19..86c96d03518d 100644 --- a/pkg/sql/internal.go +++ b/pkg/sql/internal.go @@ -24,6 +24,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/catalog" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" @@ -813,9 +814,8 @@ func (ie *InternalExecutor) execInternal( // If the caller has injected a mapping to temp schemas, install it, and // leave it installed for the rest of the transaction. if ie.extraTxnState != nil && sd.DatabaseIDToTempSchemaID != nil { - ie.extraTxnState.descCollection.SetTemporaryDescriptors( - descs.NewTemporarySchemaProvider(sessiondata.NewStack(sd)), - ) + p := catsessiondata.NewDescriptorSessionDataStackProvider(sessiondata.NewStack(sd)) + ie.extraTxnState.descCollection.SetDescriptorSessionDataProvider(p) } // The returned span is finished by this function in all error paths, but if @@ -1408,10 +1408,7 @@ func (ief *InternalExecutorFactory) DescsTxnWithExecutor( var deletedDescs catalog.DescriptorIDSet if err := run(ctx, func(ctx context.Context, txn *kv.Txn) (err error) { withNewVersion, deletedDescs = nil, catalog.DescriptorIDSet{} - descsCol := cf.NewCollection( - ctx, nil, /* temporarySchemaProvider */ - ief.monitor, - ) + descsCol := cf.NewCollection(ctx, descs.WithMonitor(ief.monitor)) defer descsCol.ReleaseAll(ctx) ie, commitTxnFn := ief.newInternalExecutorWithTxn(sd, &cf.GetClusterSettings().SV, txn, descsCol) if err := f(ctx, txn, descsCol, ie); err != nil { diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema index 0c8ca1ddb36a..d06779b5c1f8 100644 --- a/pkg/sql/logictest/testdata/logic_test/information_schema +++ b/pkg/sql/logictest/testdata/logic_test/information_schema @@ -4701,6 +4701,7 @@ default_transaction_quality_of_service regular default_transaction_read_only off default_transaction_use_follower_reads off default_with_oids off +descriptor_validation on disable_hoist_projection_in_join_limitation off disable_partially_distributed_plans off disable_plan_gists off diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog index c08ffcd3179a..3623c4b4937c 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -4180,6 +4180,7 @@ default_transaction_quality_of_service regular NULL default_transaction_read_only off NULL NULL NULL string default_transaction_use_follower_reads off NULL NULL NULL string default_with_oids off NULL NULL NULL string +descriptor_validation on NULL NULL NULL string disable_hoist_projection_in_join_limitation off NULL NULL NULL string disable_partially_distributed_plans off NULL NULL NULL string disable_plan_gists off NULL NULL NULL string @@ -4200,7 +4201,7 @@ enforce_home_region off NULL escape_string_warning on NULL NULL NULL string expect_and_ignore_not_visible_columns_in_copy off NULL NULL NULL string experimental_distsql_planning off NULL NULL NULL string -experimental_enable_auto_rehoming off NULL NULL NULL string +experimental_enable_auto_rehoming off NULL NULL NULL string experimental_enable_implicit_column_partitioning off NULL NULL NULL string experimental_enable_temp_tables off NULL NULL NULL string experimental_enable_unique_without_index_constraints on NULL NULL NULL string @@ -4317,6 +4318,7 @@ default_transaction_quality_of_service regular NULL default_transaction_read_only off NULL user NULL off off default_transaction_use_follower_reads off NULL user NULL off off default_with_oids off NULL user NULL off off +descriptor_validation on NULL user NULL on on disable_hoist_projection_in_join_limitation off NULL user NULL off off disable_partially_distributed_plans off NULL user NULL off off disable_plan_gists off NULL user NULL off off @@ -4450,6 +4452,7 @@ default_transaction_quality_of_service NULL NULL NULL default_transaction_read_only NULL NULL NULL NULL NULL default_transaction_use_follower_reads NULL NULL NULL NULL NULL default_with_oids NULL NULL NULL NULL NULL +descriptor_validation NULL NULL NULL NULL NULL disable_hoist_projection_in_join_limitation NULL NULL NULL NULL NULL disable_partially_distributed_plans NULL NULL NULL NULL NULL disable_plan_gists NULL NULL NULL NULL NULL diff --git a/pkg/sql/logictest/testdata/logic_test/schema_repair b/pkg/sql/logictest/testdata/logic_test/schema_repair index 2f9ea8013805..eb6469f1d128 100644 --- a/pkg/sql/logictest/testdata/logic_test/schema_repair +++ b/pkg/sql/logictest/testdata/logic_test/schema_repair @@ -95,7 +95,6 @@ SELECT CAST(replace('$json_corrupt','"name": "corruptdesc",', '') AS JSONB) statement ok SELECT * FROM crdb_internal.unsafe_upsert_descriptor($corrupt_id, crdb_internal.json_to_pb( 'cockroach.sql.sqlbase.Descriptor','$json_t_corrupt'), true) - query I SELECT count(*) FROM crdb_internal.lost_descriptors_with_data WHERE descid = $t_id; ---- @@ -263,7 +262,7 @@ statement error pgcode XXUUU referenced descriptor not found ALTER TYPE corrupt_backref_typ DROP VALUE 'b' # This is required to pass the validation tests when the logic test completes. -subtest cleanup +subtest cleanup_corrupt_backref query TB SELECT @@ -297,3 +296,66 @@ _corrupt_backref_typ true true corrupt_backref_fk true true corrupt_backref_typ true true corrupt_backref_view true true + +# Check that `SET descriptor_validation = off ` disables validation. +# We do so by corrupting a table descriptor and assert on the successful +# execution of DDL statements so as to not acquire a lease on the corrupt +# descriptor, which would complicate things. +subtest disable_validation + +statement ok +CREATE TABLE kv (k INT PRIMARY KEY, v STRING); + +statement ok +SELECT + crdb_internal.unsafe_upsert_descriptor( + d.id, + crdb_internal.json_to_pb( + 'cockroach.sql.sqlbase.Descriptor', + json_set( + crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', d.descriptor), + ARRAY['table', 'nextColumnId'], + '1'::JSONB + ) + ), + true + ) +FROM + system.descriptor AS d INNER JOIN system.namespace AS ns ON d.id = ns.id +WHERE + name = 'kv' + +statement error pgcode XX000 internal error: relation "kv" \(\d+\): column "k" invalid ID \(1\) >= next column ID \(1\) +ALTER TABLE kv RENAME TO kv + +statement ok +SET descriptor_validation = off + +statement ok +ALTER TABLE kv RENAME TO kv + +statement ok +SET descriptor_validation = on + +# Undo the corruption prior to dropping the table, to clean up. +statement ok +SELECT + crdb_internal.unsafe_upsert_descriptor( + d.id, + crdb_internal.json_to_pb( + 'cockroach.sql.sqlbase.Descriptor', + json_set( + crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', d.descriptor), + ARRAY['table', 'nextColumnId'], + '3'::JSONB + ) + ), + true + ) +FROM + system.descriptor AS d INNER JOIN system.namespace AS ns ON d.id = ns.id +WHERE + name = 'kv' + +statement ok +DROP TABLE kv diff --git a/pkg/sql/logictest/testdata/logic_test/show_source b/pkg/sql/logictest/testdata/logic_test/show_source index 520a278dc877..c483a7c56c2a 100644 --- a/pkg/sql/logictest/testdata/logic_test/show_source +++ b/pkg/sql/logictest/testdata/logic_test/show_source @@ -47,6 +47,7 @@ default_transaction_quality_of_service regular default_transaction_read_only off default_transaction_use_follower_reads off default_with_oids off +descriptor_validation on disable_hoist_projection_in_join_limitation off disable_partially_distributed_plans off disable_plan_gists off diff --git a/pkg/sql/planner.go b/pkg/sql/planner.go index 6c05cfa444c2..41c2d631e38f 100644 --- a/pkg/sql/planner.go +++ b/pkg/sql/planner.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/spanconfig" "github.com/cockroachdb/cockroach/pkg/sql/catalog" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" @@ -328,7 +329,10 @@ func newInternalPlanner( sds := sessiondata.NewStack(sd) if params.collection == nil { - params.collection = execCfg.CollectionFactory.NewCollection(ctx, descs.NewTemporarySchemaProvider(sds), nil /* monitor */) + dsdp := catsessiondata.NewDescriptorSessionDataStackProvider(sds) + params.collection = execCfg.CollectionFactory.NewCollection( + ctx, descs.WithDescriptorSessionDataProvider(dsdp), + ) } var ts time.Time diff --git a/pkg/sql/row/BUILD.bazel b/pkg/sql/row/BUILD.bazel index 77a21423d4e8..15c884e946a4 100644 --- a/pkg/sql/row/BUILD.bazel +++ b/pkg/sql/row/BUILD.bazel @@ -39,6 +39,7 @@ go_library( "//pkg/sql/catalog", "//pkg/sql/catalog/catalogkeys", "//pkg/sql/catalog/catpb", + "//pkg/sql/catalog/catsessiondata", "//pkg/sql/catalog/colinfo", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/descs", diff --git a/pkg/sql/row/row_converter.go b/pkg/sql/row/row_converter.go index 036441895962..ca35ad3381df 100644 --- a/pkg/sql/row/row_converter.go +++ b/pkg/sql/row/row_converter.go @@ -16,6 +16,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" @@ -267,7 +268,8 @@ func (c *DatumRowConverter) getSequenceAnnotation( // TODO(postamar): give the eval.Context a useful interface // instead of cobbling a descs.Collection in this way. cf := descs.NewBareBonesCollectionFactory(evalCtx.Settings, evalCtx.Codec) - descsCol := cf.NewCollection(ctx, descs.NewTemporarySchemaProvider(evalCtx.SessionDataStack), nil /* monitor */) + dsdp := catsessiondata.NewDescriptorSessionDataStackProvider(evalCtx.SessionDataStack) + descsCol := cf.NewCollection(ctx, descs.WithDescriptorSessionDataProvider(dsdp)) err := c.db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error { seqNameToMetadata = make(map[string]*SequenceMetadata) seqIDToMetadata = make(map[descpb.ID]*SequenceMetadata) diff --git a/pkg/sql/sessiondatapb/local_only_session_data.go b/pkg/sql/sessiondatapb/local_only_session_data.go index b4db2b33a52e..c23e91aec573 100644 --- a/pkg/sql/sessiondatapb/local_only_session_data.go +++ b/pkg/sql/sessiondatapb/local_only_session_data.go @@ -229,6 +229,48 @@ func NewSchemaChangerModeFromString(val string) (_ NewSchemaChangerMode, ok bool } } +// DescriptorValidationMode controls if and when descriptors are validated. +type DescriptorValidationMode int64 + +const ( + // DescriptorValidationOn means that we always validate descriptors, + // both when reading from storage and when writing to storage. + DescriptorValidationOn DescriptorValidationMode = iota + // DescriptorValidationOff means that we never validate descriptors. + DescriptorValidationOff + // DescriptorValidationReadOnly means that we validate descriptors when + // reading from storage, but not when writing to storage. + DescriptorValidationReadOnly +) + +func (m DescriptorValidationMode) String() string { + switch m { + case DescriptorValidationOn: + return "on" + case DescriptorValidationOff: + return "off" + case DescriptorValidationReadOnly: + return "read_only" + default: + return fmt.Sprintf("invalid (%d)", m) + } +} + +// DescriptorValidationModeFromString converts a string into a +// DescriptorValidationMode. +func DescriptorValidationModeFromString(val string) (_ DescriptorValidationMode, ok bool) { + switch strings.ToUpper(val) { + case "ON": + return DescriptorValidationOn, true + case "OFF": + return DescriptorValidationOff, true + case "READ_ONLY": + return DescriptorValidationReadOnly, true + default: + return 0, false + } +} + // QoSLevel controls the level of admission control to use for new SQL requests. type QoSLevel admissionpb.WorkPriority diff --git a/pkg/sql/sessiondatapb/local_only_session_data.proto b/pkg/sql/sessiondatapb/local_only_session_data.proto index b87184139e5f..7c9ff595fdcb 100644 --- a/pkg/sql/sessiondatapb/local_only_session_data.proto +++ b/pkg/sql/sessiondatapb/local_only_session_data.proto @@ -298,10 +298,12 @@ message LocalOnlySessionData { // TransactionSessionTimeout is the duration a transaction is permitted to // run before the transaction is canceled. If set to 0, there is no timeout. int64 transaction_timeout = 81 [(gogoproto.casttype) = "time.Duration"]; - // SystemIdentityProto is the original name of the client presented to pgwire // before it was mapped to a SQL identifier. string system_identity_proto = 82 [(gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/security/username.SQLUsernameProto"]; + // DescriptorValidationMode indicates whether to validate the descriptors at + // read and write time, at read time only, or never. + int64 descriptor_validation_mode = 83 [(gogoproto.casttype) = "DescriptorValidationMode"]; /////////////////////////////////////////////////////////////////////////// // WARNING: consider whether a session parameter you're adding needs to // diff --git a/pkg/sql/table.go b/pkg/sql/table.go index a7dc2a5bc006..8b58f306c505 100644 --- a/pkg/sql/table.go +++ b/pkg/sql/table.go @@ -20,8 +20,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/catalog" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors" @@ -304,7 +305,8 @@ func (p *planner) writeTableDescToBatch( } version := p.ExecCfg().Settings.Version.ActiveVersion(ctx) - if err := descbuilder.ValidateSelf(tableDesc, version); err != nil { + dvmp := catsessiondata.NewDescriptorSessionDataProvider(p.SessionData()) + if err := descs.ValidateSelf(tableDesc, version, dvmp); err != nil { return errors.NewAssertionErrorWithWrappedErrf(err, "table descriptor is not valid\n%v\n", tableDesc) } diff --git a/pkg/sql/table_test.go b/pkg/sql/table_test.go index 338e386d2f56..f38d375aa3e3 100644 --- a/pkg/sql/table_test.go +++ b/pkg/sql/table_test.go @@ -17,14 +17,12 @@ import ( "reflect" "testing" - "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/jobs" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" @@ -418,7 +416,7 @@ func TestPrimaryKeyUnspecified(t *testing.T) { } desc.SetPrimaryIndex(descpb.IndexDescriptor{}) - err = descbuilder.ValidateSelf(desc, clusterversion.TestingClusterVersion) + err = desctestutils.TestingValidateSelf(desc) if !testutils.IsError(err, tabledesc.ErrMissingPrimaryKey.Error()) { t.Fatalf("unexpected error: %v", err) } diff --git a/pkg/sql/temporary_schema.go b/pkg/sql/temporary_schema.go index c80cb3332e1f..20467d85d6cc 100644 --- a/pkg/sql/temporary_schema.go +++ b/pkg/sql/temporary_schema.go @@ -516,7 +516,7 @@ func (c *TemporaryObjectCleaner) doTemporaryObjectCleanup( waitTimeForCreation := TempObjectWaitInterval.Get(&c.settings.SV) // Build a set of all databases with temporary objects. var allDbDescs []catalog.DatabaseDescriptor - descsCol := c.collectionFactory.NewCollection(ctx, nil /* TemporarySchemaProvider */, nil /* monitor */) + descsCol := c.collectionFactory.NewCollection(ctx) if err := retryFunc(ctx, func() error { var err error allDbDescs, err = descsCol.GetAllDatabaseDescriptors(ctx, txn) diff --git a/pkg/sql/tests/BUILD.bazel b/pkg/sql/tests/BUILD.bazel index 5cf34956a2d2..f59466b6f0c7 100644 --- a/pkg/sql/tests/BUILD.bazel +++ b/pkg/sql/tests/BUILD.bazel @@ -60,7 +60,6 @@ go_test( "//pkg/ccl", "//pkg/ccl/utilccl", "//pkg/cloud/impl:cloudimpl", - "//pkg/clusterversion", "//pkg/config/zonepb", "//pkg/internal/rsg", "//pkg/internal/sqlsmith", @@ -80,7 +79,6 @@ go_test( "//pkg/sql/catalog", "//pkg/sql/catalog/bootstrap", "//pkg/sql/catalog/catpb", - "//pkg/sql/catalog/descbuilder", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/descs", "//pkg/sql/catalog/desctestutils", diff --git a/pkg/sql/tests/repair_test.go b/pkg/sql/tests/repair_test.go index 4996b56b2924..379c0a815c40 100644 --- a/pkg/sql/tests/repair_test.go +++ b/pkg/sql/tests/repair_test.go @@ -20,7 +20,6 @@ import ( "github.com/cockroachdb/cockroach-go/v2/crdb" "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/sql" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" @@ -502,11 +501,11 @@ SELECT crdb_internal.unsafe_delete_namespace_entry("parentID", 0, 'foo', id) defer cleanup() tdb := sqlutils.MakeSQLRunner(db) - descs.ValidateOnWriteEnabled.Override(ctx, &s.ClusterSettings().SV, false) + tdb.Exec(t, `SET descriptor_validation = read_only`) for _, op := range tc.before { tdb.Exec(t, op) } - descs.ValidateOnWriteEnabled.Override(ctx, &s.ClusterSettings().SV, true) + tdb.Exec(t, `SET descriptor_validation = on`) _, err := db.Exec(tc.op) if tc.expErrRE == "" { require.NoError(t, err) @@ -869,8 +868,7 @@ func TestDescriptorRepairIdGeneration(t *testing.T) { } }` - // Required so test doesn't fail due to namespace validation failures. - descs.ValidateOnWriteEnabled.Override(ctx, &s.ClusterSettings().SV, false) + tdb.Exec(t, `SET descriptor_validation = read_only`) // Inserting a descriptor with an ID too high should fail. tdb.ExpectErr(t, "descriptor ID 1234 must be less than the descriptor ID sequence value", q, false /* force */, d) diff --git a/pkg/sql/tests/system_table_test.go b/pkg/sql/tests/system_table_test.go index b9774d433666..449f65942a1b 100644 --- a/pkg/sql/tests/system_table_test.go +++ b/pkg/sql/tests/system_table_test.go @@ -18,7 +18,6 @@ import ( "testing" "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" @@ -27,9 +26,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" @@ -218,7 +217,7 @@ func TestSystemTableLiterals(t *testing.T) { if err != nil { t.Fatalf("test: %+v, err: %v", test, err) } - require.NoError(t, descbuilder.ValidateSelf(gen, clusterversion.TestingClusterVersion)) + require.NoError(t, desctestutils.TestingValidateSelf(gen)) if desc.TableDesc().Equal(gen.TableDesc()) { return diff --git a/pkg/sql/vars.go b/pkg/sql/vars.go index bc13a6efa36d..cdbf85836ca3 100644 --- a/pkg/sql/vars.go +++ b/pkg/sql/vars.go @@ -1705,6 +1705,25 @@ var varGen = map[string]sessionVar{ }, }, + `descriptor_validation`: { + GetStringVal: makePostgresBoolGetStringValFn(`descriptor_validation`), + Set: func(_ context.Context, m sessionDataMutator, s string) error { + mode, ok := sessiondatapb.DescriptorValidationModeFromString(s) + if !ok { + return newVarValueError(`descriptor_validation`, s, + "off", "on", "read_only") + } + m.SetDescriptorValidationMode(mode) + return nil + }, + Get: func(evalCtx *extendedEvalContext, _ *kv.Txn) (string, error) { + return evalCtx.SessionData().DescriptorValidationMode.String(), nil + }, + GlobalDefault: func(sv *settings.Values) string { + return sessiondatapb.DescriptorValidationOn.String() + }, + }, + `enable_experimental_stream_replication`: { GetStringVal: makePostgresBoolGetStringValFn(`enable_experimental_stream_replication`), Set: func(_ context.Context, m sessionDataMutator, s string) error { diff --git a/pkg/sql/virtual_schema.go b/pkg/sql/virtual_schema.go index 43481902c6cd..dec3b3fd1c0f 100644 --- a/pkg/sql/virtual_schema.go +++ b/pkg/sql/virtual_schema.go @@ -19,9 +19,10 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/catalog" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" @@ -747,7 +748,8 @@ func NewVirtualSchemaHolder( } td := tabledesc.NewBuilder(&tableDesc).BuildImmutableTable() version := st.Version.ActiveVersionOrEmpty(ctx) - if err := descbuilder.ValidateSelf(td, version); err != nil { + dvmp := catsessiondata.NewDescriptorSessionDataProvider(nil /* sd */) + if err := descs.ValidateSelf(td, version, dvmp); err != nil { return nil, errors.NewAssertionErrorWithWrappedErrf(err, "failed to validate virtual table %s: programmer error", errors.Safe(td.GetName())) }