Merge #157918

157918: catalog/lease: address bugs with leased descriptors in catalog views r=fqazi a=fqazi

This patch makes the following changes to improve behaviour of leased descriptors in catalog views and extend testing:

1. Fixing a bug in the locked leasing where processing multiple descriptor versions could lead to a panic. This would happen if the range feed delivered the special update key before the modified descriptors.
2. Addressed a bug where create_type_statments would break since comments were not being looked up for the point look up index. This patch also adds crdb_internal into the local-leased-descriptor config
3. Enable locked leasing timestamps for the large schema benchmark to improve test coverage. This also helps us ensure no performance regression exists in this mode.

Fixes: #157916
Fixes: #157917
Fixes: #157880

Co-authored-by: Faizan Qazi <[email protected]>

Author: craig[bot]

pkg/cmd/roachtest/tests/large_schema_benchmark.go

@@ -160,6 +160,9 @@ func registerLargeSchemaBenchmark(r registry.Registry, numTables int, isMultiReg
 						// pg_catalog and information_schema.
 						_, err = conn.Exec("SET CLUSTER SETTING sql.catalog.allow_leased_descriptors.enabled = 'true'")
 						require.NoError(t, err)
+						// Enabled locked descriptor leasing for correctness.
+						_, err = conn.Exec("SET CLUSTER SETTING sql.catalog.descriptor_lease.use_locked_timestamps.enabled = 'true'")
+						require.NoError(t, err)
 						// Since we will be making a large number of databases / tables
 						// quickly,on MR the job retention can slow things down. Let's
 						// minimize how long jobs are kept, so that the creation / ingest

pkg/sql/catalog/descs/getters.go

@@ -530,6 +530,14 @@ func (b ByIDGetterBuilder) WithoutSynthetic() ByIDGetterBuilder {
 	return b
 }
 
+// WithMetaData configures the byIDGetterBuilder to read zone configs and comments
+// for lease descriptors. This is always acquired for descriptors from storage,
+// and may need an extra round trip.
+func (b ByIDGetterBuilder) WithMetaData() ByIDGetterBuilder {
+	b.flags.layerFilters.withMetadata = true
+	return b
+}
+
 // WithoutDropped configures the ByIDGetterBuilder to error on descriptors
 // which are in a dropped state.
 func (b ByIDGetterBuilder) WithoutDropped() ByIDGetterBuilder {

pkg/sql/catalog/lease/lease.go

@@ -2204,7 +2204,7 @@ func (m *Manager) processDescriptorUpdate(
 			}
 			// Remove one element from the end after.
 			entry.mu.DescriptorIDs = entry.mu.DescriptorIDs[:len(entry.mu.DescriptorIDs)-1]
-			entry.mu.DescriptorVersions = entry.mu.DescriptorVersions[:len(entry.mu.DescriptorIDs)-1]
+			entry.mu.DescriptorVersions = entry.mu.DescriptorVersions[:len(entry.mu.DescriptorVersions)-1]
 			break
 		}
 		return len(entry.mu.DescriptorIDs) == 0

pkg/sql/crdb_internal.go

@@ -3227,7 +3227,7 @@ CREATE TABLE crdb_internal.create_type_statements (
 				addRow func(...tree.Datum) error,
 			) (matched bool, err error) {
 				id := descpb.ID(tree.MustBeDInt(unwrappedConstraint))
-				scName, typDesc, err := getSchemaAndTypeByTypeID(ctx, p, id)
+				scName, typDesc, err := getSchemaAndTypeByTypeID(ctx, p, id, true /* includeMetaData */)
 				if err != nil || typDesc == nil {
 					return false, err
 				}

pkg/sql/logictest/testdata/logic_test/crdb_internal

@@ -1,6 +1,6 @@
 # 3node-tenant is blocked from running this file due to heavy reliance on
 # unavailable node IDs in this test.
-# LogicTest: !3node-tenant-default-configs !local-prepared
+# LogicTest: !3node-tenant-default-configs !local-prepared local-leased-descriptors default-configs
 
 query error database "crdb_internal" does not exist
 ALTER DATABASE crdb_internal RENAME TO not_crdb_internal

pkg/sql/logictest/tests/local-leased-descriptors/BUILD.bazel

@@ -12,7 +12,7 @@ go_test(
         "//build/toolchains:is_heavy": {"test.Pool": "heavy"},
         "//conditions:default": {"test.Pool": "large"},
     }),
-    shard_count = 11,
+    shard_count = 12,
     tags = ["cpu:1"],
     deps = [
         "//pkg/base",

pkg/sql/logictest/tests/local-leased-descriptors/generated_test.go

@@ -3796,9 +3796,13 @@ func addPGAttributeRowForCompositeType(
 }
 
 func getSchemaAndTypeByTypeID(
-	ctx context.Context, p *planner, id descpb.ID,
+	ctx context.Context, p *planner, id descpb.ID, includeMetaData bool,
 ) (catalog.SchemaDescriptor, catalog.TypeDescriptor, error) {
-	typDesc, err := descs.GetCatalogDescriptorGetter(p.Descriptors(), p.txn, &p.EvalContext().Settings.SV).WithoutNonPublic().Get().Type(ctx, id)
+	getter := descs.GetCatalogDescriptorGetter(p.Descriptors(), p.txn, &p.EvalContext().Settings.SV)
+	if includeMetaData {
+		getter = getter.WithMetaData()
+	}
+	typDesc, err := getter.WithoutNonPublic().Get().Type(ctx, id)
 	if err != nil {
 		// If the type was not found, it may be a table.
 		if !(errors.Is(err, catalog.ErrDescriptorNotFound) || pgerror.GetPGCode(err) == pgcode.UndefinedObject) {
@@ -3897,7 +3901,7 @@ https://www.postgresql.org/docs/9.5/catalog-pg-type.html`,
 
 				id := typedesc.UserDefinedTypeOIDToID(ooid)
 
-				sc, typDesc, err := getSchemaAndTypeByTypeID(ctx, p, id)
+				sc, typDesc, err := getSchemaAndTypeByTypeID(ctx, p, id, false /* includeMetaData */)
 				if err != nil || typDesc == nil {
 					return false, err
 				}