release-20.2: sql: prevent DROP SCHEMA CASCADE from droping types with references

pkg/sql/drop_cascade.go

@@ -22,6 +22,7 @@ import (
 	"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/util/errorutil/unimplemented"
 	"github.com/cockroachdb/errors"
 )
 
@@ -31,6 +32,7 @@ type dropCascadeState struct {
 	objectNamesToDelete []tree.ObjectName
 
 	td                      []toDelete
+	toDeleteByID            map[descpb.ID]*toDelete
 	allTableObjectsToDelete []*tabledesc.Mutable
 	typesToDelete           []*typedesc.Mutable
 
@@ -165,6 +167,10 @@ func (d *dropCascadeState) resolveCollectedObjects(
 	}
 	d.allTableObjectsToDelete = allObjectsToDelete
 	d.td = filterImplicitlyDeletedObjects(d.td, implicitDeleteMap)
+	d.toDeleteByID = make(map[descpb.ID]*toDelete)
+	for i := range d.td {
+		d.toDeleteByID[d.td[i].desc.GetID()] = &d.td[i]
+	}
 	return nil
 }
 
@@ -192,6 +198,9 @@ func (d *dropCascadeState) dropAllCollectedObjects(ctx context.Context, p *plann
 
 	// Now delete all of the types.
 	for _, typ := range d.typesToDelete {
+		if err := d.canDropType(ctx, p, typ); err != nil {
+			return err
+		}
 		// Drop the types. Note that we set queueJob to be false because the types
 		// will be dropped in bulk as part of the DROP DATABASE job.
 		if err := p.dropTypeImpl(ctx, typ, "", false /* queueJob */); err != nil {
@@ -202,6 +211,37 @@ func (d *dropCascadeState) dropAllCollectedObjects(ctx context.Context, p *plann
 	return nil
 }
 
+func (d *dropCascadeState) canDropType(
+	ctx context.Context, p *planner, typ *typedesc.Mutable,
+) error {
+	var referencedButNotDropping []descpb.ID
+	for _, id := range typ.ReferencingDescriptorIDs {
+		if _, exists := d.toDeleteByID[id]; exists {
+			continue
+		}
+		referencedButNotDropping = append(referencedButNotDropping, id)
+	}
+	if len(referencedButNotDropping) == 0 {
+		return nil
+	}
+	dependentNames, err := p.getFullyQualifiedTableNamesFromIDs(ctx, referencedButNotDropping)
+	if err != nil {
+		return errors.Wrapf(err, "type %q has dependent objects", typ.Name)
+	}
+	fqName, err := getTypeNameFromTypeDescriptor(
+		oneAtATimeSchemaResolver{ctx, p},
+		typ,
+	)
+	if err != nil {
+		return errors.Wrapf(err, "type %q has dependent objects", typ.Name)
+	}
+	return unimplemented.NewWithIssueDetailf(51480, "DROP TYPE CASCADE is not yet supported",
+		"cannot drop type %q because other objects (%v) still depend on it",
+		fqName.FQString(),
+		dependentNames,
+	)
+}
+
 func (d *dropCascadeState) getDroppedTableDetails() []jobspb.DroppedTableDetails {
 	res := make([]jobspb.DroppedTableDetails, len(d.allTableObjectsToDelete))
 	for i := range d.allTableObjectsToDelete {

pkg/sql/drop_table.go

@@ -266,7 +266,7 @@ func (p *planner) removeInterleave(ctx context.Context, ref descpb.ForeignKeyRef
 // dropTableImpl does the work of dropping a table (and everything that depends
 // on it if `cascade` is enabled). It returns a list of view names that were
 // dropped due to `cascade` behavior. droppingParent indicates whether this
-// table's parent (either database or schema) is being dropped.
+// table's parent (either database or schema) is being dropped
 func (p *planner) dropTableImpl(
 	ctx context.Context, tableDesc *tabledesc.Mutable, droppingParent bool, jobDesc string,
 ) ([]string, error) {
@@ -346,13 +346,20 @@ func (p *planner) dropTableImpl(
 		return droppedViews, err
 	}
 
-	// Remove any references to types that this table has if a job is meant to be
-	// queued. If not, then the job that is handling the drop table will also
-	// clean up all of the types to be dropped.
-	if !droppingParent {
-		if err := p.removeBackRefsFromAllTypesInTable(ctx, tableDesc); err != nil {
-			return droppedViews, err
-		}
+	// Remove any references to types.
+	//
+	// Note: In some historical context this attempted to defer these removals to
+	// the asynchronous schema change in the case that the parent was being
+	// dropped. This optimization was, as I understand it, to avoid creating
+	// so many descriptor writes if the type was definitely being dropped. This
+	// would be the case if the database were being dropped as theoretically cross
+	// database types have never been permitted. Unfortunately, the droppingParent
+	// flag does not indicate whether it's the schema or parent being dropped.
+	//
+	// TODO(ajwerner): Consider adding a flag to indicate what is actually being
+	// dropped and to omit this step if it is the database rather than the schema.
+	if err := p.removeBackRefsFromAllTypesInTable(ctx, tableDesc); err != nil {
+		return droppedViews, err
 	}
 
 	err = p.initiateDropTable(ctx, tableDesc, !droppingParent, jobDesc, true /* drain name */)

pkg/sql/drop_type.go

@@ -97,17 +97,9 @@ func (p *planner) canDropTypeDesc(
 		return err
 	}
 	if len(desc.ReferencingDescriptorIDs) > 0 && behavior != tree.DropCascade {
-		var dependentNames []string
-		for _, id := range desc.ReferencingDescriptorIDs {
-			desc, err := p.Descriptors().GetMutableTableVersionByID(ctx, id, p.txn)
-			if err != nil {
-				return errors.Wrapf(err, "type has dependent objects")
-			}
-			fqName, err := p.getQualifiedTableName(ctx, desc)
-			if err != nil {
-				return errors.Wrapf(err, "type %q has dependent objects", desc.Name)
-			}
-			dependentNames = append(dependentNames, fqName.FQString())
+		dependentNames, err := p.getFullyQualifiedTableNamesFromIDs(ctx, desc.ReferencingDescriptorIDs)
+		if err != nil {
+			return errors.Wrapf(err, "type %q has dependent objects", desc.Name)
 		}
 		return pgerror.Newf(
 			pgcode.DependentObjectsStillExist,

pkg/sql/logictest/testdata/logic_test/drop_type

@@ -353,3 +353,43 @@ subtest regression_58461
 statement ok
 CREATE TYPE pet AS ENUM('cat');
 DROP TYPE IF EXISTS pet, alien;
+
+# Test that dropping a schema which contains a type which refers to things
+# outside of that schema works.
+
+subtest drop_schema_cascade
+
+statement ok
+CREATE SCHEMA schema_to_drop;
+CREATE TYPE schema_to_drop.typ AS ENUM ('a');
+CREATE TABLE t (k schema_to_drop.typ PRIMARY KEY);
+CREATE TABLE schema_to_drop.t (k schema_to_drop.typ PRIMARY KEY);
+
+statement error pgcode 0A000 unimplemented: cannot drop type "test.schema_to_drop._typ" because other objects \(\[test\.public\.t\]\) still depend on it
+DROP SCHEMA schema_to_drop CASCADE;
+
+statement ok
+DROP TABLE t;
+
+statement ok
+DROP SCHEMA schema_to_drop CASCADE;
+
+# Test that dropping a table via a DROP SCHEMA CASCADE properly removes
+# back-references to types in different schemas which are not being dropped.
+
+statement ok
+CREATE SCHEMA sc1;
+CREATE SCHEMA sc2;
+CREATE TYPE sc2.typ AS ENUM ('a');
+CREATE TABLE sc1.table (k sc2.typ);
+DROP SCHEMA sc1 CASCADE;
+
+
+# If the backreference to the type had not been properly removed
+# for sc1.table above, the below statement would fail.
+statement ok
+DROP TYPE sc2.typ;
+
+# This is just cleanup.
+statement ok
+DROP SCHEMA sc2 CASCADE;

pkg/sql/resolver.go

@@ -394,6 +394,31 @@ func getDescriptorsFromTargetListForPrivilegeChange(
 	return descs, nil
 }
 
+// getFullyQualifiedTableNamesFromIDs resolves a list of table IDs to their
+// fully qualified names.
+func (p *planner) getFullyQualifiedTableNamesFromIDs(
+	ctx context.Context, ids []descpb.ID,
+) (fullyQualifiedNames []*tree.TableName, _ error) {
+	for _, id := range ids {
+		desc, err := p.Descriptors().GetTableVersionByID(ctx, p.txn, id, tree.ObjectLookupFlags{
+			CommonLookupFlags: tree.CommonLookupFlags{
+				AvoidCached:    true,
+				IncludeDropped: true,
+				IncludeOffline: true,
+			},
+		})
+		if err != nil {
+			return nil, err
+		}
+		fqName, err := p.getQualifiedTableName(ctx, desc)
+		if err != nil {
+			return nil, err
+		}
+		fullyQualifiedNames = append(fullyQualifiedNames, fqName)
+	}
+	return fullyQualifiedNames, nil
+}
+
 // getQualifiedTableName returns the database-qualified name of the table
 // or view represented by the provided descriptor. It is a sort of
 // reverse of the Resolve() functions.