Merge #49656 #49699

49656: sql: fix panic when attempting to clone a table with a UDT r=rohany a=rohany

Due to how the type metadata was represented, there could have
been structs with unimplemented fields in the `TypeMeta`'s
`UserDefinedTypeName`. This would cause protobuf to panic when
attempting to perform reflection into `types.T` when cloning.

Release note: None

49699: Fixed code rot in builder README r=aaron-crl a=aaron-crl

Updated Dockerfile path in README.

Co-authored-by: Rohan Yadav <[email protected]>
Co-authored-by: Aaron Blum <[email protected]>

build/README.md

@@ -56,7 +56,7 @@ which may or may not work (and are not officially supported).
 
 ## Basic Process
 
-- Edit `build/Dockerfile` as desired
+- Edit `build/builder/Dockerfile` as desired
 - Run `build/builder.sh init` to test -- this will build the image locally. Beware this can take a lot of time. The result of `init` is a docker image version which you can subsequently stick into the `version` variable inside the `builder.sh` script for testing locally.
 - Once you are happy with the result, run `build/builder.sh push` which pushes your image towards Docker hub, so that it becomes available to others. The result is again a version number, which you then *must* copy back into `builder.sh`. Then commit the change to both Dockerfile and `builder.sh` and submit a PR.
 - Finally, use this version number to update the `builder.dockerImage` configuration parameter in TeamCity under the [`Cockroach`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Cockroach&tab=projectParams) and [`Internal`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Internal&tab=projectParams) projects.

pkg/sql/sem/tree/type_name.go

@@ -67,16 +67,6 @@ func (t *TypeName) FQString() string {
 
 func (t *TypeName) objectName() {}
 
-// Basename implements the types.UserDefinedTypeName interface.
-func (t *TypeName) Basename() string {
-	return t.Type()
-}
-
-// FQName implements the types.UserDefinedTypeName interface.
-func (t *TypeName) FQName() string {
-	return t.FQString()
-}
-
 // NewUnqualifiedTypeName returns a new base type name.
 func NewUnqualifiedTypeName(typ Name) *TypeName {
 	return &TypeName{objName{

pkg/sql/sqlbase/structured.go

@@ -4405,7 +4405,7 @@ func (desc *TypeDescriptor) HydrateTypeInfo(typ *types.T) error {
 // a type and also sets the name in the metadata to the passed in name.
 // This is used when hydrating a type with a known qualified name.
 func (desc *TypeDescriptor) HydrateTypeInfoWithName(typ *types.T, name *tree.TypeName) error {
-	typ.TypeMeta.Name = name
+	typ.TypeMeta.Name = types.MakeUserDefinedTypeName(name.Catalog(), name.Schema(), name.Object())
 	switch desc.Kind {
 	case TypeDescriptor_ENUM:
 		if typ.Family() != types.EnumFamily {

pkg/sql/table_test.go

@@ -15,10 +15,15 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
+	"github.com/cockroachdb/cockroach/pkg/sql/tests"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 )
 
 func TestMakeTableDescColumns(t *testing.T) {
@@ -309,3 +314,33 @@ func TestPrimaryKeyUnspecified(t *testing.T) {
 		t.Fatalf("unexpected error: %v", err)
 	}
 }
+
+func TestCanCloneTableWithUDT(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	ctx := context.Background()
+	params, _ := tests.CreateTestServerParams()
+	s, sqlDB, kvDB := serverutils.StartServer(t, params)
+	defer s.Stopper().Stop(ctx)
+	if _, err := sqlDB.Exec(`
+SET experimental_enable_enums=true;
+CREATE DATABASE test;
+CREATE TYPE test.t AS ENUM ('hello');
+CREATE TABLE test.tt (x test.t);
+`); err != nil {
+		t.Fatal(err)
+	}
+	desc := sqlbase.GetTableDescriptor(kvDB, keys.SystemSQLCodec, "test", "tt")
+	typLookup := func(id sqlbase.ID) (*tree.TypeName, *sqlbase.TypeDescriptor, error) {
+		typDesc, err := sqlbase.GetTypeDescFromID(ctx, kvDB, keys.SystemSQLCodec, id)
+		if err != nil {
+			return nil, nil, err
+		}
+		return &tree.TypeName{}, typDesc, nil
+	}
+	if err := sqlbase.HydrateTypesInTableDescriptor(desc, typLookup); err != nil {
+		t.Fatal(err)
+	}
+	// Ensure that we can clone this table.
+	_ = protoutil.Clone(desc).(*TableDescriptor)
+}

pkg/sql/types/types.go

@@ -185,7 +185,8 @@ type T struct {
 // UserDefinedTypeMetadata contains metadata needed for runtime
 // operations on user defined types. The metadata must be read only.
 type UserDefinedTypeMetadata struct {
-	Name UserDefinedTypeName
+	// Name is the resolved name of this type.
+	Name *UserDefinedTypeName
 
 	// enumData is non-nil iff the metadata is for an ENUM type.
 	EnumData *EnumMetadata
@@ -212,13 +213,40 @@ func (e *EnumMetadata) debugString() string {
 	)
 }
 
-// UserDefinedTypeName is an interface for the types package to manipulate
-// qualified type names from higher level packages for name display.
-type UserDefinedTypeName interface {
-	// Basename returns the unqualified name of the type.
-	Basename() string
-	// FQName returns the fully qualified name of the type.
-	FQName() string
+// UserDefinedTypeName is a struct representing a qualified user defined
+// type name. We redefine a common struct from higher level packages. We
+// do so because proto will panic if any members of a proto struct are
+// private. Rather than expose private members of higher level packages,
+// we define a separate type here to be safe.
+type UserDefinedTypeName struct {
+	Catalog string
+	Schema  string
+	Name    string
+}
+
+// MakeUserDefinedTypeName creates a user defined type name.
+func MakeUserDefinedTypeName(catalog, schema, name string) *UserDefinedTypeName {
+	return &UserDefinedTypeName{
+		Catalog: catalog,
+		Schema:  schema,
+		Name:    name,
+	}
+}
+
+// Basename returns the unqualified name.
+func (u UserDefinedTypeName) Basename() string {
+	return u.Name
+}
+
+// FQName returns the fully qualified name.
+func (u UserDefinedTypeName) FQName() string {
+	var sb strings.Builder
+	sb.WriteString(u.Catalog)
+	sb.WriteString(".")
+	sb.WriteString(u.Schema)
+	sb.WriteString(".")
+	sb.WriteString(u.Name)
+	return sb.String()
 }
 
 // Convenience list of pre-constructed types. Caller code can use any of these