Merge #77630

77630: descs: improve validation performance when reading descriptors r=postamar a=postamar

    catkv: allow ValidationDereferencer injection
    
    This commit adds a new dependency injection entry point to the descriptor
    lookup functions in the catkv functions. In itself, this change does not
    alter the current functionality at all. In a subsequent commit it will allow
    descriptor reads in the descriptor collection to leverage the latter
    for cross-reference descriptor validation.
    
    Release justification: low-risk update
    Release note: None


    descs: refine uncommitted descriptors layer
    
    When reading a descriptor from storage via the collection, we perform
    cross-reference validation on it. This implies reading all referenced
    descriptors from storage as well. Prior to this commit, these referenced
    descriptors were simply discarded. We now store them in the uncommitted
    descriptors layer of the descriptors collection. This can dramatically
    dramatically reduce the number of round-trips to storage when doing
    a sequence of descriptor lookups in the same transaction. This happens
    for instance when the declarative schema changer does a DROP DATABASE
    CASCADE on a large database.
    
    This commit adds qualifiers to the uncommitted descriptors stored in the
    descriptors collection to effectively use it as a cache.
    We now distinguish between:
    1. descriptors which have been read from storage and have since possibly
       been modified;
    2. descriptors which have been read from storage and validated but which
       we know haven't yet been modified in this transaction;
    3. descriptors which have been read from storage but only for the
       purpose of cross-reference validation of another descriptor.
    
    In this commit, right before attempting to read a descriptor from storage
    we look for it in (3). If we find it, we validate it (with
    cross-reference validation) and promote it to (2).
    
    When validating cross-references for a descriptor, we previously
    systematically read the referenced descriptors from storage. In this
    commit, we instead use any we can find in (2) or (3). Any of which we do
    end up reading from storage we then add to (3).
    
    The rest of the time, the descriptors in (3) are ignored.
    
    Release justification: low-risk high-benefit performance improvement
    Release note: None


Co-authored-by: Marius Posta <[email protected]>

pkg/bench/rttanalysis/testdata/benchmark_expectations

@@ -1,57 +1,57 @@
 exp,benchmark
