From 2aab5525fe07c4b4679347b61dbc2a0a396ec5a7 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Mon, 15 May 2023 09:37:22 +0000 Subject: [PATCH 1/3] roachtest: implement `String()` for `DefaultLeases` Epic: none Release note: None --- pkg/cmd/roachtest/registry/test_spec.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/cmd/roachtest/registry/test_spec.go b/pkg/cmd/roachtest/registry/test_spec.go index fdfafc14511e..0ab8f083f4ac 100644 --- 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: From e5e5a99059dbe634981e09bdb49e8971cd06b5f6 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Mon, 15 May 2023 09:03:30 +0000 Subject: [PATCH 2/3] roachtest: disable metamorphic expiration leases in some tests 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` Epic: none Release note: None --- pkg/cmd/roachtest/tests/decommission.go | 1 - pkg/cmd/roachtest/tests/follower_reads.go | 1 - pkg/cmd/roachtest/tests/import.go | 1 - pkg/cmd/roachtest/tests/kv.go | 12 ++++++++---- pkg/cmd/roachtest/tests/rebalance_load.go | 2 -- pkg/cmd/roachtest/tests/secondary_indexes.go | 1 - 6 files changed, 8 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/roachtest/tests/decommission.go b/pkg/cmd/roachtest/tests/decommission.go index 64294203d982..40d9625eb93b 100644 --- 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 index 860174a36be0..6ed247300866 100644 --- 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 index e6bd1915c018..8b07bdbb2509 100644 --- 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 index 3d8dcf1f17f0..2f30cab6971e 100644 --- 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 index d39346eb774a..154b9abc2e04 100644 --- 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 index 46426458e83d..c43548535bb3 100644 --- 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.OwnerSQLSchema, 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") From 5ef16bde0b19b7439db328dbfc04d79a2503daa6 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Sun, 14 May 2023 00:51:12 -0400 Subject: [PATCH 3/3] sql: clean up code for pg_description 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. Release note: None --- .../catalog/catalogkeys/commenttype_string.go | 5 +- pkg/sql/catalog/catalogkeys/keys.go | 5 +- .../catalog/internal/catkv/testdata/testdata | 5 ++ pkg/sql/crdb_internal.go | 16 +++++ .../testdata/logic_test/crdb_internal_catalog | 3 + pkg/sql/pg_catalog.go | 62 ++++++------------- pkg/sql/pg_metadata_test.go | 14 ++--- 7 files changed, 59 insertions(+), 51 deletions(-) diff --git a/pkg/sql/catalog/catalogkeys/commenttype_string.go b/pkg/sql/catalog/catalogkeys/commenttype_string.go index ccff62116c09..1dc344ccffc4 100644 --- a/pkg/sql/catalog/catalogkeys/commenttype_string.go +++ b/pkg/sql/catalog/catalogkeys/commenttype_string.go @@ -14,7 +14,8 @@ func _() { _ = x[IndexCommentType-3] _ = x[SchemaCommentType-4] _ = x[ConstraintCommentType-5] - _ = x[MaxCommentTypeValue-5] + _ = x[FunctionCommentType-6] + _ = x[MaxCommentTypeValue-6] } func (i CommentType) String() string { @@ -31,6 +32,8 @@ func (i CommentType) String() string { return "SchemaCommentType" case ConstraintCommentType: return "ConstraintCommentType" + case FunctionCommentType: + return "FunctionCommentType" default: return "CommentType(" + strconv.FormatInt(int64(i), 10) + ")" } diff --git a/pkg/sql/catalog/catalogkeys/keys.go b/pkg/sql/catalog/catalogkeys/keys.go index e62265682bc7..5dd2b4be371f 100644 --- 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 index f58bde2eb2a7..293d951845e8 100644 --- 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 index 3cf08cefbc5d..96e901747718 100644 --- 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 index 24ba27134fd3..8402312740b1 100644 --- 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 index d10d8fdb1016..dd9095569f5c 100644 --- 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 index 7c7ddbeb6a73..34c73ae998c4 100644 --- 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.