From a15b49225f491480045800a99d4c6c67e3577a74 Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Fri, 21 Oct 2022 17:07:06 -0400 Subject: [PATCH] sql,descs: add & adopt descriptor_validation session var This commit removes the `sql.catalog.descs.validate_on_write.enabled` cluster setting and replaces it with the `descriptor_validation` session variable. The default value is 'on' which indicates that catalog descriptors are to be validated when both read from- and written to the system.descriptor table. Other possible values are 'off' which disables validation entirely and 'read_only' which disables it for writes. Informs #50651. Release note (sql change): added a new 'descriptor_validation' session variable which can be set to 'read_only' or 'off' to disable descriptor validation, which may be useful when mitigating or recovering from catalog corruption. --- pkg/ccl/changefeedccl/avro_test.go | 2 +- .../cdcevent/rowfetcher_cache.go | 2 +- pkg/ccl/changefeedccl/changefeed_stmt.go | 2 +- pkg/ccl/partitionccl/partition_test.go | 4 +- pkg/jobs/registry_test.go | 4 +- pkg/settings/registry.go | 1 + pkg/sql/alter_primary_key.go | 2 +- pkg/sql/catalog/descbuilder/BUILD.bazel | 2 + pkg/sql/catalog/descbuilder/desc_builder.go | 12 +- pkg/sql/catalog/descs/BUILD.bazel | 2 +- pkg/sql/catalog/descs/collection.go | 37 +++---- pkg/sql/catalog/descs/collection_test.go | 6 +- pkg/sql/catalog/descs/descriptor.go | 8 +- pkg/sql/catalog/descs/factory.go | 4 +- .../catalog/descs/temporary_descriptors.go | 104 ++++++++---------- pkg/sql/catalog/descs/validate.go | 21 +++- .../desctestutils/descriptor_test_utils.go | 6 +- pkg/sql/catalog/internal/catkv/BUILD.bazel | 2 + pkg/sql/catalog/internal/catkv/direct.go | 17 ++- .../catalog/internal/catkv/stored_catalog.go | 38 +++++-- pkg/sql/catalog/lease/helpers_test.go | 2 +- pkg/sql/catalog/lease/lease.go | 4 +- pkg/sql/catalog/lease/storage.go | 4 +- pkg/sql/conn_executor.go | 2 +- pkg/sql/create_sequence.go | 6 +- pkg/sql/descriptor_mutation_test.go | 30 +++-- pkg/sql/distsql/server.go | 2 +- pkg/sql/drop_test.go | 8 +- pkg/sql/exec_util.go | 6 + pkg/sql/internal.go | 9 +- .../testdata/logic_test/information_schema | 1 + .../logictest/testdata/logic_test/pg_catalog | 5 +- .../testdata/logic_test/schema_repair | 66 ++++++++++- .../logictest/testdata/logic_test/show_source | 1 + pkg/sql/planner.go | 2 +- pkg/sql/row/row_converter.go | 2 +- .../sessiondatapb/local_only_session_data.go | 42 +++++++ .../local_only_session_data.proto | 4 +- pkg/sql/table.go | 2 +- pkg/sql/table_test.go | 2 +- pkg/sql/temporary_schema.go | 2 +- pkg/sql/tests/repair_test.go | 8 +- pkg/sql/tests/system_table_test.go | 2 +- pkg/sql/vars.go | 19 ++++ pkg/sql/virtual_schema.go | 2 +- 45 files changed, 351 insertions(+), 158 deletions(-) diff --git a/pkg/ccl/changefeedccl/avro_test.go b/pkg/ccl/changefeedccl/avro_test.go index f7934f36a5de..858ebd9023c4 100644 --- a/pkg/ccl/changefeedccl/avro_test.go +++ b/pkg/ccl/changefeedccl/avro_test.go @@ -88,7 +88,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, descbuilder.ValidateSelf(mutDesc, clusterversion.TestingClusterVersion, nil /* sd */) } 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 50636c18d448..0d990569c94a 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, nil /* sds */, nil /* monitor */), 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 fd1edc40a9b1..92cb7b75c447 100644 --- a/pkg/ccl/changefeedccl/changefeed_stmt.go +++ b/pkg/ccl/changefeedccl/changefeed_stmt.go @@ -1166,7 +1166,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, nil /* sds */, nil /* monitor */) dbDesc, err := col.Direct().MustGetDatabaseDescByID(ctx, txn, desc.GetParentID()) if err != nil { return tree.TableName{}, err diff --git a/pkg/ccl/partitionccl/partition_test.go b/pkg/ccl/partitionccl/partition_test.go index fa0161c8b5fa..23ed67e83352 100644 --- a/pkg/ccl/partitionccl/partition_test.go +++ b/pkg/ccl/partitionccl/partition_test.go @@ -155,7 +155,9 @@ func (pt *partitioningTest) parse() error { return err } pt.parsed.tableDesc = mutDesc - if err := descbuilder.ValidateSelf(pt.parsed.tableDesc, clusterversion.TestingClusterVersion); err != nil { + if err := descbuilder.ValidateSelf( + pt.parsed.tableDesc, clusterversion.TestingClusterVersion, nil, /* sd */ + ); err != nil { return err } } diff --git a/pkg/jobs/registry_test.go b/pkg/jobs/registry_test.go index acfab16efd1e..b9ea40b90a3d 100644 --- a/pkg/jobs/registry_test.go +++ b/pkg/jobs/registry_test.go @@ -82,7 +82,9 @@ func writeMutation( ) { tableDesc.Mutations = append(tableDesc.Mutations, m) tableDesc.Version++ - if err := descbuilder.ValidateSelf(tableDesc, clusterversion.TestingClusterVersion); err != nil { + if err := descbuilder.ValidateSelf( + tableDesc, clusterversion.TestingClusterVersion, nil, /* sd */ + ); err != nil { t.Fatal(err) } if err := kvDB.Put( 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/alter_primary_key.go b/pkg/sql/alter_primary_key.go index 4353362f5800..828da65032c9 100644 --- a/pkg/sql/alter_primary_key.go +++ b/pkg/sql/alter_primary_key.go @@ -544,7 +544,7 @@ func (p *planner) AlterPrimaryKey( } tableDesc.AddPrimaryKeySwapMutation(swapArgs) - if err := descbuilder.ValidateSelf(tableDesc, version); err != nil { + if err := descbuilder.ValidateSelf(tableDesc, version, p.SessionData()); err != nil { return err } diff --git a/pkg/sql/catalog/descbuilder/BUILD.bazel b/pkg/sql/catalog/descbuilder/BUILD.bazel index bbfe22ad3dab..932171d6393d 100644 --- a/pkg/sql/catalog/descbuilder/BUILD.bazel +++ b/pkg/sql/catalog/descbuilder/BUILD.bazel @@ -17,6 +17,8 @@ go_library( "//pkg/sql/catalog/schemadesc", "//pkg/sql/catalog/tabledesc", "//pkg/sql/catalog/typedesc", + "//pkg/sql/sessiondata", + "//pkg/sql/sessiondatapb", "//pkg/util/hlc", "//pkg/util/protoutil", "@com_github_cockroachdb_errors//:errors", diff --git a/pkg/sql/catalog/descbuilder/desc_builder.go b/pkg/sql/catalog/descbuilder/desc_builder.go index 028a93d3bae2..364145a0a2f1 100644 --- a/pkg/sql/catalog/descbuilder/desc_builder.go +++ b/pkg/sql/catalog/descbuilder/desc_builder.go @@ -21,6 +21,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/errors" @@ -174,6 +176,14 @@ func BuildMutable( } // ValidateSelf validates that the descriptor is internally consistent. -func ValidateSelf(desc catalog.Descriptor, version clusterversion.ClusterVersion) error { +// +// When the optional session data is provided and the descriptor validation mode +// session var is set to 'off', no validation is performed. +func ValidateSelf( + desc catalog.Descriptor, version clusterversion.ClusterVersion, sd *sessiondata.SessionData, +) error { + if sd != nil && sd.DescriptorValidationMode == sessiondatapb.DescriptorValidationOff { + return nil + } return validate.Self(version, desc) } diff --git a/pkg/sql/catalog/descs/BUILD.bazel b/pkg/sql/catalog/descs/BUILD.bazel index c22ade167025..12c5711a3153 100644 --- a/pkg/sql/catalog/descs/BUILD.bazel +++ b/pkg/sql/catalog/descs/BUILD.bazel @@ -34,7 +34,6 @@ go_library( "//pkg/keys", "//pkg/kv", "//pkg/roachpb", - "//pkg/settings", "//pkg/settings/cluster", "//pkg/spanconfig", "//pkg/sql/catalog", @@ -56,6 +55,7 @@ go_library( "//pkg/sql/sem/catconstants", "//pkg/sql/sem/tree", "//pkg/sql/sessiondata", + "//pkg/sql/sessiondatapb", "//pkg/sql/sqlerrors", "//pkg/sql/sqlliveness", "//pkg/sql/sqlutil", diff --git a/pkg/sql/catalog/descs/collection.go b/pkg/sql/catalog/descs/collection.go index 65e6aee87f11..3709b3d9cf05 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" @@ -29,6 +28,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/sqlliveness" "github.com/cockroachdb/cockroach/pkg/sql/sqlutil" @@ -48,7 +48,7 @@ func newCollection( hydrated *hydrateddesc.Cache, systemDatabase *catkv.SystemDatabaseCache, virtualSchemas catalog.VirtualSchemas, - temporarySchemaProvider TemporarySchemaProvider, + sds *sessiondata.Stack, monitor *mon.BytesMonitor, ) *Collection { v := settings.Version.ActiveVersion(ctx) @@ -60,8 +60,8 @@ func newCollection( virtual: makeVirtualDescriptors(virtualSchemas), leased: makeLeasedDescriptors(leaseMgr), uncommitted: makeUncommittedDescriptors(monitor), - stored: catkv.MakeStoredCatalog(cr, monitor), - temporary: makeTemporaryDescriptors(settings, codec, temporarySchemaProvider), + stored: catkv.MakeStoredCatalog(cr, sds, monitor), + sds: sds, } } @@ -111,8 +111,8 @@ type Collection struct { // non-public schema elements during a schema change). synthetic syntheticDescriptors - // temporary contains logic to access temporary schema descriptors. - temporary temporaryDescriptors + // sds is used to access temporary schema descriptors and session vars. + sds *sessiondata.Stack // hydrated is node-level cache of table descriptors which utilize // user-defined types. @@ -255,22 +255,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.validateOnWrite() { if err := validate.Self(tc.version, desc); err != nil { return err } @@ -588,11 +579,13 @@ 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) +// SetSessionDataStack sets a session data stack for this Collection. +// This is used for looking up: +// - temporary descriptors, +// - collection-specific session data variables. +func (tc *Collection) SetSessionDataStack(sds *sessiondata.Stack) { + tc.sds = sds + tc.stored.SetSessionDataStack(sds) } // Direct exports the catkv.Direct interface. @@ -600,7 +593,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.sds, tc.version) } // 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..8b0c95fe7a0c 100644 --- a/pkg/sql/catalog/descs/collection_test.go +++ b/pkg/sql/catalog/descs/collection_test.go @@ -71,7 +71,7 @@ func TestCollectionWriteDescToBatch(t *testing.T) { db := s0.DB() descriptors := s0.ExecutorConfig().(sql.ExecutorConfig).CollectionFactory. - NewCollection(ctx, nil /* TemporarySchemaProvider */, nil /* Monitor */) + NewCollection(ctx, nil /* sds */, nil /* Monitor */) // Note this transaction abuses the mechanisms normally required for updating // tables and is just for testing what this test intends to exercise. @@ -654,7 +654,7 @@ func TestCollectionProperlyUsesMemoryMonitoring(t *testing.T) { // Create a `Collection` with monitor hooked up. col := tc.Server(0).ExecutorConfig().(sql.ExecutorConfig).CollectionFactory. - NewCollection(ctx, nil /* temporarySchemaProvider */, monitor) + NewCollection(ctx, nil /* sds */, monitor) require.Equal(t, int64(0), monitor.AllocBytes()) // Read all the descriptors into `col` and assert this read will finish without error. @@ -673,7 +673,7 @@ func TestCollectionProperlyUsesMemoryMonitoring(t *testing.T) { // 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) + NewCollection(ctx, nil /* sds */, 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..eaec5125218d 100644 --- a/pkg/sql/catalog/descs/descriptor.go +++ b/pkg/sql/catalog/descs/descriptor.go @@ -23,6 +23,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" ) @@ -245,7 +246,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.temporary().getSchemaByID(id) if td == nil { return nil, catalog.NoValidation, nil } @@ -434,7 +435,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.temporary().getSchemaByName(parentID, name) if td != nil { return haltLookups, td.GetID(), nil } @@ -551,6 +552,9 @@ func (tc *Collection) finalizeDescriptors( } } // Ensure that all descriptors are sufficiently validated. + if tc.stored.ValidationMode() == sessiondatapb.DescriptorValidationOff { + 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..097ad01bdcd1 100644 --- a/pkg/sql/catalog/descs/factory.go +++ b/pkg/sql/catalog/descs/factory.go @@ -118,7 +118,7 @@ func NewBareBonesCollectionFactory( // NewCollection constructs a new Collection. func (cf *CollectionFactory) NewCollection( - ctx context.Context, temporarySchemaProvider TemporarySchemaProvider, monitor *mon.BytesMonitor, + ctx context.Context, sds *sessiondata.Stack, monitor *mon.BytesMonitor, ) *Collection { if monitor == nil { // If an upstream monitor is not provided, the default, unlimited monitor will be used. @@ -126,5 +126,5 @@ func (cf *CollectionFactory) NewCollection( monitor = cf.defaultMonitor } return newCollection(ctx, cf.leaseMgr, cf.settings, cf.codec, cf.hydrated, cf.systemDatabase, - cf.virtualSchemas, temporarySchemaProvider, monitor) + cf.virtualSchemas, sds, monitor) } diff --git a/pkg/sql/catalog/descs/temporary_descriptors.go b/pkg/sql/catalog/descs/temporary_descriptors.go index 8e91b6851922..07cfebb384a0 100644 --- a/pkg/sql/catalog/descs/temporary_descriptors.go +++ b/pkg/sql/catalog/descs/temporary_descriptors.go @@ -11,10 +11,6 @@ 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" @@ -23,54 +19,43 @@ import ( ) type temporaryDescriptors struct { - settings *cluster.Settings - codec keys.SQLCodec - tsp TemporarySchemaProvider + sds *sessiondata.Stack } -func makeTemporaryDescriptors( - settings *cluster.Settings, codec keys.SQLCodec, temporarySchemaProvider TemporarySchemaProvider, -) temporaryDescriptors { +func (tc *Collection) temporary() temporaryDescriptors { return temporaryDescriptors{ - settings: settings, - codec: codec, - tsp: temporarySchemaProvider, + sds: tc.sds, } } -// 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() +// getTemporarySchemaName delegates to GetTemporarySchemaName for the top of the +// session data stack. +func (td temporaryDescriptors) getTemporarySchemaName() string { + if td.sds == nil { + return "" + } + return td.sds.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)) +// getTemporarySchemaIDForDB delegates to GetTemporarySchemaIDForDB for the top +// of the session data stack. +func (td temporaryDescriptors) getTemporarySchemaIDForDB(id descpb.ID) (descpb.ID, bool) { + if td.sds == nil { + return descpb.InvalidID, false + } + ret, found := td.sds.Top().GetTemporarySchemaIDForDB(uint32(id)) return descpb.ID(ret), found } -// MaybeGetDatabaseForTemporarySchemaID implements the TemporarySchemaProvider interface. -func (impl *temporarySchemaProviderImpl) MaybeGetDatabaseForTemporarySchemaID( +// maybeGetDatabaseForTemporarySchemaID deletages to +// MaybeGetDatabaseForTemporarySchemaID for the top of the session data stack. +func (td temporaryDescriptors) maybeGetDatabaseForTemporarySchemaID( id descpb.ID, ) (descpb.ID, bool) { - ret, found := (*sessiondata.Stack)(impl).Top().MaybeGetDatabaseForTemporarySchemaID(uint32(id)) + if td.sds == nil { + return descpb.InvalidID, false + } + ret, found := td.sds.Top().MaybeGetDatabaseForTemporarySchemaID(uint32(id)) return descpb.ID(ret), found } @@ -80,37 +65,34 @@ func (impl *temporarySchemaProviderImpl) MaybeGetDatabaseForTemporarySchemaID( // 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 (td temporaryDescriptors) getSchemaByName( + 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 td.sds == nil { + return false, nil } - return false, nil + if schemaName != catconstants.PgTempSchemaName && schemaName != td.getTemporarySchemaName() { + return false, nil + } + schemaID, found := td.getTemporarySchemaIDForDB(dbID) + if !found { + return true, nil + } + return true, schemadesc.NewTemporarySchema( + td.getTemporarySchemaName(), + schemaID, + dbID, + ) } // 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 - } - if dbID, exists := tsp.MaybeGetDatabaseForTemporarySchemaID(schemaID); exists { +func (td temporaryDescriptors) getSchemaByID(schemaID descpb.ID) catalog.SchemaDescriptor { + if dbID, exists := td.maybeGetDatabaseForTemporarySchemaID(schemaID); exists { return schemadesc.NewTemporarySchema( - tsp.GetTemporarySchemaName(), + td.getTemporarySchemaName(), schemaID, dbID, ) diff --git a/pkg/sql/catalog/descs/validate.go b/pkg/sql/catalog/descs/validate.go index 52c9c268a9e5..a3529d415d37 100644 --- a/pkg/sql/catalog/descs/validate.go +++ b/pkg/sql/catalog/descs/validate.go @@ -18,6 +18,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/validate" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" + "github.com/cockroachdb/errors" ) // Validate returns any descriptor validation errors after validating using the @@ -30,6 +32,9 @@ func (tc *Collection) Validate( targetLevel catalog.ValidationLevel, descriptors ...catalog.Descriptor, ) (err error) { + if tc.stored.ValidationMode() == sessiondatapb.DescriptorValidationOff { + return nil + } vd := tc.newValidationDereferencer(txn) version := tc.settings.Version.ActiveVersion(ctx) return validate.Validate( @@ -48,7 +53,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.validateOnWrite() { return nil } var descs []catalog.Descriptor @@ -62,6 +67,20 @@ func (tc *Collection) ValidateUncommittedDescriptors(ctx context.Context, txn *k return tc.Validate(ctx, txn, catalog.ValidationWriteTelemetry, validate.Write, descs...) } +func (tc *Collection) validateOnWrite() bool { + if tc.skipValidationOnWrite { + return false + } + switch tc.stored.ValidationMode() { + case sessiondatapb.DescriptorValidationOn: + return true + case sessiondatapb.DescriptorValidationOff, sessiondatapb.DescriptorValidationReadOnly: + return false + default: + panic(errors.AssertionFailedf("unknown descriptor_validation value %q", tc.stored.ValidationMode())) + } +} + func (tc *Collection) newValidationDereferencer(txn *kv.Txn) validate.ValidationDereferencer { return &collectionBackedDereferencer{tc: tc, sd: tc.stored.NewValidationDereferencer(txn)} } diff --git a/pkg/sql/catalog/desctestutils/descriptor_test_utils.go b/pkg/sql/catalog/desctestutils/descriptor_test_utils.go index fb6d7a5d73a8..6baca88c8e0d 100644 --- a/pkg/sql/catalog/desctestutils/descriptor_test_utils.go +++ b/pkg/sql/catalog/desctestutils/descriptor_test_utils.go @@ -35,7 +35,7 @@ func TestingGetDatabaseDescriptorWithVersion( ) catalog.DatabaseDescriptor { ctx := context.Background() var desc catalog.Descriptor - direct := catkv.MakeDirect(codec, version) + direct := catkv.MakeDirect(codec, nil /* sds */, version) 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 +73,7 @@ func TestingGetSchemaDescriptorWithVersion( ) catalog.SchemaDescriptor { ctx := context.Background() var desc catalog.Descriptor - direct := catkv.MakeDirect(codec, version) + direct := catkv.MakeDirect(codec, nil /* sds */, version) 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 +169,7 @@ func testingGetObjectDescriptor( object string, ) (desc catalog.Descriptor) { ctx := context.Background() - direct := catkv.MakeDirect(codec, version) + direct := catkv.MakeDirect(codec, nil /* sds */, version) 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 { diff --git a/pkg/sql/catalog/internal/catkv/BUILD.bazel b/pkg/sql/catalog/internal/catkv/BUILD.bazel index fde4832355a9..249660d5ec7b 100644 --- a/pkg/sql/catalog/internal/catkv/BUILD.bazel +++ b/pkg/sql/catalog/internal/catkv/BUILD.bazel @@ -32,6 +32,8 @@ go_library( "//pkg/sql/pgwire/pgerror", "//pkg/sql/sem/catconstants", "//pkg/sql/sem/tree", + "//pkg/sql/sessiondata", + "//pkg/sql/sessiondatapb", "//pkg/sql/sqlerrors", "//pkg/util/hlc", "//pkg/util/log", diff --git a/pkg/sql/catalog/internal/catkv/direct.go b/pkg/sql/catalog/internal/catkv/direct.go index 69c9b98a9b97..3bda61e989d0 100644 --- a/pkg/sql/catalog/internal/catkv/direct.go +++ b/pkg/sql/catalog/internal/catkv/direct.go @@ -22,6 +22,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/validate" "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" @@ -148,10 +150,13 @@ 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, sds *sessiondata.Stack, version clusterversion.ClusterVersion, +) Direct { return &direct{ StoredCatalog: StoredCatalog{ CatalogReader: NewUncachedCatalogReader(codec), + sds: sds, }, version: version, } @@ -178,10 +183,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.ValidationMode() != sessiondatapb.DescriptorValidationOff { + 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..1c47af9b0913 100644 --- a/pkg/sql/catalog/internal/catkv/stored_catalog.go +++ b/pkg/sql/catalog/internal/catkv/stored_catalog.go @@ -21,6 +21,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/validate" "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" "github.com/cockroachdb/cockroach/pkg/util/mon" "github.com/cockroachdb/errors" ) @@ -58,19 +60,29 @@ type StoredCatalog struct { // above. allSchemasForDatabase map[descpb.ID]map[descpb.ID]string + // sds is used to access session variable values. + sds *sessiondata.Stack + // memAcc is the actual account of an injected, upstream monitor // to track memory usage of StoredCatalog. memAcc *mon.BoundAccount } // MakeStoredCatalog returns a new instance of StoredCatalog. -func MakeStoredCatalog(cr CatalogReader, monitor *mon.BytesMonitor) StoredCatalog { - sd := StoredCatalog{CatalogReader: cr} +func MakeStoredCatalog( + cr CatalogReader, sds *sessiondata.Stack, monitor *mon.BytesMonitor, +) StoredCatalog { + sc := StoredCatalog{CatalogReader: cr, sds: sds} if monitor != nil { memAcc := monitor.MakeBoundAccount() - sd.memAcc = &memAcc + sc.memAcc = &memAcc } - return sd + return sc +} + +// SetSessionDataStack sets a session data stack for this StoredCatalog. +func (sc *StoredCatalog) SetSessionDataStack(sds *sessiondata.Stack) { + sc.sds = sds } // Reset zeroes the object for re-use in a new transaction. @@ -85,10 +97,20 @@ func (sc *StoredCatalog) Reset(ctx context.Context) { CatalogReader: old.CatalogReader, cache: old.cache, nameIndex: old.nameIndex, + sds: old.sds, memAcc: old.memAcc, } } +// ValidationMode returns the validation mode currently in force. +func (sc *StoredCatalog) ValidationMode() sessiondatapb.DescriptorValidationMode { + if sc.sds == nil { + // Return default value, 'on'. + return 0 + } + return sc.sds.Top().DescriptorValidationMode +} + // ensure adds a descriptor to the StoredCatalog layer. // This should not cause any information loss. func (sc *StoredCatalog) ensure(ctx context.Context, desc catalog.Descriptor) error { @@ -396,10 +418,12 @@ func (c storedCatalogBackedDereferencer) DereferenceDescriptors( if desc == nil { continue } - if err = validate.Self(version, desc); err != nil { - return nil, err + if c.sc.ValidationMode() != sessiondatapb.DescriptorValidationOff { + 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/helpers_test.go b/pkg/sql/catalog/lease/helpers_test.go index 7b7ee44dceb1..78f1c8f3debd 100644 --- a/pkg/sql/catalog/lease/helpers_test.go +++ b/pkg/sql/catalog/lease/helpers_test.go @@ -174,7 +174,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)) + direct := catkv.MakeDirect(m.storage.codec, nil /* sds */, m.storage.settings.Version.ActiveVersion(ctx)) desc, err := direct.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. diff --git a/pkg/sql/catalog/lease/lease.go b/pkg/sql/catalog/lease/lease.go index 13ebcc0a1f8a..af9feeeb979c 100644 --- a/pkg/sql/catalog/lease/lease.go +++ b/pkg/sql/catalog/lease/lease.go @@ -127,7 +127,7 @@ 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)) + sc := catkv.MakeDirect(m.storage.codec, nil /* sds */, 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 @@ -919,7 +919,7 @@ func (m *Manager) resolveName( return err } var err error - direct := catkv.MakeDirect(m.storage.codec, m.storage.settings.Version.ActiveVersion(ctx)) + direct := catkv.MakeDirect(m.storage.codec, nil /* sds */, m.storage.settings.Version.ActiveVersion(ctx)) id, err = direct.LookupDescriptorID(ctx, txn, parentID, parentSchemaID, name) return err }); err != nil { diff --git a/pkg/sql/catalog/lease/storage.go b/pkg/sql/catalog/lease/storage.go index c686177fdd1d..462429378f14 100644 --- a/pkg/sql/catalog/lease/storage.go +++ b/pkg/sql/catalog/lease/storage.go @@ -132,7 +132,7 @@ func (s storage) acquire( } version := s.settings.Version.ActiveVersion(ctx) - direct := catkv.MakeDirect(s.codec, version) + direct := catkv.MakeDirect(s.codec, nil /* sds */, version) desc, err = direct.MustGetDescriptorByID(ctx, txn, id, catalog.Any) if err != nil { return err @@ -259,7 +259,7 @@ func (s storage) getForExpiration( return err } version := s.settings.Version.ActiveVersion(ctx) - direct := catkv.MakeDirect(s.codec, version) + direct := catkv.MakeDirect(s.codec, nil /* sds */, version) desc, err = direct.MustGetDescriptorByID(ctx, txn, id, catalog.Any) if err != nil { return err diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index c7f6e526a782..4c8e22a0eb6d 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -1006,7 +1006,7 @@ 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) + ex.extraTxnState.descCollection = s.cfg.CollectionFactory.NewCollection(ctx, sdMutIterator.sds, 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..b825d9277f40 100644 --- a/pkg/sql/create_sequence.go +++ b/pkg/sql/create_sequence.go @@ -334,7 +334,11 @@ 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() + } + if err := descbuilder.ValidateSelf(&desc, version, sd); 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..51bd810c2f73 100644 --- a/pkg/sql/descriptor_mutation_test.go +++ b/pkg/sql/descriptor_mutation_test.go @@ -87,7 +87,11 @@ 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 := descbuilder.ValidateSelf( + mt.tableDesc, + clusterversion.TestingClusterVersion, + nil, /* sd */ + ); err != nil { mt.Fatal(err) } if err := mt.kvDB.Put( @@ -147,7 +151,9 @@ 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 := descbuilder.ValidateSelf( + mt.tableDesc, clusterversion.TestingClusterVersion, nil, /* sd */ + ); err != nil { mt.Fatal(err) } if err := mt.kvDB.Put( @@ -450,21 +456,29 @@ 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 := descbuilder.ValidateSelf( + tableDesc, clusterversion.TestingClusterVersion, nil, /* sd */ + ); !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 := descbuilder.ValidateSelf( + tableDesc, clusterversion.TestingClusterVersion, nil, /* sd */ + ); !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 := descbuilder.ValidateSelf( + tableDesc, clusterversion.TestingClusterVersion, nil, /* sd */ + ); !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 := descbuilder.ValidateSelf( + tableDesc, clusterversion.TestingClusterVersion, nil, /* sd */ + ); !testutils.IsError(err, `mutation in state UNKNOWN, direction DROP, col "i", id 3`) { t.Fatal(err) } } @@ -651,7 +665,9 @@ 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 := descbuilder.ValidateSelf( + tableDesc, clusterversion.TestingClusterVersion, nil, /* sd */ + ); !testutils.IsError(err, "mutation in state UNKNOWN, direction NONE, index foo, id 2") { t.Fatal(err) } } diff --git a/pkg/sql/distsql/server.go b/pkg/sql/distsql/server.go index 2fc96ecd04ff..759e1a52ab55 100644 --- a/pkg/sql/distsql/server.go +++ b/pkg/sql/distsql/server.go @@ -488,7 +488,7 @@ 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 */) + flowCtx.Descriptors = ds.CollectionFactory.NewCollection(ctx, evalCtx.SessionDataStack, nil /* monitor */) 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..1d8143cfca6e 100644 --- a/pkg/sql/internal.go +++ b/pkg/sql/internal.go @@ -813,9 +813,7 @@ 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)), - ) + ie.extraTxnState.descCollection.SetSessionDataStack(sessiondata.NewStack(sd)) } // The returned span is finished by this function in all error paths, but if @@ -1408,10 +1406,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, nil /* sds */, 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..c8249a71a302 100644 --- a/pkg/sql/planner.go +++ b/pkg/sql/planner.go @@ -328,7 +328,7 @@ func newInternalPlanner( sds := sessiondata.NewStack(sd) if params.collection == nil { - params.collection = execCfg.CollectionFactory.NewCollection(ctx, descs.NewTemporarySchemaProvider(sds), nil /* monitor */) + params.collection = execCfg.CollectionFactory.NewCollection(ctx, sds, nil /* monitor */) } var ts time.Time diff --git a/pkg/sql/row/row_converter.go b/pkg/sql/row/row_converter.go index 036441895962..4ae99ff30691 100644 --- a/pkg/sql/row/row_converter.go +++ b/pkg/sql/row/row_converter.go @@ -267,7 +267,7 @@ 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 */) + descsCol := cf.NewCollection(ctx, evalCtx.SessionDataStack, nil /* monitor */) 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..2f866c9a7e58 100644 --- a/pkg/sql/table.go +++ b/pkg/sql/table.go @@ -304,7 +304,7 @@ func (p *planner) writeTableDescToBatch( } version := p.ExecCfg().Settings.Version.ActiveVersion(ctx) - if err := descbuilder.ValidateSelf(tableDesc, version); err != nil { + if err := descbuilder.ValidateSelf(tableDesc, version, p.SessionData()); 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..4295c8eeea95 100644 --- a/pkg/sql/table_test.go +++ b/pkg/sql/table_test.go @@ -418,7 +418,7 @@ func TestPrimaryKeyUnspecified(t *testing.T) { } desc.SetPrimaryIndex(descpb.IndexDescriptor{}) - err = descbuilder.ValidateSelf(desc, clusterversion.TestingClusterVersion) + err = descbuilder.ValidateSelf(desc, clusterversion.TestingClusterVersion, nil /* sd */) 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..0b889d917d8e 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, nil /* sds */, nil /* monitor */) if err := retryFunc(ctx, func() error { var err error allDbDescs, err = descsCol.GetAllDatabaseDescriptors(ctx, txn) 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..f6c971f8c7ad 100644 --- a/pkg/sql/tests/system_table_test.go +++ b/pkg/sql/tests/system_table_test.go @@ -218,7 +218,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, descbuilder.ValidateSelf(gen, clusterversion.TestingClusterVersion, nil /* sd */)) 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..961380d68477 100644 --- a/pkg/sql/virtual_schema.go +++ b/pkg/sql/virtual_schema.go @@ -747,7 +747,7 @@ func NewVirtualSchemaHolder( } td := tabledesc.NewBuilder(&tableDesc).BuildImmutableTable() version := st.Version.ActiveVersionOrEmpty(ctx) - if err := descbuilder.ValidateSelf(td, version); err != nil { + if err := descbuilder.ValidateSelf(td, version, nil /* sd */); err != nil { return nil, errors.NewAssertionErrorWithWrappedErrf(err, "failed to validate virtual table %s: programmer error", errors.Safe(td.GetName())) }