-14,AlterRole/alter_role_with_1_option
-15,AlterRole/alter_role_with_2_options
-20,AlterRole/alter_role_with_3_options
-18,AlterTableAddCheckConstraint/alter_table_add_1_check_constraint
-18,AlterTableAddCheckConstraint/alter_table_add_2_check_constraints
-18,AlterTableAddCheckConstraint/alter_table_add_3_check_constraints
-18,AlterTableAddColumn/alter_table_add_1_column
-18,AlterTableAddColumn/alter_table_add_2_columns
-18,AlterTableAddColumn/alter_table_add_3_columns
-23,AlterTableAddForeignKey/alter_table_add_1_foreign_key
-28,AlterTableAddForeignKey/alter_table_add_2_foreign_keys
-33,AlterTableAddForeignKey/alter_table_add_3_foreign_keys
-23,AlterTableAddForeignKey/alter_table_add_foreign_key_with_3_columns
-23,AlterTableConfigureZone/alter_table_configure_zone_5_replicas
-23,AlterTableConfigureZone/alter_table_configure_zone_7_replicas_
-23,AlterTableConfigureZone/alter_table_configure_zone_ranges
-19,AlterTableDropColumn/alter_table_drop_1_column
-20,AlterTableDropColumn/alter_table_drop_2_columns
-21,AlterTableDropColumn/alter_table_drop_3_columns
-18,AlterTableDropConstraint/alter_table_drop_1_check_constraint
-18,AlterTableDropConstraint/alter_table_drop_2_check_constraints
-18,AlterTableDropConstraint/alter_table_drop_3_check_constraints
-15,AlterTableSplit/alter_table_split_at_1_value
-21,AlterTableSplit/alter_table_split_at_2_values
-27,AlterTableSplit/alter_table_split_at_3_values
-11,AlterTableUnsplit/alter_table_unsplit_at_1_value
-13,AlterTableUnsplit/alter_table_unsplit_at_2_values
-15,AlterTableUnsplit/alter_table_unsplit_at_3_values
-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
-20,DropDatabase/drop_database_0_tables
-22,DropDatabase/drop_database_1_table
-24,DropDatabase/drop_database_2_tables
-26,DropDatabase/drop_database_3_tables
-24,DropRole/drop_1_role
-31,DropRole/drop_2_roles
-38,DropRole/drop_3_roles
-20,DropSequence/drop_1_sequence
-23,DropSequence/drop_2_sequences
-26,DropSequence/drop_3_sequences
-20,DropTable/drop_1_table
-23,DropTable/drop_2_tables
-26,DropTable/drop_3_tables
-23,DropView/drop_1_view
-26,DropView/drop_2_views
-29,DropView/drop_3_views
-19,Grant/grant_all_on_1_table
-19,Grant/grant_all_on_2_tables
-19,Grant/grant_all_on_3_tables
-13,GrantRole/grant_1_role
-15,GrantRole/grant_2_roles
+11,AlterRole/alter_role_with_1_option
+12,AlterRole/alter_role_with_2_options
+16,AlterRole/alter_role_with_3_options
+13,AlterTableAddCheckConstraint/alter_table_add_1_check_constraint
+13,AlterTableAddCheckConstraint/alter_table_add_2_check_constraints
+13,AlterTableAddCheckConstraint/alter_table_add_3_check_constraints
+13,AlterTableAddColumn/alter_table_add_1_column
+13,AlterTableAddColumn/alter_table_add_2_columns
+13,AlterTableAddColumn/alter_table_add_3_columns
+17,AlterTableAddForeignKey/alter_table_add_1_foreign_key
+21,AlterTableAddForeignKey/alter_table_add_2_foreign_keys
+25,AlterTableAddForeignKey/alter_table_add_3_foreign_keys
+17,AlterTableAddForeignKey/alter_table_add_foreign_key_with_3_columns
+19,AlterTableConfigureZone/alter_table_configure_zone_5_replicas
+19,AlterTableConfigureZone/alter_table_configure_zone_7_replicas_
+19,AlterTableConfigureZone/alter_table_configure_zone_ranges
+14,AlterTableDropColumn/alter_table_drop_1_column
+15,AlterTableDropColumn/alter_table_drop_2_columns
+16,AlterTableDropColumn/alter_table_drop_3_columns
+13,AlterTableDropConstraint/alter_table_drop_1_check_constraint
+13,AlterTableDropConstraint/alter_table_drop_2_check_constraints
+13,AlterTableDropConstraint/alter_table_drop_3_check_constraints
+10,AlterTableSplit/alter_table_split_at_1_value
+15,AlterTableSplit/alter_table_split_at_2_values
+20,AlterTableSplit/alter_table_split_at_3_values
+7,AlterTableUnsplit/alter_table_unsplit_at_1_value
+9,AlterTableUnsplit/alter_table_unsplit_at_2_values
+11,AlterTableUnsplit/alter_table_unsplit_at_3_values
+13,CreateRole/create_role_with_1_option
+15,CreateRole/create_role_with_2_options
+16,CreateRole/create_role_with_3_options
+14,CreateRole/create_role_with_no_options
+14,DropDatabase/drop_database_0_tables
+15,DropDatabase/drop_database_1_table
+16,DropDatabase/drop_database_2_tables
+17,DropDatabase/drop_database_3_tables
+20,DropRole/drop_1_role
+27,DropRole/drop_2_roles
+34,DropRole/drop_3_roles
+14,DropSequence/drop_1_sequence
+16,DropSequence/drop_2_sequences
+18,DropSequence/drop_3_sequences
+14,DropTable/drop_1_table
+16,DropTable/drop_2_tables
+18,DropTable/drop_3_tables
+16,DropView/drop_1_view
+18,DropView/drop_2_views
+20,DropView/drop_3_views
+14,Grant/grant_all_on_1_table
+14,Grant/grant_all_on_2_tables
+14,Grant/grant_all_on_3_tables
+10,GrantRole/grant_1_role
+12,GrantRole/grant_2_roles
 2,ORMQueries/activerecord_type_introspection_query
 2,ORMQueries/django_table_introspection_1_table
 2,ORMQueries/django_table_introspection_4_tables
@@ -76,19 +76,19 @@ exp,benchmark
 4,ORMQueries/pg_my_temp_schema_multiple_times
 4,ORMQueries/pg_namespace
 2,ORMQueries/pg_type
-19,Revoke/revoke_all_on_1_table
-19,Revoke/revoke_all_on_2_tables
-19,Revoke/revoke_all_on_3_tables
-13,RevokeRole/revoke_1_role
-15,RevokeRole/revoke_2_roles
+14,Revoke/revoke_all_on_1_table
+14,Revoke/revoke_all_on_2_tables
+14,Revoke/revoke_all_on_3_tables
+10,RevokeRole/revoke_1_role
+12,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
-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
+18,Truncate/truncate_1_column_0_rows
+18,Truncate/truncate_1_column_1_row
+18,Truncate/truncate_1_column_2_rows
+18,Truncate/truncate_2_column_0_rows
+18,Truncate/truncate_2_column_1_rows
+18,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

pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations

