Merge #50936

50936: sql,jobs: implement a type schema changer and `RENAME TYPE` r=rohany a=rohany

Fixes #48672.

This PR adds a "type schema changer" that has similar logic to the table
schema changer, but is much simpler. It mostly will just drain names and
wait for leases, but will have important work to do in the `ALTER TYPE
ADD VALUE` case. Using this type schema changer, we implement the
`RENAME TYPE` command.

Release note: None

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

docs/generated/sql/bnf/stmt_block.bnf

@@ -1035,7 +1035,7 @@ alter_type_stmt ::=
 	'ALTER' 'TYPE' type_name 'ADD' 'VALUE' 'SCONST' opt_add_val_placement
 	| 'ALTER' 'TYPE' type_name 'ADD' 'VALUE' 'IF' 'NOT' 'EXISTS' 'SCONST' opt_add_val_placement
 	| 'ALTER' 'TYPE' type_name 'RENAME' 'VALUE' 'SCONST' 'TO' 'SCONST'
-	| 'ALTER' 'TYPE' type_name 'RENAME' 'TO' type_name
+	| 'ALTER' 'TYPE' type_name 'RENAME' 'TO' name
 	| 'ALTER' 'TYPE' type_name 'SET' 'SCHEMA' schema_name
 
 role_or_group_or_user ::=

pkg/base/testing_knobs.go

