sql/schemachangr: dropping column families ran into errors

Previously, dropping column families was not implemented,
when we eliminated oprules, we replaced the ops with
NotImplementedForDrop, which wasn't sufficient for
dropped columns. The column families are cleaned up
when the column type itself is dropped, so we don't
really need to do much here. To address this, this
patch will add code to only assert that either the
table is being dropped or the column family has been
removed earlier.

Fixes: cockroachdb#99796
Release note: None

pkg/sql/schemachanger/scexec/scmutationexec/column.go

@@ -235,6 +235,22 @@ func (i *immediateVisitor) AddColumnFamily(ctx context.Context, op scop.AddColum
 	return nil
 }
 
+func (i *immediateVisitor) AssertColumnFamilyIsRemoved(
+	ctx context.Context, op scop.AssertColumnFamilyIsRemoved,
+) error {
+	tbl, err := i.checkOutTable(ctx, op.TableID)
+	if err != nil || tbl.Dropped() {
+		return err
+	}
+	for idx := range tbl.Families {
+		if tbl.Families[idx].ID == op.FamilyID {
+			return errors.AssertionFailedf("column family was leaked during schema change %v",
+				tbl.Families[idx])
+		}
+	}
+	return nil
+}
+
 func (i *immediateVisitor) SetColumnName(ctx context.Context, op scop.SetColumnName) error {
 	tbl, err := i.checkOutTable(ctx, op.TableID)
 	if err != nil || tbl.Dropped() {

pkg/sql/schemachanger/scexec/scmutationexec/scmutationexec.go

@@ -57,9 +57,9 @@ func (i *immediateVisitor) NotImplementedForPublicObjects(
 	ctx context.Context, op scop.NotImplementedForPublicObjects,
 ) error {
 	desc := i.MaybeGetCheckedOutDescriptor(op.DescID)
-	if desc.Dropped() {
+	if desc == nil || desc.Dropped() {
 		return nil
 	}
-	return errors.AssertionFailedf("not implemented operation was hit " +
-		"unexpectedly, no dropped descriptor was found.")
+	return errors.AssertionFailedf("not implemented operation was hit "+
+		"unexpectedly, no dropped descriptor was found. %v", op.ElementType)
 }

pkg/sql/schemachanger/scop/immediate_mutation.go

@@ -429,6 +429,13 @@ type AddColumnFamily struct {
 	Name     string
 }
 
+// AssertColumnFamilyIsRemoved asserts that a column family is removed.
+type AssertColumnFamilyIsRemoved struct {
+	immediateMutationOp
+	TableID  descpb.ID
+	FamilyID descpb.FamilyID
+}
+
 // AddColumnDefaultExpression adds a DEFAULT expression to a column.
 type AddColumnDefaultExpression struct {
 	immediateMutationOp

pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_family.go

@@ -11,6 +11,7 @@
 package opgen
 
 import (
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scop"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
 )
@@ -34,8 +35,18 @@ func init() {
 			scpb.Status_PUBLIC,
 			to(scpb.Status_ABSENT,
 				revertible(false),
-				emit(func(this *scpb.ColumnFamily) *scop.NotImplementedForPublicObjects {
-					return notImplementedForPublicObjects(this)
+				emit(func(this *scpb.ColumnFamily, md *opGenContext) *scop.AssertColumnFamilyIsRemoved {
+					// Use stricter criteria for clean up, since the newer
+					// rules enforce proper ordering here.
+					if md.ActiveVersion.IsActive(clusterversion.V23_1) {
+						return &scop.AssertColumnFamilyIsRemoved{
+							TableID:  this.TableID,
+							FamilyID: this.FamilyID,
+						}
+					}
+					// We would have used NotImplemented before, but that will be an
+					// assertion now, so just return no-ops.
+					return nil
 				}),
 			),
 		),

pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_column.go

@@ -48,6 +48,20 @@ func init() {
 			}
 		},
 	)
+
+	registerDepRule(
+		"column type removed before column family",
+		scgraph.Precedence,
+		"column-type", "column-family",
+		func(from, to NodeVars) rel.Clauses {
+			return rel.Clauses{
+				from.Type((*scpb.ColumnType)(nil)),
+				to.Type((*scpb.ColumnFamily)(nil)),
+				JoinOnColumnFamilyID(from, to, "table-id", "family-id"),
+				StatusesToAbsent(from, scpb.Status_ABSENT, to, scpb.Status_ABSENT),
+			}
+		},
+	)
 }
 
 // Special cases of the above.

pkg/sql/schemachanger/scplan/internal/rules/helpers.go

@@ -111,6 +111,11 @@ func JoinOnColumnID(a, b NodeVars, relationIDVar, columnIDVar rel.Var) rel.Claus
 	return joinOnColumnIDUntyped(a.El, b.El, relationIDVar, columnIDVar)
 }
 
+// JoinOnColumnFamilyID joins elements on column ID.
+func JoinOnColumnFamilyID(a, b NodeVars, relationIDVar, columnFamilyIDVar rel.Var) rel.Clause {
+	return joinOnColumnFamilyIDUntyped(a.El, b.El, relationIDVar, columnFamilyIDVar)
+}
+
 // JoinOnIndexID joins elements on index ID.
 func JoinOnIndexID(a, b NodeVars, relationIDVar, indexIDVar rel.Var) rel.Clause {
 	return joinOnIndexIDUntyped(a.El, b.El, relationIDVar, indexIDVar)
@@ -206,6 +211,16 @@ var (
 			}
 		},
 	)
+	joinOnColumnFamilyIDUntyped = screl.Schema.Def4(
+		"joinOnColumnFamilyID", "a", "b", "desc-id", "family-id", func(
+			a, b, descID, familyID rel.Var,
+		) rel.Clauses {
+			return rel.Clauses{
+				JoinOnDescIDUntyped(a, b, descID),
+				familyID.Entities(screl.ColumnFamilyID, a, b),
+			}
+		},
+	)
 	joinOnConstraintIDUntyped = screl.Schema.Def4(
 		"joinOnConstraintID", "a", "b", "desc-id", "constraint-id", func(
 			a, b, descID, constraintID rel.Var,