Merge cockroachdb#97769

97769: sql/schemachanger: version gate dropping constraints / functions r=fqazi a=fqazi Previously, it was possible in a mixed version state to cleanup constraints (foreign keys, unique without indexes, function bodies) when dropping columns. This would cause down level nodes to fail at planning seeing unsupported status transitions. To address this, this patch adds a version gate to prevent dropping any of these objects when cleaning up a column. Fixes: cockroachdb#97576 Release note: None Co-authored-by: Faizan Qazi <[email protected]>
nicktrav · Feb 28, 2023 · fb6eb66 · fb6eb66
2 parents 3ed2c8f + 626483a
commit fb6eb66
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 1 deletion.
diff --git a/pkg/sql/pgwire/testdata/pgtest/notice b/pkg/sql/pgwire/testdata/pgtest/notice
@@ -55,7 +55,7 @@ Query {"String": "DROP INDEX t_x_idx"}
 until crdb_only
 CommandComplete
 ----
-{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"the data for dropped indexes is reclaimed asynchronously","Detail":"","Hint":"The reclamation delay can be customized in the zone configuration for the table.","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"drop_index.go","Line":66,"Routine":"DropIndex","UnknownFields":null}
+{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"the data for dropped indexes is reclaimed asynchronously","Detail":"","Hint":"The reclamation delay can be customized in the zone configuration for the table.","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"drop_index.go","Line":68,"Routine":"DropIndex","UnknownFields":null}
 {"Type":"CommandComplete","CommandTag":"DROP INDEX"}
 
 until noncrdb_only

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go
@@ -13,6 +13,7 @@ package scbuildstmt
 import (
 	"sort"
 
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
@@ -255,6 +256,12 @@ func dropColumn(
 			}
 			dropCascadeDescriptor(b, e.FunctionID)
 		case *scpb.UniqueWithoutIndexConstraint:
+			// Until the appropriate version gate is hit, we still do not allow
+			// dropping unique without index constraints.
+			if !b.ClusterSettings().Version.IsActive(b, clusterversion.V23_1) {
+				panic(scerrors.NotImplementedErrorf(nil, "dropping without"+
+					"index constraints is not allowed."))
+			}
 			constraintElems := b.QueryByID(e.TableID).Filter(hasConstraintIDAttrFilter(e.ConstraintID))
 			_, _, constraintName := scpb.FindConstraintWithoutIndexName(constraintElems.Filter(publicTargetFilter))
 			alterTableDropConstraint(b, tn, tbl, &tree.AlterTableDropConstraint{
@@ -263,6 +270,12 @@ func dropColumn(
 				DropBehavior: behavior,
 			})
 		case *scpb.UniqueWithoutIndexConstraintUnvalidated:
+			// Until the appropriate version gate is hit, we still do not allow
+			// dropping unique without index constraints.
+			if !b.ClusterSettings().Version.IsActive(b, clusterversion.V23_1) {
+				panic(scerrors.NotImplementedErrorf(nil, "dropping without"+
+					"index constraints is not allowed."))
+			}
 			constraintElems := b.QueryByID(e.TableID).Filter(hasConstraintIDAttrFilter(e.ConstraintID))
 			_, _, constraintName := scpb.FindConstraintWithoutIndexName(constraintElems.Filter(publicTargetFilter))
 			alterTableDropConstraint(b, tn, tbl, &tree.AlterTableDropConstraint{

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go
@@ -13,11 +13,13 @@ package scbuildstmt
 import (
 	"fmt"
 
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
+	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -235,6 +237,12 @@ func maybeDropDependentFKConstraints(
 	// shouldDropFK returns true if it is a dependent FK and no uniqueness-providing
 	// replacement can be found.
 	shouldDropFK := func(fkReferencedColIDs []catid.ColumnID) bool {
+		// Until the appropriate version gate is hit, we still do not allow
+		// dropping foreign keys in any the context of secondary indexes.
+		if !b.ClusterSettings().Version.IsActive(b, clusterversion.V23_1) {
+			panic(scerrors.NotImplementedErrorf(nil, "dropping FK constraints"+
+				" as a result of `DROP INDEX CASCADE` is not supported yet."))
+		}
 		return canToBeDroppedConstraintServeFK(fkReferencedColIDs) &&
 			!hasColsUniquenessConstraintOtherThan(b, tableID, fkReferencedColIDs, toBeDroppedConstraintID)
 	}