@@ -19,17 +19,18 @@ type ModuleTestingKnobs interface {
 // TestingKnobs contains facilities for controlling various parts of the
 // system for testing.
 type TestingKnobs struct {
-	Store               ModuleTestingKnobs
-	KVClient            ModuleTestingKnobs
-	SQLExecutor         ModuleTestingKnobs
-	SQLLeaseManager     ModuleTestingKnobs
-	SQLSchemaChanger    ModuleTestingKnobs
-	GCJob               ModuleTestingKnobs
-	PGWireTestingKnobs  ModuleTestingKnobs
-	SQLMigrationManager ModuleTestingKnobs
-	DistSQL             ModuleTestingKnobs
-	SQLEvalContext      ModuleTestingKnobs
-	RegistryLiveness    ModuleTestingKnobs
-	Server              ModuleTestingKnobs
-	TenantTestingKnobs  ModuleTestingKnobs
+	Store                ModuleTestingKnobs
+	KVClient             ModuleTestingKnobs
+	SQLExecutor          ModuleTestingKnobs
+	SQLLeaseManager      ModuleTestingKnobs
+	SQLSchemaChanger     ModuleTestingKnobs
+	SQLTypeSchemaChanger ModuleTestingKnobs
+	GCJob                ModuleTestingKnobs
+	PGWireTestingKnobs   ModuleTestingKnobs
+	SQLMigrationManager  ModuleTestingKnobs
+	DistSQL              ModuleTestingKnobs
+	SQLEvalContext       ModuleTestingKnobs
+	RegistryLiveness     ModuleTestingKnobs
+	Server               ModuleTestingKnobs
+	TenantTestingKnobs   ModuleTestingKnobs
 }

pkg/jobs/jobspb/jobs.proto

@@ -151,7 +151,7 @@ message ImportDetails {
   // corresponding to this job. While the job ought to clean up the record
   // when it enters a terminal state, there may be cases where it cannot or
   // does not run the code to do so. To deal with this there is a background
-  // reconcilliation loop to ensure that protected timestamps are cleaned up.
+  // reconciliation loop to ensure that protected timestamps are cleaned up.
   bytes protected_timestamp_record = 22 [
     (gogoproto.customname) = "ProtectedTimestampRecord",
     (gogoproto.customtype) = "github.com/cockroachdb/cockroach/pkg/util/uuid.UUID"
@@ -173,6 +173,16 @@ message ImportProgress {
   repeated int64 resume_pos = 5; // Only set by direct import.
 }
 
+// TypeSchemaChangeDetails is the job detail information for a type schema change job.
+message TypeSchemaChangeDetails {
+  uint32 type_id = 1 [(gogoproto.customname) = "TypeID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sqlbase.ID"];
+}
+
+// TypeSchemaChangeProgress is the persisted progress for a type schema change job.
+message TypeSchemaChangeProgress {
+
+}
+
 message ResumeSpanList {
   repeated roachpb.Span resume_spans = 1 [(gogoproto.nullable) = false];
 }
@@ -420,6 +430,7 @@ message Payload {
     ChangefeedDetails changefeed = 14;
     CreateStatsDetails createStats = 15;
     SchemaChangeGCDetails schemaChangeGC = 21;
+    TypeSchemaChangeDetails typeSchemaChange = 22;
   }
 }
 
@@ -439,6 +450,7 @@ message Progress {
     ChangefeedProgress changefeed = 14;
     CreateStatsProgress createStats = 15;
     SchemaChangeGCProgress schemaChangeGC = 16;
+    TypeSchemaChangeProgress typeSchemaChange = 17;
   }
 }
 
@@ -455,6 +467,9 @@ enum Type {
   CREATE_STATS = 6 [(gogoproto.enumvalue_customname) = "TypeCreateStats"];
   AUTO_CREATE_STATS = 7 [(gogoproto.enumvalue_customname) = "TypeAutoCreateStats"];
   SCHEMA_CHANGE_GC = 8 [(gogoproto.enumvalue_customname) = "TypeSchemaChangeGC"];
+  // We can't name this TYPE_SCHEMA_CHANGE due to how proto generates actual
+  // names for this enum, which cause a conflict with the SCHEMA_CHANGE entry.
+  TYPEDESC_SCHEMA_CHANGE = 9 [(gogoproto.enumvalue_customname) = "TypeTypeSchemaChange"];
 }
 
 message Job {

pkg/jobs/jobspb/wrap.go

@@ -64,6 +64,8 @@ func DetailsType(d isPayload_Details) Type {
 		return TypeCreateStats
 	case *Payload_SchemaChangeGC:
 		return TypeSchemaChangeGC
+	case *Payload_TypeSchemaChange:
+		return TypeTypeSchemaChange
 	default:
 		panic(fmt.Sprintf("Payload.Type called on a payload with an unknown details type: %T", d))
 	}
@@ -92,6 +94,8 @@ func WrapProgressDetails(details ProgressDetails) interface {
 		return &Progress_CreateStats{CreateStats: &d}
 	case SchemaChangeGCProgress:
 		return &Progress_SchemaChangeGC{SchemaChangeGC: &d}
+	case TypeSchemaChangeProgress:
+		return &Progress_TypeSchemaChange{TypeSchemaChange: &d}
 	default:
 		panic(fmt.Sprintf("WrapProgressDetails: unknown details type %T", d))
 	}
@@ -115,6 +119,8 @@ func (p *Payload) UnwrapDetails() Details {
 		return *d.CreateStats
 	case *Payload_SchemaChangeGC:
 		return *d.SchemaChangeGC
+	case *Payload_TypeSchemaChange:
+		return *d.TypeSchemaChange
 	default:
 		return nil
 	}
@@ -138,6 +144,8 @@ func (p *Progress) UnwrapDetails() ProgressDetails {
 		return *d.CreateStats
 	case *Progress_SchemaChangeGC:
 		return *d.SchemaChangeGC
+	case *Progress_TypeSchemaChange:
+		return *d.TypeSchemaChange
 	default:
 		return nil
 	}
@@ -174,6 +182,8 @@ func WrapPayloadDetails(details Details) interface {
 		return &Payload_CreateStats{CreateStats: &d}
 	case SchemaChangeGCDetails:
 		return &Payload_SchemaChangeGC{SchemaChangeGC: &d}
+	case TypeSchemaChangeDetails:
+		return &Payload_TypeSchemaChange{TypeSchemaChange: &d}
 	default:
 		panic(fmt.Sprintf("jobs.WrapPayloadDetails: unknown details type %T", d))
 	}

pkg/server/server_sql.go

@@ -474,6 +474,11 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*sqlServer, error) {
 	} else {
 		execCfg.SchemaChangerTestingKnobs = new(sql.SchemaChangerTestingKnobs)
 	}
+	if sqlTypeSchemaChangerTestingKnobs := cfg.TestingKnobs.SQLTypeSchemaChanger; sqlTypeSchemaChangerTestingKnobs != nil {
+		execCfg.TypeSchemaChangerTestingKnobs = sqlTypeSchemaChangerTestingKnobs.(*sql.TypeSchemaChangerTestingKnobs)
+	} else {
+		execCfg.TypeSchemaChangerTestingKnobs = new(sql.TypeSchemaChangerTestingKnobs)
+	}
 	if gcJobTestingKnobs := cfg.TestingKnobs.GCJob; gcJobTestingKnobs != nil {
 		execCfg.GCJobTestingKnobs = gcJobTestingKnobs.(*sql.GCJobTestingKnobs)
 	} else {

pkg/sql/alter_type.go

@@ -13,35 +13,148 @@ package sql
 import (
 	"context"
 
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
 	"github.com/cockroachdb/errors"
 )
 
 type alterTypeNode struct {
-	n *tree.AlterType
+	n    *tree.AlterType
+	desc *sqlbase.MutableTypeDescriptor
 }
 
 // alterTypeNode implements planNode. We set n here to satisfy the linter.
 var _ planNode = &alterTypeNode{n: nil}
 
 func (p *planner) AlterType(ctx context.Context, n *tree.AlterType) (planNode, error) {
-	return &alterTypeNode{n: n}, nil
+	// Resolve the type.
+	desc, err := p.ResolveMutableTypeDescriptor(ctx, n.Type, true /* required */)
+	if err != nil {
+		return nil, err
+	}
+	// The implicit array types are not modifiable.
+	if desc.Kind == sqlbase.TypeDescriptor_ALIAS {
+		return nil, pgerror.Newf(
+			pgcode.WrongObjectType,
+			"%q is an implicit array type and cannot be modified",
+			tree.AsStringWithFQNames(n.Type, &p.semaCtx.Annotations),
+		)
+	}
+	// TODO (rohany): Check permissions here once we track them.
+	return &alterTypeNode{
+		n:    n,
+		desc: desc,
+	}, nil
 }
 
 func (n *alterTypeNode) startExec(params runParams) error {
+	var err error
 	switch t := n.n.Cmd.(type) {
 	case *tree.AlterTypeAddValue:
-		return unimplemented.NewWithIssue(48670, "ALTER TYPE ADD VALUE unsupported")
+		err = unimplemented.NewWithIssue(48670, "ALTER TYPE ADD VALUE unsupported")
 	case *tree.AlterTypeRenameValue:
-		return unimplemented.NewWithIssue(48697, "ALTER TYPE RENAME VALUE unsupported")
+		err = unimplemented.NewWithIssue(48697, "ALTER TYPE RENAME VALUE unsupported")
 	case *tree.AlterTypeRename:
-		return unimplemented.NewWithIssue(48671, "ALTER TYPE RENAME unsupported")
+		err = params.p.renameType(params, n, t.NewName)
 	case *tree.AlterTypeSetSchema:
-		return unimplemented.NewWithIssue(48672, "ALTER TYPE SET SCHEMA unsupported")
+		err = unimplemented.NewWithIssue(48672, "ALTER TYPE SET SCHEMA unsupported")
 	default:
-		return errors.AssertionFailedf("unknown alter type cmd %s", t)
+		err = errors.AssertionFailedf("unknown alter type cmd %s", t)
+	}
+	if err != nil {
+		return err
+	}
+	return n.desc.Validate(params.ctx, params.p.txn, params.ExecCfg().Codec)
+}
+
+func (p *planner) renameType(params runParams, n *alterTypeNode, newName string) error {
+	// See if there is a name collision with the new name.
+	exists, id, err := sqlbase.LookupObjectID(
+		params.ctx,
+		p.txn,
+		p.ExecCfg().Codec,
+		n.desc.ParentID,
+		n.desc.ParentSchemaID,
+		newName,
+	)
+	if err == nil && exists {
+		// Try and see what kind of object we collided with.
+		desc, err := catalogkv.GetDescriptorByID(params.ctx, p.txn, p.ExecCfg().Codec, id)
+		if err != nil {
+			return sqlbase.WrapErrorWhileConstructingObjectAlreadyExistsErr(err)
+		}
+		return sqlbase.MakeObjectAlreadyExistsError(desc.DescriptorProto(), newName)
+	} else if err != nil {
+		return err
+	}
+
+	// Rename the base descriptor.
+	if err := p.performRenameTypeDesc(
+		params.ctx,
+		n.desc,
+		newName,
+		tree.AsStringWithFQNames(n.n, params.Ann()),
+	); err != nil {
+		return err
+	}
+
+	// Now rename the array type.
+	newArrayName, err := findFreeArrayTypeName(
+		params.ctx,
+		p.txn,
+		p.ExecCfg().Codec,
+		n.desc.ParentID,
+		n.desc.ParentSchemaID,
+		newName,
+	)
+	if err != nil {
+		return err
+	}
+	// TODO (rohany): This should use a method on the desc collection instead.
+	arrayDesc, err := sqlbase.GetTypeDescFromID(params.ctx, p.txn, p.ExecCfg().Codec, n.desc.ArrayTypeID)
+	if err != nil {
+		return err
+	}
+	if err := p.performRenameTypeDesc(
+		params.ctx,
+		sqlbase.NewMutableExistingTypeDescriptor(*arrayDesc.TypeDesc()),
+		newArrayName,
+		tree.AsStringWithFQNames(n.n, params.Ann()),
+	); err != nil {
+		return err
+	}
+	return nil
+}
+
+func (p *planner) performRenameTypeDesc(
+	ctx context.Context, desc *sqlbase.MutableTypeDescriptor, newName string, jobDesc string,
+) error {
+	// Record the rename details in the descriptor for draining.
+	desc.DrainingNames = append(desc.DrainingNames, sqlbase.NameInfo{
+		ParentID:       desc.ParentID,
+		ParentSchemaID: desc.ParentSchemaID,
+		Name:           desc.Name,
+	})
+	// Set the descriptor up with the new name.
+	desc.Name = newName
+	if err := p.writeTypeChange(ctx, desc, jobDesc); err != nil {
+		return err
 	}
+	// Construct the new namespace key.
+	b := p.txn.NewBatch()
+	key := sqlbase.MakeObjectNameKey(
+		ctx,
+		p.ExecCfg().Settings,
+		desc.ParentID,
+		desc.ParentSchemaID,
+		newName,
+	).Key(p.ExecCfg().Codec)
+	b.CPut(key, desc.ID, nil /* expected */)
+	return p.txn.Run(ctx, b)
 }
 
 func (n *alterTypeNode) Next(params runParams) (bool, error) { return false, nil }

pkg/sql/backfill.go

@@ -383,11 +383,11 @@ func (sc *SchemaChanger) dropConstraints(
 	if err != nil {
 		return nil, err
 	}
-	if err := sc.waitToUpdateLeases(ctx, sc.tableID); err != nil {
+	if err := waitToUpdateLeases(ctx, sc.leaseMgr, sc.tableID); err != nil {
 		return nil, err
 	}
 	for id := range fksByBackrefTable {
-		if err := sc.waitToUpdateLeases(ctx, id); err != nil {
+		if err := waitToUpdateLeases(ctx, sc.leaseMgr, id); err != nil {
 			return nil, err
 		}
 	}
@@ -490,11 +490,11 @@ func (sc *SchemaChanger) addConstraints(
 	if _, err := sc.leaseMgr.PublishMultiple(ctx, tableIDsToUpdate, update, nil); err != nil {
 		return err
 	}
-	if err := sc.waitToUpdateLeases(ctx, sc.tableID); err != nil {
+	if err := waitToUpdateLeases(ctx, sc.leaseMgr, sc.tableID); err != nil {
 		return err
 	}
 	for id := range fksByBackrefTable {
-		if err := sc.waitToUpdateLeases(ctx, id); err != nil {
+		if err := waitToUpdateLeases(ctx, sc.leaseMgr, id); err != nil {
 			return err
 		}
 	}

pkg/sql/catalog/catalog.go

@@ -29,6 +29,8 @@ type MutableDescriptor interface {
 	// descriptor should increment the version on the mutable copy from the
 	// outset.
 	MaybeIncrementVersion()
+	// SetDrainingNames sets the draining names for the descriptor.
+	SetDrainingNames([]sqlbase.NameInfo)
 	// Immutable returns an immutable copy of this descriptor.
 	Immutable() Descriptor
 }

pkg/sql/catalog/resolver/resolver.go

@@ -123,6 +123,25 @@ func ResolveMutableExistingTableObject(
 	return desc.(*sqlbase.MutableTableDescriptor), nil
 }
 
+// ResolveMutableType resolves a type descriptor for mutable access. It
+// returns the resolved descriptor, as well as the fully qualified resolved
+// object name.
+func ResolveMutableType(
+	ctx context.Context, sc SchemaResolver, un *tree.UnresolvedObjectName, required bool,
+) (*tree.TypeName, *sqlbase.MutableTypeDescriptor, error) {
+	lookupFlags := tree.ObjectLookupFlags{
+		CommonLookupFlags: tree.CommonLookupFlags{Required: required},
+		RequireMutable:    true,
+		DesiredObjectKind: tree.TypeObject,
+	}
+	desc, prefix, err := ResolveExistingObject(ctx, sc, un, lookupFlags)
+	if err != nil || desc == nil {
+		return nil, nil, err
+	}
+	tn := tree.MakeTypeNameFromPrefix(prefix, tree.Name(un.Object()))
+	return &tn, desc.(*sqlbase.MutableTypeDescriptor), nil
+}
+
 // ResolveExistingObject resolves an object with the given flags.
 func ResolveExistingObject(
 	ctx context.Context,