@@ -1,19 +1,19 @@
 exp,benchmark
-21,AlterPrimaryRegion/alter_empty_database_alter_primary_region
-25-26,AlterPrimaryRegion/alter_empty_database_set_initial_primary_region
-21,AlterPrimaryRegion/alter_populated_database_alter_primary_region
-26,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
-25,AlterTableLocality/alter_from_global_to_rbr
-27,AlterTableLocality/alter_from_global_to_regional_by_table
-23,AlterTableLocality/alter_from_rbr_to_global
-23,AlterTableLocality/alter_from_rbr_to_regional_by_table
-27,AlterTableLocality/alter_from_regional_by_table_to_global
-25,AlterTableLocality/alter_from_regional_by_table_to_rbr
+17,AlterPrimaryRegion/alter_empty_database_alter_primary_region
+23,AlterPrimaryRegion/alter_empty_database_set_initial_primary_region
+17,AlterPrimaryRegion/alter_populated_database_alter_primary_region
+24,AlterPrimaryRegion/alter_populated_database_set_initial_primary_region
+17,AlterRegions/alter_empty_database_add_region
+17,AlterRegions/alter_empty_database_drop_region
+17,AlterRegions/alter_populated_database_add_region
+17,AlterRegions/alter_populated_database_drop_region
+17,AlterSurvivalGoals/alter_empty_database_from_region_to_zone
+17,AlterSurvivalGoals/alter_empty_database_from_zone_to_region
+37,AlterSurvivalGoals/alter_populated_database_from_region_to_zone
+37,AlterSurvivalGoals/alter_populated_database_from_zone_to_region
+15,AlterTableLocality/alter_from_global_to_rbr
+17,AlterTableLocality/alter_from_global_to_regional_by_table
+14,AlterTableLocality/alter_from_rbr_to_global
+14,AlterTableLocality/alter_from_rbr_to_regional_by_table
+17,AlterTableLocality/alter_from_regional_by_table_to_global
+15,AlterTableLocality/alter_from_regional_by_table_to_rbr

pkg/sql/catalog/descs/collection.go

@@ -194,9 +194,6 @@ func (tc *Collection) HasUncommittedTypes() bool {
 	return tc.uncommitted.hasUncommittedTypes()
 }
 
-// Satisfy the linter.
-var _ = (*Collection).HasUncommittedTypes
-
 // AddUncommittedDescriptor adds an uncommitted descriptor modified in the
 // transaction to the Collection. The descriptor must either be a new descriptor
 // or carry the original version or carry the subsequent version to the original
