Merge #128084 #128715

128084: sql: SHOW CREATE ALL TYPES doesn't work with comment on r=Dedej-Bergin a=Dedej-Bergin There was a bug where comments did not show up in the output of the show create all types command. The issue was that we did not have any code to get the comment and add it to the create type descriptor. I added these changes and also did a bit of cleanup/removed duplicate code in the process. Fixes: #126005 Release note (bug fix): SHOW CREATE ALL TYPES now shows corresponding type comments in it's output. 128715: telemetry: remove expectation of no force-custom counter r=mgartner,kyle-a-wong a=dhartunian This test flakes in cases where we run a query and expect the `sql.plan.type.force-custom` to not get incremented. This can't be guaranteed as this is the default counter and it occasionally gets bumped by background operations. There's no easy way to prevent these from happening so these cases are removed from this suite. Resolves: #128523, #128640 Epic: None Release note: None Co-authored-by: Bergin Dedej <[email protected]> Co-authored-by: David Hartunian <[email protected]>
cockroachdb · Aug 12, 2024 · 5dbc891 · 5dbc891
3 parents 6bae06f + 175a9db + 38665d9
commit 5dbc891
Show file tree

Hide file tree

Showing 3 changed files with 99 additions and 54 deletions.
diff --git a/pkg/ccl/telemetryccl/testdata/telemetry/generic b/pkg/ccl/telemetryccl/testdata/telemetry/generic
@@ -54,15 +54,6 @@ EXECUTE p_join(1)
 ----
 sql.plan.type.force-custom
 
-# Non-prepared statements do not increment plan type counters.
-feature-usage
-SELECT * FROM kv WHERE v = 100
-----
-
-feature-usage
-SELECT * FROM kv WHERE v = 100
-----
-
 feature-list
 sql.plan.type.force-generic
 ----

diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go
@@ -3466,10 +3466,17 @@ func writeCreateTypeDescRow(
 	ctx context.Context,
 	db catalog.DatabaseDescriptor,
 	sc catalog.SchemaDescriptor,
-	resolver tree.TypeReferenceResolver,
+	p *planner,
 	typeDesc catalog.TypeDescriptor,
 	addRow func(...tree.Datum) error,
 ) (written bool, err error) {
+	var typeVariety tree.CreateTypeVariety
+	var typeList []tree.CompositeTypeElem
+	var enumLabels tree.EnumValueList
+	enumLabelsDatum := tree.NewDArray(types.String)
+	resolver := p.semaCtx.TypeResolver
+	descriptors := p.descCollection
+
 	if typeDesc.AsAliasTypeDescriptor() != nil {
 		// Alias types are created implicitly, so we don't have create
 		// statements for them.
@@ -3481,40 +3488,17 @@ func writeCreateTypeDescRow(
 			// statements for them.
 			return false, nil
 		}
-		var enumLabels tree.EnumValueList
-		enumLabelsDatum := tree.NewDArray(types.String)
+
 		for i := 0; i < e.NumEnumMembers(); i++ {
 			rep := e.GetMemberLogicalRepresentation(i)
 			enumLabels = append(enumLabels, tree.EnumValue(rep))
 			if err := enumLabelsDatum.Append(tree.NewDString(rep)); err != nil {
 				return false, err
 			}
 		}
-		name, err := tree.NewUnresolvedObjectName(2, [3]string{e.GetName(), sc.GetName()}, 0)
-		if err != nil {
-			return false, err
-		}
-		node := &tree.CreateType{
-			Variety:    tree.Enum,
-			TypeName:   name,
-			EnumLabels: enumLabels,
-		}
-		return true, addRow(
-			tree.NewDInt(tree.DInt(db.GetID())),  // database_id
-			tree.NewDString(db.GetName()),        // database_name
-			tree.NewDString(sc.GetName()),        // schema_name
-			tree.NewDInt(tree.DInt(e.GetID())),   // descriptor_id
-			tree.NewDString(e.GetName()),         // descriptor_name
-			tree.NewDString(tree.AsString(node)), // create_statement
-			enumLabelsDatum,
-		)
-	}
-	if c := typeDesc.AsCompositeTypeDescriptor(); c != nil {
-		name, err := tree.NewUnresolvedObjectName(2, [3]string{c.GetName(), sc.GetName()}, 0)
-		if err != nil {
-			return false, err
-		}
-		typeList := make([]tree.CompositeTypeElem, c.NumElements())
+		typeVariety = tree.Enum
+	} else if c := typeDesc.AsCompositeTypeDescriptor(); c != nil {
+		typeList = make([]tree.CompositeTypeElem, c.NumElements())
 		for i := 0; i < c.NumElements(); i++ {
 			t := c.GetElementType(i)
 			if err := typedesc.EnsureTypeIsHydrated(
@@ -3525,22 +3509,39 @@ func writeCreateTypeDescRow(
 			typeList[i].Type = t
 			typeList[i].Label = tree.Name(c.GetElementLabel(i))
 		}
-		node := &tree.CreateType{
-			Variety:           tree.Composite,
-			TypeName:          name,
-			CompositeTypeList: typeList,
-		}
-		return true, addRow(
-			tree.NewDInt(tree.DInt(db.GetID())),  // database_id
-			tree.NewDString(db.GetName()),        // database_name
-			tree.NewDString(sc.GetName()),        // schema_name
-			tree.NewDInt(tree.DInt(c.GetID())),   // descriptor_id
-			tree.NewDString(c.GetName()),         // descriptor_name
-			tree.NewDString(tree.AsString(node)), // create_statement
-			tree.DNull,                           // enum_members
-		)
+		typeVariety = tree.Composite
+	} else {
+		return false, errors.AssertionFailedf("unknown type descriptor kind %s", typeDesc.GetKind())
+	}
+
+	name, err := tree.NewUnresolvedObjectName(2, [3]string{typeDesc.GetName(), sc.GetName()}, 0)
+	if err != nil {
+		return false, err
+	}
+	node := &tree.CreateType{
+		Variety:           typeVariety,
+		TypeName:          name,
+		CompositeTypeList: typeList,
+		EnumLabels:        enumLabels,
 	}
-	return false, errors.AssertionFailedf("unknown type descriptor kind %s", typeDesc.GetKind())
+
+	createStatement := tree.AsString(node)
+
+	comment, ok := descriptors.GetTypeComment(typeDesc.GetID())
+	if ok {
+		commentOnType := tree.CommentOnType{Comment: &comment, Name: name}
+		createStatement += ";\n" + tree.AsString(&commentOnType)
+	}
+
+	return true, addRow(
+		tree.NewDInt(tree.DInt(db.GetID())),       // database_id
+		tree.NewDString(db.GetName()),             // database_name
+		tree.NewDString(sc.GetName()),             // schema_name
+		tree.NewDInt(tree.DInt(typeDesc.GetID())), // descriptor_id
+		tree.NewDString(typeDesc.GetName()),       // descriptor_name
+		tree.NewDString(createStatement),          // create_statement
+		enumLabelsDatum,                           // empty for composite types
+	)
 }
 
 var crdbInternalCreateTypeStmtsTable = virtualSchemaTable{
@@ -3559,7 +3560,8 @@ CREATE TABLE crdb_internal.create_type_statements (
 `,
 	populate: func(ctx context.Context, p *planner, db catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
 		return forEachTypeDesc(ctx, p, db, func(ctx context.Context, db catalog.DatabaseDescriptor, sc catalog.SchemaDescriptor, typeDesc catalog.TypeDescriptor) error {
-			_, err := writeCreateTypeDescRow(ctx, db, sc, p.semaCtx.TypeResolver, typeDesc, addRow)
+			_, err := writeCreateTypeDescRow(ctx, db, sc, p, typeDesc, addRow)
+
 			return err
 		})
 	},
@@ -3577,7 +3579,7 @@ CREATE TABLE crdb_internal.create_type_statements (
 				if err != nil || typDesc == nil {
 					return false, err
 				}
-				return writeCreateTypeDescRow(ctx, db, scName, p.semaCtx.TypeResolver, typDesc, addRow)
+				return writeCreateTypeDescRow(ctx, db, scName, p, typDesc, addRow)
 			},
 		},
 	},

diff --git a/pkg/sql/logictest/testdata/logic_test/show_create_all_types b/pkg/sql/logictest/testdata/logic_test/show_create_all_types
@@ -62,3 +62,55 @@ statement ok
 CREATE DATABASE "a""bc";
 USE "a""bc";
 SHOW CREATE ALL TYPES;
+
+subtest bug-show-create-all-types-#126005
+
+skipif config local-legacy-schema-changer
+skipif config local-mixed-24.1
+statement ok
+CREATE TYPE address AS (
+        street STRING,
+        city STRING,
+        state STRING,
+        zipcode STRING
+    );
+
+skipif config local-legacy-schema-changer
+skipif config local-mixed-24.1
+statement ok
+COMMENT ON TYPE address IS 'comment for composite type address';
+
+skipif config local-legacy-schema-changer
+skipif config local-mixed-24.1
+query T colnames
+SHOW CREATE ALL TYPES
+----
+create_statement
+CREATE TYPE public.address AS (street STRING, city STRING, state STRING, zipcode STRING);
+COMMENT ON TYPE public.address IS 'comment for composite type address';
+
+skipif config local-legacy-schema-changer
+skipif config local-mixed-24.1
+statement ok
+DROP TYPE address;
+
+skipif config local-legacy-schema-changer
+skipif config local-mixed-24.1
+statement ok
+CREATE TYPE roaches AS ENUM('papa_roach','mama_roach','baby_roach');
+
+skipif config local-legacy-schema-changer
+skipif config local-mixed-24.1
+statement ok
+COMMENT ON TYPE roaches IS 'comment for enum type roaches';
+
+skipif config local-legacy-schema-changer
+skipif config local-mixed-24.1
+query T colnames
+SHOW CREATE ALL TYPES
+----
+create_statement
+CREATE TYPE public.roaches AS ENUM ('papa_roach', 'mama_roach', 'baby_roach');
+COMMENT ON TYPE public.roaches IS 'comment for enum type roaches';
+
+subtest end