diff --git a/pkg/bench/rttanalysis/testdata/benchmark_expectations b/pkg/bench/rttanalysis/testdata/benchmark_expectations index 3cfa801cda3d..6ebdefce2f96 100644 --- a/pkg/bench/rttanalysis/testdata/benchmark_expectations +++ b/pkg/bench/rttanalysis/testdata/benchmark_expectations @@ -1,57 +1,57 @@ exp,benchmark -19,AlterRole/alter_role_with_1_option -20,AlterRole/alter_role_with_2_options -25,AlterRole/alter_role_with_3_options -17,AlterTableAddCheckConstraint/alter_table_add_1_check_constraint -17,AlterTableAddCheckConstraint/alter_table_add_2_check_constraints -17,AlterTableAddCheckConstraint/alter_table_add_3_check_constraints -17,AlterTableAddColumn/alter_table_add_1_column -17,AlterTableAddColumn/alter_table_add_2_columns -17,AlterTableAddColumn/alter_table_add_3_columns -22,AlterTableAddForeignKey/alter_table_add_1_foreign_key -27,AlterTableAddForeignKey/alter_table_add_2_foreign_keys -32,AlterTableAddForeignKey/alter_table_add_3_foreign_keys -22,AlterTableAddForeignKey/alter_table_add_foreign_key_with_3_columns +15,AlterRole/alter_role_with_1_option +16,AlterRole/alter_role_with_2_options +21,AlterRole/alter_role_with_3_options +15,AlterTableAddCheckConstraint/alter_table_add_1_check_constraint +15,AlterTableAddCheckConstraint/alter_table_add_2_check_constraints +15,AlterTableAddCheckConstraint/alter_table_add_3_check_constraints +15,AlterTableAddColumn/alter_table_add_1_column +15,AlterTableAddColumn/alter_table_add_2_columns +15,AlterTableAddColumn/alter_table_add_3_columns +20,AlterTableAddForeignKey/alter_table_add_1_foreign_key +25,AlterTableAddForeignKey/alter_table_add_2_foreign_keys +30,AlterTableAddForeignKey/alter_table_add_3_foreign_keys +20,AlterTableAddForeignKey/alter_table_add_foreign_key_with_3_columns 20,AlterTableConfigureZone/alter_table_configure_zone_5_replicas 20,AlterTableConfigureZone/alter_table_configure_zone_7_replicas_ 20,AlterTableConfigureZone/alter_table_configure_zone_ranges -20,AlterTableDropColumn/alter_table_drop_1_column -22,AlterTableDropColumn/alter_table_drop_2_columns -24,AlterTableDropColumn/alter_table_drop_3_columns -18,AlterTableDropConstraint/alter_table_drop_1_check_constraint -19,AlterTableDropConstraint/alter_table_drop_2_check_constraints -20,AlterTableDropConstraint/alter_table_drop_3_check_constraints +18,AlterTableDropColumn/alter_table_drop_1_column +20,AlterTableDropColumn/alter_table_drop_2_columns +22,AlterTableDropColumn/alter_table_drop_3_columns +16,AlterTableDropConstraint/alter_table_drop_1_check_constraint +17,AlterTableDropConstraint/alter_table_drop_2_check_constraints +18,AlterTableDropConstraint/alter_table_drop_3_check_constraints 12-13,AlterTableSplit/alter_table_split_at_1_value 18-19,AlterTableSplit/alter_table_split_at_2_values 24-25,AlterTableSplit/alter_table_split_at_3_values 8,AlterTableUnsplit/alter_table_unsplit_at_1_value 10,AlterTableUnsplit/alter_table_unsplit_at_2_values 12,AlterTableUnsplit/alter_table_unsplit_at_3_values -21,CreateRole/create_role_with_1_option -23,CreateRole/create_role_with_2_options -24,CreateRole/create_role_with_3_options -22,CreateRole/create_role_with_no_options -21,DropDatabase/drop_database_0_tables -28,DropDatabase/drop_database_1_table -35,DropDatabase/drop_database_2_tables -42,DropDatabase/drop_database_3_tables -29,DropRole/drop_1_role -36,DropRole/drop_2_roles -43,DropRole/drop_3_roles -19,DropSequence/drop_1_sequence -26,DropSequence/drop_2_sequences -33,DropSequence/drop_3_sequences -21,DropTable/drop_1_table -29,DropTable/drop_2_tables -37,DropTable/drop_3_tables -22,DropView/drop_1_view -30,DropView/drop_2_views -38,DropView/drop_3_views -18,Grant/grant_all_on_1_table -20,Grant/grant_all_on_2_tables -22,Grant/grant_all_on_3_tables -21,GrantRole/grant_1_role -24,GrantRole/grant_2_roles +17,CreateRole/create_role_with_1_option +19,CreateRole/create_role_with_2_options +20,CreateRole/create_role_with_3_options +18,CreateRole/create_role_with_no_options +19,DropDatabase/drop_database_0_tables +26,DropDatabase/drop_database_1_table +33,DropDatabase/drop_database_2_tables +40,DropDatabase/drop_database_3_tables +25,DropRole/drop_1_role +32,DropRole/drop_2_roles +39,DropRole/drop_3_roles +17,DropSequence/drop_1_sequence +24,DropSequence/drop_2_sequences +31,DropSequence/drop_3_sequences +19,DropTable/drop_1_table +27,DropTable/drop_2_tables +35,DropTable/drop_3_tables +20,DropView/drop_1_view +28,DropView/drop_2_views +36,DropView/drop_3_views +16,Grant/grant_all_on_1_table +18,Grant/grant_all_on_2_tables +20,Grant/grant_all_on_3_tables +17,GrantRole/grant_1_role +20,GrantRole/grant_2_roles 2,ORMQueries/activerecord_type_introspection_query 2,ORMQueries/django_table_introspection_1_table 2,ORMQueries/django_table_introspection_4_tables @@ -64,19 +64,19 @@ exp,benchmark 2,ORMQueries/pg_class 2,ORMQueries/pg_namespace 2,ORMQueries/pg_type -18,Revoke/revoke_all_on_1_table -20,Revoke/revoke_all_on_2_tables -22,Revoke/revoke_all_on_3_tables -20,RevokeRole/revoke_1_role -22,RevokeRole/revoke_2_roles +16,Revoke/revoke_all_on_1_table +18,Revoke/revoke_all_on_2_tables +20,Revoke/revoke_all_on_3_tables +16,RevokeRole/revoke_1_role +18,RevokeRole/revoke_2_roles 1,SystemDatabaseQueries/select_system.users_with_empty_database_Name 1,SystemDatabaseQueries/select_system.users_with_schema_Name 2,SystemDatabaseQueries/select_system.users_without_schema_Name -25,Truncate/truncate_1_column_0_rows -25,Truncate/truncate_1_column_1_row -25,Truncate/truncate_1_column_2_rows -25,Truncate/truncate_2_column_0_rows -25,Truncate/truncate_2_column_1_rows -25,Truncate/truncate_2_column_2_rows +23,Truncate/truncate_1_column_0_rows +23,Truncate/truncate_1_column_1_row +23,Truncate/truncate_1_column_2_rows +23,Truncate/truncate_2_column_0_rows +23,Truncate/truncate_2_column_1_rows +23,Truncate/truncate_2_column_2_rows 1,VirtualTableQueries/select_crdb_internal.invalid_objects_with_1_fk 1,VirtualTableQueries/select_crdb_internal.tables_with_1_fk diff --git a/pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations b/pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations index 294ce36ffcee..08d4bd2a04ea 100644 --- a/pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations +++ b/pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations @@ -1,19 +1,19 @@ exp,benchmark -23,AlterPrimaryRegion/alter_empty_database_alter_primary_region -24,AlterPrimaryRegion/alter_empty_database_set_initial_primary_region -23,AlterPrimaryRegion/alter_populated_database_alter_primary_region -25,AlterPrimaryRegion/alter_populated_database_set_initial_primary_region -22,AlterRegions/alter_empty_database_add_region -23,AlterRegions/alter_empty_database_drop_region -22,AlterRegions/alter_populated_database_add_region -23,AlterRegions/alter_populated_database_drop_region -23,AlterSurvivalGoals/alter_empty_database_from_region_to_zone -23,AlterSurvivalGoals/alter_empty_database_from_zone_to_region -43,AlterSurvivalGoals/alter_populated_database_from_region_to_zone -43,AlterSurvivalGoals/alter_populated_database_from_zone_to_region -24,AlterTableLocality/alter_from_global_to_rbr -26,AlterTableLocality/alter_from_global_to_regional_by_table -22,AlterTableLocality/alter_from_rbr_to_global -22,AlterTableLocality/alter_from_rbr_to_regional_by_table -26,AlterTableLocality/alter_from_regional_by_table_to_global -24,AlterTableLocality/alter_from_regional_by_table_to_rbr +21,AlterPrimaryRegion/alter_empty_database_alter_primary_region +22,AlterPrimaryRegion/alter_empty_database_set_initial_primary_region +21,AlterPrimaryRegion/alter_populated_database_alter_primary_region +23,AlterPrimaryRegion/alter_populated_database_set_initial_primary_region +20,AlterRegions/alter_empty_database_add_region +21,AlterRegions/alter_empty_database_drop_region +20,AlterRegions/alter_populated_database_add_region +21,AlterRegions/alter_populated_database_drop_region +21,AlterSurvivalGoals/alter_empty_database_from_region_to_zone +21,AlterSurvivalGoals/alter_empty_database_from_zone_to_region +41,AlterSurvivalGoals/alter_populated_database_from_region_to_zone +41,AlterSurvivalGoals/alter_populated_database_from_zone_to_region +22,AlterTableLocality/alter_from_global_to_rbr +24,AlterTableLocality/alter_from_global_to_regional_by_table +20,AlterTableLocality/alter_from_rbr_to_global +20,AlterTableLocality/alter_from_rbr_to_regional_by_table +24,AlterTableLocality/alter_from_regional_by_table_to_global +22,AlterTableLocality/alter_from_regional_by_table_to_rbr diff --git a/pkg/migration/migrations/delete_deprecated_namespace_tabledesc_external_test.go b/pkg/migration/migrations/delete_deprecated_namespace_tabledesc_external_test.go index 2f26155494fa..87fa78618078 100644 --- a/pkg/migration/migrations/delete_deprecated_namespace_tabledesc_external_test.go +++ b/pkg/migration/migrations/delete_deprecated_namespace_tabledesc_external_test.go @@ -47,16 +47,21 @@ func TestDeleteDeprecatedNamespaceDescriptorMigration(t *testing.T) { }) defer tc.Stopper().Stop(ctx) - // Inject deprecated namespace table descriptor and namespace entries. + // Inject deprecated namespace table descriptors and namespace entries. err := tc.Servers[0].DB().Txn(ctx, func(ctx context.Context, txn *kv.Txn) error { codec := keys.SystemSQLCodec deprecated := *systemschema.NamespaceTable.TableDesc() deprecated.ID = keys.DeprecatedNamespaceTableID - descProto := &descpb.Descriptor{Union: &descpb.Descriptor_Table{Table: &deprecated}} + deprecatedDescProto := &descpb.Descriptor{Union: &descpb.Descriptor_Table{Table: &deprecated}} + namespace2 := *systemschema.NamespaceTable.TableDesc() + namespace2.ID = keys.NamespaceTableID + namespace2.Name = `namespace2` + ns2DescProto := &descpb.Descriptor{Union: &descpb.Descriptor_Table{Table: &namespace2}} b := txn.NewBatch() - b.Put(catalogkeys.MakeDescMetadataKey(codec, keys.DeprecatedNamespaceTableID), descProto) + b.Put(catalogkeys.MakeDescMetadataKey(codec, keys.DeprecatedNamespaceTableID), deprecatedDescProto) namespaceKey := catalogkeys.MakePublicObjectNameKey(codec, keys.SystemDatabaseID, `namespace`) b.Put(namespaceKey, keys.DeprecatedNamespaceTableID) + b.Put(catalogkeys.MakeDescMetadataKey(codec, keys.NamespaceTableID), ns2DescProto) namespace2Key := catalogkeys.MakePublicObjectNameKey(codec, keys.SystemDatabaseID, `namespace2`) b.Put(namespace2Key, keys.NamespaceTableID) return txn.Run(ctx, b) diff --git a/pkg/sql/catalog/descs/BUILD.bazel b/pkg/sql/catalog/descs/BUILD.bazel index 09528256412c..e5ed97985968 100644 --- a/pkg/sql/catalog/descs/BUILD.bazel +++ b/pkg/sql/catalog/descs/BUILD.bazel @@ -19,6 +19,8 @@ go_library( "temporary_descriptors.go", "txn.go", "type.go", + "uncommitted_descriptors.go", + "validate.go", "virtual_descriptors.go", ], importpath = "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs", diff --git a/pkg/sql/catalog/descs/collection.go b/pkg/sql/catalog/descs/collection.go index 0ce8b843ab2f..0a84fb47a5da 100644 --- a/pkg/sql/catalog/descs/collection.go +++ b/pkg/sql/catalog/descs/collection.go @@ -26,7 +26,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/hydratedtables" "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/util/encoding" @@ -53,6 +52,7 @@ func makeCollection( virtual: makeVirtualDescriptors(virtualSchemas), leased: makeLeasedDescriptors(leaseMgr), synthetic: makeSyntheticDescriptors(), + uncommitted: makeUncommittedDescriptors(), kv: makeKVDescriptors(codec), temporary: makeTemporaryDescriptors(codec, sessionData), } @@ -75,6 +75,14 @@ type Collection struct { // the transaction using them is complete. leased leasedDescriptors + // Descriptors modified by the uncommitted transaction affiliated with this + // Collection. This allows a transaction to see its own modifications while + // bypassing the descriptor lease mechanism. The lease mechanism will have its + // own transaction to read the descriptor and will hang waiting for the + // uncommitted changes to the descriptor. These descriptors are local to this + // Collection and invisible to other transactions. + uncommitted uncommittedDescriptors + // A collection of descriptors which were read from the store. kv kvDescriptors @@ -149,6 +157,7 @@ func (tc *Collection) ReleaseLeases(ctx context.Context) { // ReleaseAll calls ReleaseLeases. func (tc *Collection) ReleaseAll(ctx context.Context) { tc.ReleaseLeases(ctx) + tc.uncommitted.reset() tc.kv.reset() tc.synthetic.reset() tc.deletedDescs = nil @@ -157,13 +166,13 @@ func (tc *Collection) ReleaseAll(ctx context.Context) { // HasUncommittedTables returns true if the Collection contains uncommitted // tables. func (tc *Collection) HasUncommittedTables() bool { - return tc.kv.hasUncommittedTables() + return tc.uncommitted.hasUncommittedTables() } // HasUncommittedTypes returns true if the Collection contains uncommitted // types. func (tc *Collection) HasUncommittedTypes() bool { - return tc.kv.hasUncommittedTypes() + return tc.uncommitted.hasUncommittedTypes() } // Satisfy the linter. @@ -179,23 +188,7 @@ var _ = (*Collection).HasUncommittedTypes // immutably will return a copy of the descriptor in the current state. A deep // copy is performed in this call. func (tc *Collection) AddUncommittedDescriptor(desc catalog.MutableDescriptor) error { - _, err := tc.kv.addUncommittedDescriptor(desc) - return err -} - -// maybeRefreshCachedFieldsOnTypeDescriptor refreshes the cached fields on a -// Mutable if the given descriptor is a type descriptor and works as a pass -// through for all other descriptors. Mutable type descriptors are refreshed to -// reconstruct enumMetadata. This ensures that tables hydration following a -// type descriptor update (in the same txn) happens using the modified fields. -func maybeRefreshCachedFieldsOnTypeDescriptor( - desc catalog.MutableDescriptor, -) (catalog.MutableDescriptor, error) { - typeDesc, ok := desc.(catalog.TypeDescriptor) - if ok { - return typedesc.UpdateCachedFieldsOnModifiedMutable(typeDesc) - } - return desc, nil + return tc.uncommitted.checkIn(desc) } // ValidateOnWriteEnabled is the cluster setting used to enable or disable @@ -238,27 +231,18 @@ func (tc *Collection) WriteDesc( // undergone a schema change. Returns nil for no schema changes. The version // returned for each schema change is ClusterVersion - 1, because that's the one // that will be used when checking for table descriptor two version invariance. -func (tc *Collection) GetDescriptorsWithNewVersion() []lease.IDVersion { - return tc.kv.getDescriptorsWithNewVersion() +func (tc *Collection) GetDescriptorsWithNewVersion() (originalVersions []lease.IDVersion) { + _ = tc.uncommitted.iterateNewVersionByID(func(_ catalog.NameEntry, originalVersion lease.IDVersion) error { + originalVersions = append(originalVersions, originalVersion) + return nil + }) + return originalVersions } // GetUncommittedTables returns all the tables updated or created in the // transaction. func (tc *Collection) GetUncommittedTables() (tables []catalog.TableDescriptor) { - return tc.kv.getUncommittedTables() -} - -// ValidateUncommittedDescriptors validates all uncommitted descriptors. -// Validation includes cross-reference checks. Referenced descriptors are -// read from the store unless they happen to also be part of the uncommitted -// descriptor set. We purposefully avoid using leased descriptors as those may -// 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) error { - if tc.skipValidationOnWrite || !ValidateOnWriteEnabled.Get(&tc.settings.SV) { - return nil - } - return tc.kv.validateUncommittedDescriptors(ctx, txn) + return tc.uncommitted.getUncommittedTables() } func newMutableSyntheticDescriptorAssertionError(id descpb.ID) error { diff --git a/pkg/sql/catalog/descs/descriptor.go b/pkg/sql/catalog/descs/descriptor.go index 55cd0d307ffc..519867d6c73e 100644 --- a/pkg/sql/catalog/descs/descriptor.go +++ b/pkg/sql/catalog/descs/descriptor.go @@ -78,9 +78,8 @@ func (tc *Collection) getDescriptorByIDMaybeSetTxnDeadline( ctx context.Context, txn *kv.Txn, id descpb.ID, flags tree.CommonLookupFlags, setTxnDeadline bool, ) (catalog.Descriptor, error) { getDescriptorByID := func() (catalog.Descriptor, error) { - if vd, err := tc.virtual.getByID( - ctx, id, flags.RequireMutable, - ); vd != nil || err != nil { + vd, err := tc.virtual.getByID(ctx, id, flags.RequireMutable) + if vd != nil || err != nil { return vd, err } @@ -90,35 +89,43 @@ func (tc *Collection) getDescriptorByIDMaybeSetTxnDeadline( } return sd, nil } - if ud := tc.kv.getUncommittedByID(id); ud != nil { - log.VEventf(ctx, 2, "found uncommitted descriptor %d", id) - if flags.RequireMutable { - return ud.mutable, nil + + { + ud := tc.uncommitted.getByID(id) + if ud != nil { + log.VEventf(ctx, 2, "found uncommitted descriptor %d", id) + if flags.RequireMutable { + ud, err = tc.uncommitted.checkOut(id) + if err != nil { + return nil, err + } + } + return ud, nil } - return ud.immutable, nil } - if flags.AvoidCached || flags.RequireMutable || lease.TestingTableLeasesAreDisabled() { - return tc.kv.getByID(ctx, txn, id, flags.RequireMutable) - } + if !flags.AvoidCached && !flags.RequireMutable && !lease.TestingTableLeasesAreDisabled() { + // If we have already read all of the descriptors, use it as a negative + // cache to short-circuit a lookup we know will be doomed to fail. + // + // TODO(ajwerner): More generally leverage this set of kv descriptors on + // the resolution path. + if tc.kv.idDefinitelyDoesNotExist(id) { + return nil, catalog.ErrDescriptorNotFound + } - // If we have already read all of the descriptor, use it as a negative - // cache to short-circuit a lookup we know will be doomed to fail. - // - // TODO(ajwerner): More generally leverage this set of kv descriptors on - // the resolution path. - if tc.kv.idDefinitelyDoesNotExist(id) { - return nil, catalog.ErrDescriptorNotFound + desc, shouldReadFromStore, err := tc.leased.getByID(ctx, tc.deadlineHolder(txn), id, setTxnDeadline) + if err != nil { + return nil, err + } + if !shouldReadFromStore { + return desc, nil + } } - desc, shouldReadFromStore, err := tc.leased.getByID(ctx, tc.deadlineHolder(txn), id, setTxnDeadline) - if err != nil { - return nil, err - } - if shouldReadFromStore { - return tc.kv.getByID(ctx, txn, id, flags.RequireMutable) - } - return desc, nil + return tc.withReadFromStore(flags.RequireMutable, func() (desc catalog.MutableDescriptor, err error) { + return tc.kv.getByID(ctx, txn, id) + }) } desc, err := getDescriptorByID() @@ -172,36 +179,59 @@ func (tc *Collection) getByName( } { - refuseFurtherLookup, ud := tc.kv.getUncommittedByName(parentID, parentSchemaID, name) + refuseFurtherLookup, ud := tc.uncommitted.getByName(parentID, parentSchemaID, name) if ud != nil { log.VEventf(ctx, 2, "found uncommitted descriptor %d", ud.GetID()) if mutable { - return true, ud.mutable, nil + ud, err = tc.uncommitted.checkOut(ud.GetID()) + if err != nil { + return false, nil, err + } } - return true, ud.immutable, nil + return true, ud, nil } if refuseFurtherLookup { return false, nil, nil } } - if avoidCached || mutable || lease.TestingTableLeasesAreDisabled() { - return tc.kv.getByName( - ctx, txn, parentID, parentSchemaID, name, mutable, - ) + if !avoidCached && !mutable && !lease.TestingTableLeasesAreDisabled() { + var shouldReadFromStore bool + desc, shouldReadFromStore, err = tc.leased.getByName(ctx, tc.deadlineHolder(txn), parentID, parentSchemaID, name) + if err != nil { + return false, nil, err + } + if !shouldReadFromStore { + return desc != nil, desc, nil + } } - desc, shouldReadFromStore, err := tc.leased.getByName( - ctx, tc.deadlineHolder(txn), parentID, parentSchemaID, name) + desc, err = tc.withReadFromStore(mutable, func() (desc catalog.MutableDescriptor, err error) { + uncommittedDB, _ := tc.uncommitted.getByID(parentID).(catalog.DatabaseDescriptor) + return tc.kv.getByName(ctx, txn, uncommittedDB, parentID, parentSchemaID, name) + }) + return desc != nil, desc, err +} + +func (tc *Collection) withReadFromStore( + mutable bool, fn func() (desc catalog.MutableDescriptor, err error), +) (desc catalog.Descriptor, err error) { + mut, err := fn() + if mut == nil || err != nil { + return nil, err + } + desc, err = tc.uncommitted.add(mut) if err != nil { - return false, nil, err + return nil, err } - if shouldReadFromStore { - return tc.kv.getByName( - ctx, txn, parentID, parentSchemaID, name, mutable, - ) + if mutable { + desc, err = tc.uncommitted.checkOut(desc.GetID()) + if err != nil { + return nil, err + } } - return desc != nil, desc, nil + tc.kv.releaseAllDescriptors() + return desc, nil } func (tc *Collection) deadlineHolder(txn *kv.Txn) deadlineHolder { diff --git a/pkg/sql/catalog/descs/kv_descriptors.go b/pkg/sql/catalog/descs/kv_descriptors.go index 23a194e30e32..858cbc4506a8 100644 --- a/pkg/sql/catalog/descs/kv_descriptors.go +++ b/pkg/sql/catalog/descs/kv_descriptors.go @@ -19,58 +19,15 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" - "github.com/cockroachdb/cockroach/pkg/util/iterutil" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" ) -// uncommittedDescriptor is a descriptor that has been modified in the current -// transaction. -type uncommittedDescriptor struct { - mutable catalog.MutableDescriptor - immutable catalog.Descriptor -} - -func (u *uncommittedDescriptor) GetName() string { - return u.immutable.GetName() -} - -func (u *uncommittedDescriptor) GetParentID() descpb.ID { - return u.immutable.GetParentID() -} - -func (u uncommittedDescriptor) GetParentSchemaID() descpb.ID { - return u.immutable.GetParentSchemaID() -} - -func (u uncommittedDescriptor) GetID() descpb.ID { - return u.immutable.GetID() -} - -var _ catalog.NameEntry = (*uncommittedDescriptor)(nil) - type kvDescriptors struct { codec keys.SQLCodec - // Descriptors modified by the uncommitted transaction affiliated with this - // Collection. This allows a transaction to see its own modifications while - // bypassing the descriptor lease mechanism. The lease mechanism will have its - // own transaction to read the descriptor and will hang waiting for the - // uncommitted changes to the descriptor. These descriptors are local to this - // Collection and invisible to other transactions. - uncommittedDescriptors nstree.Map - // uncommittedDescriptorNames is the set of names which a read or written - // descriptor took on at some point in its lifetime. Everything added to - // uncommittedDescriptors is added to uncommittedDescriptorNames as well - // as all of the known draining names. The idea is that if we find that - // a name is not in the above map but is in the set, then we can avoid - // doing a lookup. - uncommittedDescriptorNames nstree.Set - // allDescriptors is a slice of all available descriptors. The descriptors // are cached to avoid repeated lookups by users like virtual tables. The // cache is purged whenever events would cause a scan of all descriptors to @@ -129,16 +86,12 @@ func (d *allDescriptors) contains(id descpb.ID) bool { func makeKVDescriptors(codec keys.SQLCodec) kvDescriptors { return kvDescriptors{ - codec: codec, - uncommittedDescriptors: nstree.MakeMap(), - uncommittedDescriptorNames: nstree.MakeSet(), + codec: codec, } } func (kd *kvDescriptors) reset() { kd.releaseAllDescriptors() - kd.uncommittedDescriptors.Clear() - kd.uncommittedDescriptorNames.Clear() } // releaseAllDescriptors releases the cached slice of all descriptors @@ -153,14 +106,18 @@ func (kd *kvDescriptors) releaseAllDescriptors() { } func (kd *kvDescriptors) getByID( - ctx context.Context, txn *kv.Txn, id descpb.ID, mutable bool, -) (desc catalog.Descriptor, err error) { - _, desc, err = kd.getDescriptor(ctx, txn, id, mutable, "" /* maybeLookedUpName */) - return desc, err + ctx context.Context, txn *kv.Txn, id descpb.ID, +) (desc catalog.MutableDescriptor, err error) { + return kd.getDescriptor(ctx, txn, id, "" /* maybeLookedUpName */) } func (kd *kvDescriptors) lookupName( - ctx context.Context, txn *kv.Txn, parentID descpb.ID, parentSchemaID descpb.ID, name string, + ctx context.Context, + txn *kv.Txn, + maybeDB catalog.DatabaseDescriptor, + parentID descpb.ID, + parentSchemaID descpb.ID, + name string, ) (found bool, _ descpb.ID, _ error) { // Handle special cases which might avoid a namespace table query. switch parentID { @@ -177,10 +134,10 @@ func (kd *kvDescriptors) lookupName( return id != descpb.InvalidID, id, err default: if parentSchemaID == descpb.InvalidID { - if ud := kd.getUncommittedByID(parentID); ud != nil { + if maybeDB != nil { // Special case: looking up a schema, but in a database which we already // have the descriptor for. We find the schema ID in there. - id := ud.immutable.(catalog.DatabaseDescriptor).GetSchemaID(name) + id := maybeDB.GetSchemaID(name) return id != descpb.InvalidID, id, nil } } @@ -198,82 +155,52 @@ func (kd *kvDescriptors) lookupName( func (kd *kvDescriptors) getByName( ctx context.Context, txn *kv.Txn, + maybeDB catalog.DatabaseDescriptor, parentID descpb.ID, parentSchemaID descpb.ID, name string, - mutable bool, -) (found bool, desc catalog.Descriptor, err error) { - found, descID, err := kd.lookupName(ctx, txn, parentID, parentSchemaID, name) +) (desc catalog.MutableDescriptor, err error) { + found, descID, err := kd.lookupName(ctx, txn, maybeDB, parentID, parentSchemaID, name) if !found || err != nil { - return found, nil, err + return nil, err } - return kd.getDescriptor(ctx, txn, descID, mutable, name) + return kd.getDescriptor(ctx, txn, descID, name) } func (kd *kvDescriptors) getDescriptor( - ctx context.Context, txn *kv.Txn, id descpb.ID, mutable bool, maybeLookedUpName string, -) (found bool, _ catalog.Descriptor, _ error) { - withNameLookup := maybeLookedUpName != "" + ctx context.Context, txn *kv.Txn, id descpb.ID, maybeLookedUpName string, +) (_ catalog.MutableDescriptor, _ error) { if id == keys.SystemDatabaseID { - b := dbdesc.NewBuilder(systemschema.SystemDB.DatabaseDesc()) // Special handling for the system database descriptor. + // + // This is done for performance reasons, to save ourselves an unnecessary + // round trip to storage which otherwise quickly compounds. + // // The system database descriptor should never actually be mutated, which is // why we return the same hard-coded descriptor every time. It's assumed // that callers of this method will check the privileges on the descriptor // (like any other database) and return an error. - if mutable { - return true, b.BuildExistingMutable(), nil - } - return true, b.BuildImmutable(), nil + return dbdesc.NewBuilder(systemschema.SystemDB.DatabaseDesc()).BuildExistingMutable(), nil } - // Always pick up a mutable copy so it can be cached. - desc, err := catalogkv.MustGetMutableDescriptorByID(ctx, txn, kd.codec, id) + + withNameLookup := maybeLookedUpName != "" + mut, err := catalogkv.MustGetMutableDescriptorByID(ctx, txn, kd.codec, id) if err != nil { if withNameLookup && errors.Is(err, catalog.ErrDescriptorNotFound) { // Having done the namespace lookup, the descriptor must exist. err = errors.WithAssertionFailure(err) } - return false, nil, err + return nil, err } if withNameLookup { // Immediately after a RENAME an old name still points to the descriptor // during the drain phase for the name. Do not return a descriptor during // draining. - if desc.GetName() != maybeLookedUpName { - return false, nil, nil + if mut.GetName() != maybeLookedUpName { + return nil, nil } } - ud, err := kd.addUncommittedDescriptor(desc) - if err != nil { - return false, nil, err - } - if !mutable { - return true, ud.immutable, nil - } - return true, ud.mutable, nil -} - -// getUncommittedByName returns a descriptor for the requested name -// if the requested name is for a descriptor modified within the transaction -// affiliated with the Collection. -// -// The first return value "refuseFurtherLookup" is true when there is a known -// rename of that descriptor, so it would be invalid to miss the cache and go to -// KV (where the descriptor prior to the rename may still exist). -func (kd *kvDescriptors) getUncommittedByName( - dbID descpb.ID, schemaID descpb.ID, name string, -) (refuseFurtherLookup bool, desc *uncommittedDescriptor) { - - // Walk latest to earliest so that a DROP followed by a CREATE with the same - // name will result in the CREATE being seen. - if got := kd.uncommittedDescriptors.GetByName(dbID, schemaID, name); got != nil { - return false, got.(*uncommittedDescriptor) - } - return kd.uncommittedDescriptorNames.Contains(descpb.NameInfo{ - ParentID: dbID, - ParentSchemaID: schemaID, - Name: name, - }), nil + return mut, nil } func (kd *kvDescriptors) getAllDescriptors( @@ -333,103 +260,6 @@ func (kd *kvDescriptors) getSchemasForDatabase( return kd.allSchemasForDatabase[dbID], nil } -func (kd *kvDescriptors) getUncommittedByID(id descpb.ID) *uncommittedDescriptor { - ud, _ := kd.uncommittedDescriptors.GetByID(id).(*uncommittedDescriptor) - return ud -} - -func (kd *kvDescriptors) getDescriptorsWithNewVersion() []lease.IDVersion { - var descs []lease.IDVersion - _ = kd.uncommittedDescriptors.IterateByID(func(entry catalog.NameEntry) error { - desc := entry.(*uncommittedDescriptor) - if mut := desc.mutable; !mut.IsNew() && mut.IsUncommittedVersion() { - descs = append(descs, lease.NewIDVersionPrev(mut.OriginalName(), mut.OriginalID(), mut.OriginalVersion())) - } - return nil - }) - return descs -} - -func (kd *kvDescriptors) getUncommittedTables() (tables []catalog.TableDescriptor) { - _ = kd.uncommittedDescriptors.IterateByID(func(entry catalog.NameEntry) error { - desc := entry.(*uncommittedDescriptor) - table, ok := desc.immutable.(catalog.TableDescriptor) - if ok && desc.immutable.IsUncommittedVersion() { - tables = append(tables, table) - } - return nil - }) - return tables -} - -func (kd *kvDescriptors) validateUncommittedDescriptors(ctx context.Context, txn *kv.Txn) error { - descs := make([]catalog.Descriptor, 0, kd.uncommittedDescriptors.Len()) - _ = kd.uncommittedDescriptors.IterateByID(func(descriptor catalog.NameEntry) error { - descs = append(descs, descriptor.(*uncommittedDescriptor).immutable) - return nil - }) - if len(descs) == 0 { - return nil - } - // TODO(ajwerner): Leverage this cache as the DescGetter. - bdg := catalogkv.NewOneLevelUncachedDescGetter(txn, kd.codec) - return catalog.Validate( - ctx, - bdg, - catalog.ValidationWriteTelemetry, - catalog.ValidationLevelAllPreTxnCommit, - descs..., - ).CombinedError() -} - -func (kd *kvDescriptors) hasUncommittedTables() (has bool) { - _ = kd.uncommittedDescriptors.IterateByID(func(entry catalog.NameEntry) error { - if _, has = entry.(*uncommittedDescriptor).immutable.(catalog.TableDescriptor); has { - return iterutil.StopIteration() - } - return nil - }) - return has -} - -func (kd *kvDescriptors) hasUncommittedTypes() (has bool) { - _ = kd.uncommittedDescriptors.IterateByID(func(entry catalog.NameEntry) error { - if _, has = entry.(*uncommittedDescriptor).immutable.(catalog.TypeDescriptor); has { - return iterutil.StopIteration() - } - return nil - }) - return has -} - -func (kd *kvDescriptors) addUncommittedDescriptor( - desc catalog.MutableDescriptor, -) (*uncommittedDescriptor, error) { - version := desc.GetVersion() - origVersion := desc.OriginalVersion() - if version != origVersion && version != origVersion+1 { - return nil, errors.AssertionFailedf( - "descriptor %d version %d not compatible with cluster version %d", - desc.GetID(), version, origVersion) - } - - mutable, err := maybeRefreshCachedFieldsOnTypeDescriptor(desc) - if err != nil { - return nil, err - } - - ud := &uncommittedDescriptor{ - mutable: mutable, - immutable: desc.ImmutableCopy(), - } - for _, n := range desc.GetDrainingNames() { - kd.uncommittedDescriptorNames.Add(n) - } - kd.uncommittedDescriptors.Upsert(ud) - kd.releaseAllDescriptors() - return ud, nil -} - func (kd *kvDescriptors) idDefinitelyDoesNotExist(id descpb.ID) bool { if kd.allDescriptors.isEmpty() { return false diff --git a/pkg/sql/catalog/descs/table.go b/pkg/sql/catalog/descs/table.go index 0a0332b80b51..4a6419c6ef08 100644 --- a/pkg/sql/catalog/descs/table.go +++ b/pkg/sql/catalog/descs/table.go @@ -58,14 +58,22 @@ func (tc *Collection) getTableByName( return true, desc.(catalog.TableDescriptor), nil } -// GetUncommittedTableByID returns an uncommitted table by its ID. -func (tc *Collection) GetUncommittedTableByID(id descpb.ID) *tabledesc.Mutable { - if ud := tc.kv.getUncommittedByID(id); ud != nil { - if table, ok := ud.mutable.(*tabledesc.Mutable); ok { - return table - } +// GetUncommittedMutableTableByID returns an uncommitted mutable table by its +// ID. +func (tc *Collection) GetUncommittedMutableTableByID(id descpb.ID) (*tabledesc.Mutable, error) { + ud := tc.uncommitted.getByID(id) + if ud == nil { + return nil, nil + } + mut, err := tc.uncommitted.checkOut(ud.GetID()) + if err != nil { + return nil, err + } + if table, ok := mut.(*tabledesc.Mutable); ok { + return table, nil } - return nil + // Check non-table descriptors back in. + return nil, tc.uncommitted.checkIn(mut) } // GetMutableTableByID returns a mutable table descriptor with diff --git a/pkg/sql/catalog/descs/uncommitted_descriptors.go b/pkg/sql/catalog/descs/uncommitted_descriptors.go new file mode 100644 index 000000000000..f3712dbe5c97 --- /dev/null +++ b/pkg/sql/catalog/descs/uncommitted_descriptors.go @@ -0,0 +1,264 @@ +// Copyright 2021 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" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" + "github.com/cockroachdb/cockroach/pkg/util/iterutil" + "github.com/cockroachdb/errors" +) + +// uncommittedDescriptor is a descriptor that has been modified in the current +// transaction. +type uncommittedDescriptor struct { + mutable catalog.MutableDescriptor + immutable catalog.Descriptor +} + +// GetName implements the catalog.NameEntry interface. +func (u *uncommittedDescriptor) GetName() string { + return u.immutable.GetName() +} + +// GetParentID implements the catalog.NameEntry interface. +func (u *uncommittedDescriptor) GetParentID() descpb.ID { + return u.immutable.GetParentID() +} + +// GetParentSchemaID implements the catalog.NameEntry interface. +func (u uncommittedDescriptor) GetParentSchemaID() descpb.ID { + return u.immutable.GetParentSchemaID() +} + +// GetID implements the catalog.NameEntry interface. +func (u uncommittedDescriptor) GetID() descpb.ID { + return u.immutable.GetID() +} + +var _ catalog.NameEntry = (*uncommittedDescriptor)(nil) + +// uncommittedDescriptors is the data structure holding all +// uncommittedDescriptor objects for a Collection. +// +// Immutable descriptors can be freely looked up. +// Mutable descriptors can be: +// 1. added into it, +// 2. checked out of it, +// 3. checked back in to it. +// +// An error will be triggered by: +// - checking out a mutable descriptor that hasn't yet been added, +// - checking in a descriptor that has been added but not yet checked out, +// - any checked-out-but-not-checked-in mutable descriptors at commit time. +// +type uncommittedDescriptors struct { + + // Descriptors modified by the uncommitted transaction affiliated with this + // Collection. This allows a transaction to see its own modifications while + // bypassing the descriptor lease mechanism. The lease mechanism will have its + // own transaction to read the descriptor and will hang waiting for the + // uncommitted changes to the descriptor. These descriptors are local to this + // Collection and invisible to other transactions. + descs nstree.Map + + // descNames is the set of names which a read or written + // descriptor took on at some point in its lifetime. Everything added to + // uncommittedDescriptors is added to descNames as well + // as all of the known draining names. The idea is that if we find that + // a name is not in the above map but is in the set, then we can avoid + // doing a lookup. + descNames nstree.Set +} + +func makeUncommittedDescriptors() uncommittedDescriptors { + ud := uncommittedDescriptors{ + descs: nstree.MakeMap(), + descNames: nstree.MakeSet(), + } + ud.reset() + return ud +} + +func (ud *uncommittedDescriptors) reset() { + ud.descs.Clear() + ud.descNames.Clear() +} + +// add adds a descriptor to the set of uncommitted descriptors and returns +// an immutable copy of that descriptor. +func (ud *uncommittedDescriptors) add(mut catalog.MutableDescriptor) (catalog.Descriptor, error) { + uNew, err := makeUncommittedDescriptor(mut) + if err != nil { + return nil, err + } + for _, n := range uNew.immutable.GetDrainingNames() { + ud.descNames.Add(n) + } + ud.descs.Upsert(uNew) + return uNew.immutable, err +} + +// checkOut checks out an uncommitted mutable descriptor for use in the +// transaction. This descriptor should later be checked in again. +func (ud *uncommittedDescriptors) checkOut(id descpb.ID) (catalog.MutableDescriptor, error) { + entry := ud.descs.GetByID(id) + if entry == nil { + return nil, errors.NewAssertionErrorWithWrappedErrf( + errors.New("descriptor hasn't been added yet"), + "cannot check in uncommitted descriptor with ID %d", + id) + + } + u := entry.(*uncommittedDescriptor) + return u.mutable, nil +} + +// checkIn checks in an uncommitted mutable descriptor that was previously +// checked out. +func (ud *uncommittedDescriptors) checkIn(mut catalog.MutableDescriptor) error { + // TODO(postamar): actually check that the descriptor has been checked out. + _, err := ud.add(mut) + return err +} + +func makeUncommittedDescriptor(desc catalog.MutableDescriptor) (*uncommittedDescriptor, error) { + version := desc.GetVersion() + origVersion := desc.OriginalVersion() + if version != origVersion && version != origVersion+1 { + return nil, errors.AssertionFailedf( + "descriptor %d version %d not compatible with cluster version %d", + desc.GetID(), version, origVersion) + } + + mutable, err := maybeRefreshCachedFieldsOnTypeDescriptor(desc) + if err != nil { + return nil, err + } + + return &uncommittedDescriptor{ + mutable: mutable, + immutable: mutable.ImmutableCopy(), + }, nil +} + +// maybeRefreshCachedFieldsOnTypeDescriptor refreshes the cached fields on a +// Mutable if the given descriptor is a type descriptor and works as a pass +// through for all other descriptors. Mutable type descriptors are refreshed to +// reconstruct enumMetadata. This ensures that tables hydration following a +// type descriptor update (in the same txn) happens using the modified fields. +func maybeRefreshCachedFieldsOnTypeDescriptor( + desc catalog.MutableDescriptor, +) (catalog.MutableDescriptor, error) { + typeDesc, ok := desc.(catalog.TypeDescriptor) + if ok { + return typedesc.UpdateCachedFieldsOnModifiedMutable(typeDesc) + } + return desc, nil +} + +// getByID looks up an uncommitted descriptor by ID. +func (ud *uncommittedDescriptors) getByID(id descpb.ID) catalog.Descriptor { + entry := ud.descs.GetByID(id) + if entry == nil { + return nil + } + return entry.(*uncommittedDescriptor).immutable +} + +// getByName returns a descriptor for the requested name if the requested name +// is for a descriptor modified within the transaction affiliated with the +// Collection. +// +// The first return value "hasKnownRename" is true when there is a known +// rename of that descriptor, so it would be invalid to miss the cache and go to +// KV (where the descriptor prior to the rename may still exist). +func (ud *uncommittedDescriptors) getByName( + dbID descpb.ID, schemaID descpb.ID, name string, +) (hasKnownRename bool, desc catalog.Descriptor) { + // Walk latest to earliest so that a DROP followed by a CREATE with the same + // name will result in the CREATE being seen. + if got := ud.descs.GetByName(dbID, schemaID, name); got != nil { + return false, got.(*uncommittedDescriptor).immutable + } + return ud.descNames.Contains(descpb.NameInfo{ + ParentID: dbID, + ParentSchemaID: schemaID, + Name: name, + }), nil +} + +func (ud *uncommittedDescriptors) iterateNewVersionByID( + fn func(entry catalog.NameEntry, originalVersion lease.IDVersion) error, +) error { + return ud.descs.IterateByID(func(entry catalog.NameEntry) error { + mut := entry.(*uncommittedDescriptor).mutable + if mut.IsNew() || !mut.IsUncommittedVersion() { + return nil + } + return fn(entry, lease.NewIDVersionPrev(mut.OriginalName(), mut.OriginalID(), mut.OriginalVersion())) + }) +} + +func (ud *uncommittedDescriptors) iterateImmutableByID( + fn func(imm catalog.Descriptor) error, +) error { + return ud.descs.IterateByID(func(entry catalog.NameEntry) error { + return fn(entry.(*uncommittedDescriptor).immutable) + }) +} + +func (ud *uncommittedDescriptors) getUncommittedTables() (tables []catalog.TableDescriptor) { + _ = ud.iterateImmutableByID(func(imm catalog.Descriptor) error { + table, ok := imm.(catalog.TableDescriptor) + if ok && imm.IsUncommittedVersion() { + tables = append(tables, table) + } + return nil + }) + return tables +} + +func (ud *uncommittedDescriptors) getUncommittedDescriptorsForValidation() ( + descs []catalog.Descriptor, +) { + _ = ud.iterateImmutableByID(func(imm catalog.Descriptor) error { + // TODO(postamar): only return descriptors with !IsUncommittedVersion() + // This requires safeguard mechanisms like actually enforcing uncommitted + // descriptor check=out and check-in rules. + descs = append(descs, imm) + return nil + }) + return descs +} + +func (ud *uncommittedDescriptors) hasUncommittedTables() (has bool) { + _ = ud.iterateImmutableByID(func(desc catalog.Descriptor) error { + if _, has = desc.(catalog.TableDescriptor); has { + return iterutil.StopIteration() + } + return nil + }) + return has +} + +func (ud *uncommittedDescriptors) hasUncommittedTypes() (has bool) { + _ = ud.iterateImmutableByID(func(desc catalog.Descriptor) error { + if _, has = desc.(catalog.TypeDescriptor); has { + return iterutil.StopIteration() + } + return nil + }) + return has +} diff --git a/pkg/sql/catalog/descs/validate.go b/pkg/sql/catalog/descs/validate.go new file mode 100644 index 000000000000..4dadf004d6a0 --- /dev/null +++ b/pkg/sql/catalog/descs/validate.go @@ -0,0 +1,175 @@ +// Copyright 2021 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 ( + "context" + + "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/kv" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catconstants" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" + "github.com/cockroachdb/errors" +) + +// ValidateUncommittedDescriptors validates all uncommitted descriptors. +// Validation includes cross-reference checks. Referenced descriptors are +// read from the store unless they happen to also be part of the uncommitted +// descriptor set. We purposefully avoid using leased descriptors as those may +// 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) { + return nil + } + descs := tc.uncommitted.getUncommittedDescriptorsForValidation() + if len(descs) == 0 { + return nil + } + + bdg := collectionBatchDescGetter{tc: tc, txn: txn} + return errors.CombineErrors(err, catalog.Validate( + ctx, + bdg, + catalog.ValidationWriteTelemetry, + catalog.ValidationLevelAllPreTxnCommit, + descs..., + ).CombinedError()) +} + +// collectionBatchDescGetter wraps a Collection to implement the +// catalog.BatchDescGetter interface for validation. +type collectionBatchDescGetter struct { + tc *Collection + txn *kv.Txn +} + +var _ catalog.BatchDescGetter = &collectionBatchDescGetter{} + +func (c collectionBatchDescGetter) fallback() catalog.BatchDescGetter { + return catalogkv.NewOneLevelUncachedDescGetter(c.txn, c.tc.codec()) +} + +// GetDescs implements the catalog.BatchDescGetter interface by leveraging the +// collection's uncommitted descriptors. +func (c collectionBatchDescGetter) GetDescs( + ctx context.Context, reqs []descpb.ID, +) (ret []catalog.Descriptor, _ error) { + ret = make([]catalog.Descriptor, len(reqs)) + fallbackReqs := make([]descpb.ID, 0, len(reqs)) + fallbackRetIndexes := make([]int, 0, len(reqs)) + for i, id := range reqs { + desc, err := c.fastDescLookup(ctx, id) + if err != nil { + return nil, err + } + if desc == nil { + fallbackReqs = append(fallbackReqs, id) + fallbackRetIndexes = append(fallbackRetIndexes, i) + } else { + ret[i] = desc + } + } + if len(fallbackReqs) > 0 { + fallbackRet, err := c.fallback().GetDescs(ctx, fallbackReqs) + if err != nil { + return nil, err + } + for j, desc := range fallbackRet { + ret[fallbackRetIndexes[j]] = desc + } + } + return ret, nil +} + +func (c collectionBatchDescGetter) fastDescLookup( + ctx context.Context, id descpb.ID, +) (catalog.Descriptor, error) { + leaseCacheEntry := c.tc.leased.cache.GetByID(id) + if leaseCacheEntry != nil { + return leaseCacheEntry.(lease.LeasedDescriptor).Underlying(), nil + } + return c.tc.uncommitted.getByID(id), nil +} + +// GetNamespaceEntries implements the catalog.BatchDescGetter interface by +// delegating to the fallback catalogkv implementation. +func (c collectionBatchDescGetter) GetNamespaceEntries( + ctx context.Context, reqs []descpb.NameInfo, +) (ret []descpb.ID, _ error) { + ret = make([]descpb.ID, len(reqs)) + fallbackReqs := make([]descpb.NameInfo, 0, len(reqs)) + fallbackRetIndexes := make([]int, 0, len(reqs)) + for i, req := range reqs { + found, id, err := c.fastNamespaceLookup(ctx, req) + if err != nil { + return nil, err + } + if found { + ret[i] = id + } else { + fallbackReqs = append(fallbackReqs, req) + fallbackRetIndexes = append(fallbackRetIndexes, i) + } + } + if len(fallbackReqs) > 0 { + fallbackRet, err := c.fallback().GetNamespaceEntries(ctx, fallbackReqs) + if err != nil { + return nil, err + } + for j, id := range fallbackRet { + ret[fallbackRetIndexes[j]] = id + } + } + return ret, nil +} + +func (c collectionBatchDescGetter) fastNamespaceLookup( + ctx context.Context, req descpb.NameInfo, +) (found bool, id descpb.ID, err error) { + // Handle special cases. + // TODO(postamar): namespace lookups should go through Collection + switch req.ParentID { + case descpb.InvalidID: + if req.ParentSchemaID == descpb.InvalidID && req.Name == catconstants.SystemDatabaseName { + // Looking up system database ID, which is hard-coded. + return true, keys.SystemDatabaseID, nil + } + case keys.SystemDatabaseID: + // Looking up system database objects, which are cached. + id, err = lookupSystemDatabaseNamespaceCache(ctx, c.tc.codec(), req.ParentSchemaID, req.Name) + return id != descpb.InvalidID, id, err + } + return false, descpb.InvalidID, nil +} + +// GetDesc implements the catalog.BatchDescGetter interface by delegating to +// GetDescs. +func (c collectionBatchDescGetter) GetDesc( + ctx context.Context, id descpb.ID, +) (catalog.Descriptor, error) { + descs, err := c.GetDescs(ctx, []descpb.ID{id}) + if err != nil { + return nil, err + } + return descs[0], nil +} + +// GetNamespaceEntry implements the catalog.BatchDescGetter interface by +// delegating to GetNamespaceEntries. +func (c collectionBatchDescGetter) GetNamespaceEntry( + ctx context.Context, parentID, parentSchemaID descpb.ID, name string, +) (descpb.ID, error) { + return c.fallback().GetNamespaceEntry(ctx, parentID, parentSchemaID, name) +} diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index f937cc61f7d2..c9ed93fd2840 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -323,7 +323,9 @@ func (n *createTableNode) startExec(params runParams) error { } var foundExternalReference bool for id := range refs { - if t := params.p.Descriptors().GetUncommittedTableByID(id); t == nil || !t.IsNew() { + if t, err := params.p.Descriptors().GetUncommittedMutableTableByID(id); err != nil { + return err + } else if t == nil || !t.IsNew() { foundExternalReference = true break } diff --git a/pkg/sql/create_view.go b/pkg/sql/create_view.go index 97631391d2c2..c54d6c582819 100644 --- a/pkg/sql/create_view.go +++ b/pkg/sql/create_view.go @@ -104,7 +104,10 @@ func (n *createViewNode) startExec(params runParams) error { backRefMutables := make(map[descpb.ID]*tabledesc.Mutable, len(n.planDeps)) hasTempBackref := false for id, updated := range n.planDeps { - backRefMutable := params.p.Descriptors().GetUncommittedTableByID(id) + backRefMutable, err := params.p.Descriptors().GetUncommittedMutableTableByID(id) + if err != nil { + return err + } if backRefMutable == nil { backRefMutable = tabledesc.NewBuilder(updated.desc.TableDesc()).BuildExistingMutableTable() }