@@ -261,7 +258,7 @@ func (tc *Collection) WriteDesc(
 // 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() (originalVersions []lease.IDVersion) {
-	_ = tc.uncommitted.iterateNewVersionByID(func(_ catalog.NameEntry, originalVersion lease.IDVersion) error {
+	_ = tc.uncommitted.iterateNewVersionByID(func(originalVersion lease.IDVersion) error {
 		originalVersions = append(originalVersions, originalVersion)
 		return nil
 	})
@@ -294,7 +291,8 @@ func (tc *Collection) GetAllDescriptors(ctx context.Context, txn *kv.Txn) (nstre
 func (tc *Collection) GetAllDatabaseDescriptors(
 	ctx context.Context, txn *kv.Txn,
 ) ([]catalog.DatabaseDescriptor, error) {
-	return tc.kv.getAllDatabaseDescriptors(ctx, txn, tc.version)
+	vd := tc.newValidationDereferencer(txn)
+	return tc.kv.getAllDatabaseDescriptors(ctx, tc.version, txn, vd)
 }
 
 // GetAllTableDescriptorsInDatabase returns all the table descriptors visible to

pkg/sql/catalog/descs/descriptor.go

@@ -125,21 +125,48 @@ func (tc *Collection) getDescriptorsByID(
 		}
 	}
 
-	kvIDs := make([]descpb.ID, 0, len(ids))
+	remainingIDs := make([]descpb.ID, 0, len(ids))
 	indexes := make([]int, 0, len(ids))
 	for i, id := range ids {
 		if descs[i] != nil {
 			continue
 		}
-		kvIDs = append(kvIDs, id)
+		remainingIDs = append(remainingIDs, id)
 		indexes = append(indexes, i)
 	}
-	if len(kvIDs) == 0 {
+	if len(remainingIDs) == 0 {
 		// No KV lookup necessary, return early.
 		return descs, nil
 	}
-	kvDescs, err := tc.withReadFromStore(flags.RequireMutable, func() ([]catalog.MutableDescriptor, error) {
-		return tc.kv.getByIDs(ctx, txn, tc.version, kvIDs)
+	kvDescs, err := tc.withReadFromStore(flags.RequireMutable, func() ([]catalog.Descriptor, error) {
+		ret := make([]catalog.Descriptor, len(remainingIDs))
+		// Try to re-use any unvalidated descriptors we may have.
+		kvIDs := make([]descpb.ID, 0, len(remainingIDs))
+		kvIndexes := make([]int, 0, len(remainingIDs))
+		for i, id := range remainingIDs {
+			if imm, status := tc.uncommitted.getImmutableByID(id); imm != nil && status == notValidatedYet {
+				err := tc.Validate(ctx, txn, catalog.ValidationReadTelemetry, catalog.ValidationLevelCrossReferences, imm)
+				if err != nil {
+					return nil, err
+				}
+				ret[i] = imm
+				continue
+			}
+			kvIDs = append(kvIDs, id)
+			kvIndexes = append(kvIndexes, i)
+		}
+		// Read all others from the store.
+		if len(kvIDs) > 0 {
+			vd := tc.newValidationDereferencer(txn)
+			kvDescs, err := tc.kv.getByIDs(ctx, tc.version, txn, vd, kvIDs)
+			if err != nil {
+				return nil, err
+			}
+			for k, imm := range kvDescs {
+				ret[kvIndexes[k]] = imm
+			}
+		}
+		return ret, nil
 	})
 	if err != nil {
 		return nil, err
@@ -179,8 +206,8 @@ func (q *byIDLookupContext) lookupSynthetic(id descpb.ID) (catalog.Descriptor, e
 }
 
 func (q *byIDLookupContext) lookupUncommitted(id descpb.ID) (_ catalog.Descriptor, err error) {
-	ud := q.tc.uncommitted.getByID(id)
-	if ud == nil {
+	ud, status := q.tc.uncommitted.getImmutableByID(id)
+	if ud == nil || status == notValidatedYet {
 		return nil, nil
 	}
 	log.VEventf(q.ctx, 2, "found uncommitted descriptor %d", id)
@@ -291,21 +318,22 @@ func (tc *Collection) getByName(
 	}
 
 	var descs []catalog.Descriptor
-	descs, err = tc.withReadFromStore(mutable, func() ([]catalog.MutableDescriptor, error) {
-		uncommittedDB, _ := tc.uncommitted.getByID(parentID).(catalog.DatabaseDescriptor)
+	descs, err = tc.withReadFromStore(mutable, func() ([]catalog.Descriptor, error) {
+		// Try to re-use an unvalidated descriptor if there is one.
+		if imm := tc.uncommitted.getUnvalidatedByName(parentID, parentSchemaID, name); imm != nil {
+			return []catalog.Descriptor{imm},
+				tc.Validate(ctx, txn, catalog.ValidationReadTelemetry, catalog.ValidationLevelCrossReferences, imm)
+		}
+		// If not possible, read it from the store.
+		uncommittedParent, _ := tc.uncommitted.getImmutableByID(parentID)
+		uncommittedDB, _ := catalog.AsDatabaseDescriptor(uncommittedParent)
 		version := tc.settings.Version.ActiveVersion(ctx)
-		desc, err := tc.kv.getByName(
-			ctx,
-			txn,
-			version,
-			uncommittedDB,
-			parentID,
-			parentSchemaID,
-			name)
+		vd := tc.newValidationDereferencer(txn)
+		imm, err := tc.kv.getByName(ctx, version, txn, vd, uncommittedDB, parentID, parentSchemaID, name)
 		if err != nil {
 			return nil, err
 		}
-		return []catalog.MutableDescriptor{desc}, nil
+		return []catalog.Descriptor{imm}, nil
 	})
 	if err != nil {
 		return false, nil, err
@@ -318,18 +346,17 @@ func (tc *Collection) getByName(
 // layer. The logic is the same regardless of whether the descriptor was read
 // by name or by ID.
 func (tc *Collection) withReadFromStore(
-	requireMutable bool, readFn func() ([]catalog.MutableDescriptor, error),
+	requireMutable bool, readFn func() ([]catalog.Descriptor, error),
 ) (descs []catalog.Descriptor, _ error) {
-	muts, err := readFn()
+	descs, err := readFn()
 	if err != nil {
 		return nil, err
 	}
-	descs = make([]catalog.Descriptor, len(muts))
-	for i, mut := range muts {
-		if mut == nil {
+	for i, desc := range descs {
+		if desc == nil {
 			continue
 		}
-		desc, err := tc.uncommitted.add(mut)
+		desc, err = tc.uncommitted.add(desc.NewBuilder().BuildExistingMutable(), notCheckedOutYet)
 		if err != nil {
 			return nil, err
 		}