Merge #103257 #103276

103257: sql: clean up code for pg_description r=fqazi a=rafiss By including builtin function comments in crdb_internal.kv_catalog_comments, we can remove a lot of corner case handling in the pg_description table generation code. informs #103106 Release note: None 103276: roachtest: disable metamorphic expiration leases in some tests r=erikgrinaker a=erikgrinaker This caused test failures in: * `decommission/mixed-versions` * `follower-reads/mixed-version/single-region` * `import/mixed-versions` * `kv/splits/nodes=3/quiesce=true` * `rebalance/by-load/leases/mixed-version` * `rebalance/by-load/replicas/mixed-version` * `schemachange/secondary-index-multi-version` Follows #103190. Resolves #103271. Resolves #103272. Resolves #103275. Resolves #103268. Resolves #103287. Resolves #103303. Touches #101620. Epic: none Release note: None Co-authored-by: Rafi Shamim <[email protected]> Co-authored-by: Erik Grinaker <[email protected]>
cockroachdb · May 15, 2023 · a077bee · a077bee
3 parents 4bb9d4b + 5ef16bd + e5e5a99
commit a077bee
Show file tree

Hide file tree

Showing 14 changed files with 69 additions and 61 deletions.
diff --git a/pkg/cmd/roachtest/registry/test_spec.go b/pkg/cmd/roachtest/registry/test_spec.go
@@ -192,6 +192,8 @@ type LeaseType int
 
 func (l LeaseType) String() string {
 	switch l {
+	case DefaultLeases:
+		return "default"
 	case EpochLeases:
 		return "epoch"
 	case ExpirationLeases:

diff --git a/pkg/cmd/roachtest/tests/decommission.go b/pkg/cmd/roachtest/tests/decommission.go
@@ -102,7 +102,6 @@ func registerDecommission(r registry.Registry) {
 			Name:    "decommission/mixed-versions",
 			Owner:   registry.OwnerKV,
 			Cluster: r.MakeClusterSpec(numNodes),
-			Leases:  registry.MetamorphicLeases,
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 				if c.IsLocal() && runtime.GOARCH == "arm64" {
 					t.Skip("Skip under ARM64. See https://github.com/cockroachdb/cockroach/issues/89268")

diff --git a/pkg/cmd/roachtest/tests/follower_reads.go b/pkg/cmd/roachtest/tests/follower_reads.go
@@ -102,7 +102,6 @@ func registerFollowerReads(r registry.Registry) {
 			3, /* nodeCount */
 			spec.CPU(2),
 		),
-		Leases: registry.MetamorphicLeases,
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 			if c.IsLocal() && runtime.GOARCH == "arm64" {
 				t.Skip("Skip under ARM64. See https://github.com/cockroachdb/cockroach/issues/89268")

diff --git a/pkg/cmd/roachtest/tests/import.go b/pkg/cmd/roachtest/tests/import.go
@@ -356,7 +356,6 @@ func registerImportMixedVersion(r registry.Registry) {
 		Owner: registry.OwnerSQLQueries,
 		// Mixed-version support was added in 21.1.
 		Cluster: r.MakeClusterSpec(4),
-		Leases:  registry.MetamorphicLeases,
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 			if c.IsLocal() && runtime.GOARCH == "arm64" {
 				t.Skip("Skip under ARM64. See https://github.com/cockroachdb/cockroach/issues/89268")

diff --git a/pkg/cmd/roachtest/tests/kv.go b/pkg/cmd/roachtest/tests/kv.go
@@ -699,26 +699,30 @@ func registerKVSplits(r registry.Registry) {
 	for _, item := range []struct {
 		quiesce bool
 		splits  int
+		leases  registry.LeaseType
 		timeout time.Duration
 	}{
 		// NB: with 500000 splits, this test sometimes fails since it's pushing
 		// far past the number of replicas per node we support, at least if the
 		// ranges start to unquiesce (which can set off a cascade due to resource
 		// exhaustion).
-		{true, 300000, 2 * time.Hour},
+		{true, 300000, registry.EpochLeases, 2 * time.Hour},
 		// This version of the test prevents range quiescence to trigger the
 		// badness described above more reliably for when we wish to improve
 		// the performance. For now, just verify that 30k unquiesced ranges
 		// is tenable.
-		{false, 30000, 2 * time.Hour},
+		{false, 30000, registry.EpochLeases, 2 * time.Hour},
+		// Expiration-based leases prevent quiescence, and are also more expensive
+		// to keep alive. Again, just verify that 30k ranges is ok.
+		{false, 30000, registry.ExpirationLeases, 2 * time.Hour},
 	} {
 		item := item // for use in closure below
 		r.Add(registry.TestSpec{
-			Name:    fmt.Sprintf("kv/splits/nodes=3/quiesce=%t", item.quiesce),
+			Name:    fmt.Sprintf("kv/splits/nodes=3/quiesce=%t/lease=%s", item.quiesce, item.leases),
 			Owner:   registry.OwnerKV,
 			Timeout: item.timeout,
 			Cluster: r.MakeClusterSpec(4),
-			Leases:  registry.MetamorphicLeases,
+			Leases:  item.leases,
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 				nodes := c.Spec().NodeCount - 1
 				c.Put(ctx, t.Cockroach(), "./cockroach", c.Range(1, nodes))

diff --git a/pkg/cmd/roachtest/tests/rebalance_load.go b/pkg/cmd/roachtest/tests/rebalance_load.go
@@ -209,7 +209,6 @@ func registerRebalanceLoad(r registry.Registry) {
 			Name:    `rebalance/by-load/leases/mixed-version`,
 			Owner:   registry.OwnerKV,
 			Cluster: r.MakeClusterSpec(4), // the last node is just used to generate load
-			Leases:  registry.MetamorphicLeases,
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 				if c.IsLocal() {
 					concurrency = 32
@@ -241,7 +240,6 @@ func registerRebalanceLoad(r registry.Registry) {
 			Name:    `rebalance/by-load/replicas/mixed-version`,
 			Owner:   registry.OwnerKV,
 			Cluster: r.MakeClusterSpec(7), // the last node is just used to generate load
-			Leases:  registry.MetamorphicLeases,
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 				if c.IsLocal() {
 					concurrency = 32

diff --git a/pkg/cmd/roachtest/tests/secondary_indexes.go b/pkg/cmd/roachtest/tests/secondary_indexes.go
@@ -139,7 +139,6 @@ func registerSecondaryIndexesMultiVersionCluster(r registry.Registry) {
 		Name:    "schemachange/secondary-index-multi-version",
 		Owner:   registry.OwnerSQLFoundations,
 		Cluster: r.MakeClusterSpec(3),
-		Leases:  registry.MetamorphicLeases,
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 			if c.IsLocal() && runtime.GOARCH == "arm64" {
 				t.Skip("Skip under ARM64. See https://github.com/cockroachdb/cockroach/issues/89268")

diff --git a/pkg/sql/catalog/catalogkeys/commenttype_string.go b/pkg/sql/catalog/catalogkeys/commenttype_string.go
diff --git a/pkg/sql/catalog/catalogkeys/keys.go b/pkg/sql/catalog/catalogkeys/keys.go
@@ -59,10 +59,12 @@ const (
 	SchemaCommentType CommentType = 4
 	// ConstraintCommentType comment on a constraint.
 	ConstraintCommentType CommentType = 5
+	// FunctionCommentType comment on a function.
+	FunctionCommentType CommentType = 6
 
 	// MaxCommentTypeValue is the max possible integer of CommentType type.
 	// Update this whenever a new comment type is added.
-	MaxCommentTypeValue = ConstraintCommentType
+	MaxCommentTypeValue = FunctionCommentType
 )
 
 // AllCommentTypes is a slice of all valid schema comment types.
@@ -73,6 +75,7 @@ var AllCommentTypes = []CommentType{
 	IndexCommentType,
 	SchemaCommentType,
 	ConstraintCommentType,
+	FunctionCommentType,
 }
 
 // IsValidCommentType checks if a given comment type is in the valid value

diff --git a/pkg/sql/catalog/internal/catkv/testdata/testdata b/pkg/sql/catalog/internal/catkv/testdata/testdata
@@ -127,6 +127,7 @@ trace:
 - Scan /Table/24/1/3/108
 - Scan /Table/24/1/4/108
 - Scan /Table/24/1/5/108
+- Scan /Table/24/1/6/108
 - Get /Table/5/1/108/2/1
 
 # Zone config, but no descriptor should be present.
@@ -143,6 +144,7 @@ trace:
 - Scan /Table/24/1/3/0
 - Scan /Table/24/1/4/0
 - Scan /Table/24/1/5/0
+- Scan /Table/24/1/6/0
 - Get /Table/5/1/0/2/1
 
 get_by_ids id=104 id=105 id=106 id=107
@@ -166,6 +168,7 @@ trace:
 - Scan Range /Table/24/1/3/104 /Table/24/1/3/108
 - Scan Range /Table/24/1/4/104 /Table/24/1/4/108
 - Scan Range /Table/24/1/5/104 /Table/24/1/5/108
+- Scan Range /Table/24/1/6/104 /Table/24/1/6/108
 - Scan Range /Table/5/1/104/2/1 /Table/5/1/108/2/1
 
 is_id_in_cache id=107
@@ -436,6 +439,7 @@ trace:
 - Scan /Table/24/1/3/456
 - Scan /Table/24/1/4/456
 - Scan /Table/24/1/5/456
+- Scan /Table/24/1/6/456
 - Get /Table/5/1/456/2/1
 cached:
 - Get /Table/3/1/456/2/1
@@ -445,6 +449,7 @@ cached:
 - Scan /Table/24/1/3/456
 - Scan /Table/24/1/4/456
 - Scan /Table/24/1/5/456
+- Scan /Table/24/1/6/456
 - Get /Table/5/1/456/2/1
 
 get_by_names name_key=(123,456,foo)

diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go
@@ -5435,6 +5435,22 @@ CREATE TABLE crdb_internal.kv_catalog_comments (
 				return err
 			}
 		}
+		// Loop over all builtin function comments.
+		// TODO(rafi): This could be moved directly into the catalog, similar to
+		// virtual table comments.
+		for _, name := range builtins.AllBuiltinNames() {
+			_, overloads := builtinsregistry.GetBuiltinProperties(name)
+			for _, builtin := range overloads {
+				if err := addRow(
+					tree.NewDString(catalogkeys.FunctionCommentType.String()),
+					tree.NewDInt(tree.DInt(builtin.Oid)),
+					tree.DZero,
+					tree.NewDString(builtin.Info),
+				); err != nil {
+					return err
+				}
+			}
+		}
 		return nil
 	},
 }

diff --git a/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog b/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog
@@ -581,6 +581,7 @@ SELECT
   sub_id,
   to_json(regexp_replace(btrim(comment), 'docs\/v\d+\.\d+\/', 'docs/dev/', 'g'))
 FROM "".crdb_internal.kv_catalog_comments
+WHERE type != 'FunctionCommentType'; -- exclude builtin comments since there are so many.
 ----
 DatabaseCommentType    104         0  "this is the test database"
 TableCommentType       111         0  "this is a table"
@@ -918,6 +919,7 @@ SELECT * FROM crdb_internal.kv_catalog_comments WHERE object_id = $kv_id
 TableCommentType       111  0  this is a table
 ConstraintCommentType  111  1  this is a primary key constraint
 ConstraintCommentType  111  2  this is a check constraint
+FunctionCommentType    111  0  Calculates square of the correlation coefficient.
 
 statement ok
 DROP TABLE db.kv CASCADE;
@@ -950,6 +952,7 @@ SELECT * FROM crdb_internal.kv_catalog_comments WHERE object_id = $kv_id
 TableCommentType       111  0  this is a table
 ConstraintCommentType  111  1  this is a primary key constraint
 ConstraintCommentType  111  2  this is a check constraint
+FunctionCommentType    111  0  Calculates square of the correlation coefficient.
 
 query TIIT
 SELECT * FROM system.comments WHERE object_id = $kv_id

diff --git a/pkg/sql/pg_catalog.go b/pkg/sql/pg_catalog.go
@@ -1600,6 +1600,7 @@ func getComments(ctx context.Context, p *planner, descriptorID descpb.ID) ([]tre
 	WHEN 'IndexCommentType' THEN 3
 	WHEN 'SchemaCommentType' THEN 4
 	WHEN 'ConstraintCommentType' THEN 5
+	WHEN 'FunctionCommentType' THEN 6
 	END
 	AS type
 		FROM
@@ -1620,15 +1621,15 @@ WHERE object_id=$1`)
 		queryArgs...)
 }
 
-// populatePgCatalogFromComments populates the for the pg_description table
-// based on a set of fetched comments.
-func populatePgCatalogFromComments(
+// populatePgDescription populates the pg_description table for the given
+// object ID, or for all objects if id==0.
+func populatePgDescription(
 	ctx context.Context,
 	p *planner,
-	dbContext catalog.DatabaseDescriptor,
-	addRow func(...tree.Datum) error,
+	databaseID descpb.ID,
 	id oid.Oid,
-) (populated bool, err error) {
+	addRow func(...tree.Datum) error,
+) (populated bool, retErr error) {
 	comments, err := getComments(ctx, p, descpb.ID(id))
 	if err != nil {
 		return false, err
@@ -1671,7 +1672,7 @@ func populatePgCatalogFromComments(
 			if err != nil {
 				return false, err
 			}
-			objID = getOIDFromConstraint(c, dbContext.GetID(), schema.GetID(), tableDesc)
+			objID = getOIDFromConstraint(c, databaseID, schema.GetID(), tableDesc)
 			objSubID = tree.DZero
 			classOid = tree.NewDOid(catconstants.PgCatalogConstraintTableID)
 		case catalogkeys.IndexCommentType:
@@ -1680,6 +1681,12 @@ func populatePgCatalogFromComments(
 				descpb.IndexID(tree.MustBeDInt(objSubID)))
 			objSubID = tree.DZero
 			classOid = tree.NewDOid(catconstants.PgCatalogClassTableID)
+		case catalogkeys.FunctionCommentType:
+			// TODO: The type conversion to oid.Oid is safe since we use desc IDs
+			// for this, but it's not ideal. The backing column for objId should be
+			// changed to use the OID type.
+			objID = tree.NewDOid(oid.Oid(tree.MustBeDInt(objID)))
+			classOid = tree.NewDOid(catconstants.PgCatalogProcTableID)
 		}
 		if err := addRow(
 			objID,
@@ -1690,38 +1697,7 @@ func populatePgCatalogFromComments(
 		}
 	}
 
-	if id != 0 && populated {
-		return populated, nil
-	}
-
-	// Populate rows based on builtins as well.
-	fmtOverLoad := func(builtin tree.Overload) error {
-		return addRow(
-			tree.NewDOid(builtin.Oid),
-			tree.NewDOid(catconstants.PgCatalogProcTableID),
-			tree.DZero,
-			tree.NewDString(builtin.Info),
-		)
-	}
-	// For direct lookups match with builtins, if this fails
-	// we will do a full scan, since the OID might be an index
-	// or constraint.
-	if id != 0 {
-		builtin, matched := tree.OidToQualifiedBuiltinOverload[id]
-		if !matched {
-			return matched, err
-		}
-		return true, fmtOverLoad(*builtin.Overload)
-	}
-	for _, name := range builtins.AllBuiltinNames() {
-		_, overloads := builtinsregistry.GetBuiltinProperties(name)
-		for _, builtin := range overloads {
-			if err := fmtOverLoad(builtin); err != nil {
-				return false, err
-			}
-		}
-	}
-	return true, nil
+	return populated, nil
 }
 
 var pgCatalogDescriptionTable = virtualSchemaTable{
@@ -1733,15 +1709,17 @@ https://www.postgresql.org/docs/9.5/catalog-pg-description.html`,
 		p *planner,
 		dbContext catalog.DatabaseDescriptor,
 		addRow func(...tree.Datum) error) error {
-		_, err := populatePgCatalogFromComments(ctx, p, dbContext, addRow, 0)
-		return err
+		if _, err := populatePgDescription(ctx, p, dbContext.GetID(), 0 /* all comments */, addRow); err != nil {
+			return err
+		}
+		return nil
 	},
 	indexes: []virtualIndex{
 		{
 			incomplete: true,
 			populate: func(ctx context.Context, unwrappedConstraint tree.Datum, p *planner, dbContext catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) (matched bool, err error) {
 				oid := tree.MustBeDOid(unwrappedConstraint)
-				return populatePgCatalogFromComments(ctx, p, dbContext, addRow, oid.Oid)
+				return populatePgDescription(ctx, p, dbContext.GetID(), oid.Oid, addRow)
 			},
 		},
 	},

diff --git a/pkg/sql/pg_metadata_test.go b/pkg/sql/pg_metadata_test.go
@@ -222,11 +222,11 @@ var none = struct{}{}
 var mappedPopulateFunctions = map[string]string{
 	// Currently pg_type cannot be found automatically by this code because it is
 	// not the populate function. Same for pg_proc.
-	"addPGTypeRow":                  "PGCatalogType",
-	"addPgProcUDFRow":               "PGCatalogProc",
-	"addPgProcBuiltinRow":           "PgCatalogProc",
-	"addRowForTimezoneNames":        "PgCatalogTimezoneNames",
-	"populatePgCatalogFromComments": "PGCatalogDescription",
+	"addPGTypeRow":           "PGCatalogType",
+	"addPgProcUDFRow":        "PGCatalogProc",
+	"addPgProcBuiltinRow":    "PgCatalogProc",
+	"addRowForTimezoneNames": "PgCatalogTimezoneNames",
+	"populatePgDescription":  "PGCatalogDescription",
 }
 
 // schemaCodeFixer have specific configurations to fix the files with virtual
@@ -554,7 +554,7 @@ func (scf schemaCodeFixer) fixCatalogGo(t *testing.T, unimplementedTables PGMeta
 			text := reader.Text()
 			trimText := strings.TrimSpace(text)
 			if trimText == scf.textForNewTableInsertion {
-				//VirtualSchemas doesn't have a particular place to start we just print
+				// VirtualSchemas doesn't have a particular place to start we just print
 				// it before virtualTablePosition.
 				output.appendString(scf.printVirtualSchemas(unimplementedTables))
 			}
@@ -589,7 +589,7 @@ func fixPgCatalogGoColumns(pgCode *pgCatalogCode) {
 			if currentPosition < len(positions) && int64(scannedUntil+count) > positions[currentPosition].insertPosition {
 				relativeIndex := int(positions[currentPosition].insertPosition-int64(scannedUntil)) - 1
 				left := text[:relativeIndex]
-				indentation := indentationRE.FindStringSubmatch(text)[1] //The way it is it should at least give ""
+				indentation := indentationRE.FindStringSubmatch(text)[1] // The way it is it should at least give ""
 				if len(strings.TrimSpace(left)) > 0 {
 					// Parenthesis is right after the last variable in this case
 					// indentation is correct.