Merge pull request #145392 from rafiss/backport24.3-145107

release-24.3: schemachanger: check dependents when dropping hash-sharded index

Author: rafiss

pkg/sql/drop_index.go

@@ -174,7 +174,7 @@ func (n *dropIndexNode) startExec(params runParams) error {
 				// Only drop this shard column if it's not a physical column (as is the case for all hash-sharded index in 22.1
 				// and after), or, CASCADE is set.
 				if shardColDesc.IsVirtual() || n.n.DropBehavior == tree.DropCascade {
-					ok, err := n.maybeQueueDropShardColumn(tableDesc, shardColDesc)
+					ok, err := n.maybeQueueDropShardColumn(params, index.tn, tableDesc, shardColDesc)
 					if err != nil {
 						return err
 					}
@@ -222,7 +222,7 @@ func (n *dropIndexNode) queueDropColumn(tableDesc *tabledesc.Mutable, col catalo
 //
 // Assumes that the given index is sharded.
 func (n *dropIndexNode) maybeQueueDropShardColumn(
-	tableDesc *tabledesc.Mutable, shardColDesc catalog.Column,
+	params runParams, tn *tree.TableName, tableDesc *tabledesc.Mutable, shardColDesc catalog.Column,
 ) (bool, error) {
 	if catalog.FindNonDropIndex(tableDesc, func(otherIdx catalog.Index) bool {
 		colIDs := otherIdx.CollectKeyColumnIDs()
@@ -234,7 +234,7 @@ func (n *dropIndexNode) maybeQueueDropShardColumn(
 	}) != nil {
 		return false, nil
 	}
-	if err := n.dropShardColumnAndConstraint(tableDesc, shardColDesc); err != nil {
+	if err := n.dropShardColumnAndConstraint(params, tn, tableDesc, shardColDesc); err != nil {
 		return false, err
 	}
 	return true, nil
@@ -243,7 +243,7 @@ func (n *dropIndexNode) maybeQueueDropShardColumn(
 // dropShardColumnAndConstraint drops the given shard column and its associated check
 // constraint.
 func (n *dropIndexNode) dropShardColumnAndConstraint(
-	tableDesc *tabledesc.Mutable, shardCol catalog.Column,
+	params runParams, tn *tree.TableName, tableDesc *tabledesc.Mutable, shardCol catalog.Column,
 ) error {
 	validChecks := tableDesc.Checks[:0]
 	for _, check := range tableDesc.CheckConstraints() {
@@ -260,8 +260,14 @@ func (n *dropIndexNode) dropShardColumnAndConstraint(
 	if len(validChecks) != len(tableDesc.Checks) {
 		tableDesc.Checks = validChecks
 	}
-
-	n.queueDropColumn(tableDesc, shardCol)
+	if _, err := dropColumnImpl(
+		params, tn, tableDesc, tableDesc.GetRowLevelTTL(),
+		&tree.AlterTableDropColumn{
+			Column:       shardCol.ColName(),
+			DropBehavior: n.n.DropBehavior},
+	); err != nil {
+		return err
+	}
 	return nil
 }
 

pkg/sql/logictest/testdata/logic_test/drop_index

@@ -505,3 +505,59 @@ SELECT constraint_name from [SHOW CONSTRAINTS FROM fk_ref_dst];
 fk_ref_dst_pkey
 j_fk
 m_fk
+
+subtest drop_hash_sharded_index_depended_on_by_procedure
+
+statement ok
+CREATE TABLE tab_145100 (
+  id UUID PRIMARY KEY,
+  i INT NOT NULL,
+  j int not null,
+  INDEX (i ASC) USING HASH,
+  FAMILY (id, i, j)
+)
+
+statement ok
+CREATE PROCEDURE proc_insert_145100(in_id UUID, in_i INT) LANGUAGE SQL AS $$
+  INSERT INTO tab_145100 (id, i) VALUES (in_id, in_i);
+$$;
+
+# Note: Due to https://github.com/cockroachdb/cockroach/issues/145098, the
+# procedure has a dependency on the shard column. When that issue is resolved,
+# this DROP statement should succeed.
+statement error cannot drop column "crdb_internal_i_shard_16" because function "proc_insert_145100" depends on it
+DROP INDEX tab_145100@tab_145100_i_idx
+
+statement ok
+DROP PROCEDURE proc_insert_145100
+
+statement ok
+CREATE PROCEDURE proc_select_145100() LANGUAGE SQL AS $$
+  SELECT *, crdb_internal_i_shard_16 FROM tab_145100;
+$$;
+
+statement error cannot drop column "crdb_internal_i_shard_16" because function "proc_select_145100" depends on it
+DROP INDEX tab_145100@tab_145100_i_idx
+
+statement ok
+DROP INDEX tab_145100@tab_145100_i_idx CASCADE
+
+query T
+SELECT create_statement FROM [SHOW CREATE TABLE tab_145100]
+----
+CREATE TABLE public.tab_145100 (
+  id UUID NOT NULL,
+  i INT8 NOT NULL,
+  j INT8 NOT NULL,
+  CONSTRAINT tab_145100_pkey PRIMARY KEY (id ASC),
+  FAMILY fam_0_id_i_j (id, i, j)
+)
+
+# DROP INDEX ... CASCADE should have caused the procedure to be dropped.
+statement error procedure proc_select_145100 does not exist
+CALL proc_select_145100()
+
+statement ok
+DROP TABLE tab_145100
+
+subtest end

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go

@@ -172,6 +172,11 @@ func dropColumn(
 			if indexTargetStatus == scpb.ToAbsent {
 				return
 			}
+			// If we entered this function because of a DROP INDEX statement (e.g. for
+			// a hash-sharded index), avoid recursive calls to drop the index again.
+			if _, isDropIndex := stmt.(*tree.DropIndex); isDropIndex {
+				return
+			}
 			name := tree.TableIndexName{
 				Table: *tn,
 				Index: tree.UnrestrictedName(indexName.Name),
@@ -181,7 +186,7 @@ func dropColumn(
 				indexName.Name,
 				cn.Name,
 			))
-			dropSecondaryIndex(b, &name, behavior, e)
+			dropSecondaryIndex(b, &name, behavior, e, stmt)
 		case *scpb.View:
 			if behavior != tree.DropCascade {
 				_, _, ns := scpb.FindNamespace(b.QueryByID(col.TableID))

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go

@@ -105,7 +105,7 @@ func maybeDropIndex(
 			"use CASCADE if you really want to drop it.",
 		))
 	}
-	dropSecondaryIndex(b, indexName, n.DropBehavior, sie)
+	dropSecondaryIndex(b, indexName, n.DropBehavior, sie, n)
 	return sie
 }
 
@@ -116,6 +116,7 @@ func dropSecondaryIndex(
 	indexName *tree.TableIndexName,
 	dropBehavior tree.DropBehavior,
 	sie *scpb.SecondaryIndex,
+	stmt tree.Statement,
 ) {
 	{
 		next := b.WithNewSourceElementID()
@@ -142,7 +143,9 @@ func dropSecondaryIndex(
 
 		// If shard index, also drop the shard column and all check constraints that
 		// uses this shard column if no other index uses the shard column.
-		maybeDropAdditionallyForShardedIndex(next, sie, indexName.Index.String(), dropBehavior)
+		maybeDropAdditionallyForShardedIndex(
+			next, sie, indexName.Index.String(), stmt, dropBehavior,
+		)
 
 		// If expression index, also drop the expression column if no other index is
 		// using the expression column.
@@ -209,7 +212,7 @@ func maybeDropDependentFunctions(
 			if forwardRef.IndexID != toBeDroppedIndex.IndexID {
 				continue
 			}
-			// This view depends on the to-be-dropped index;
+			// This function depends on the to-be-dropped index.
 			if dropBehavior != tree.DropCascade {
 				// Get view name for the error message
 				_, _, fnName := scpb.FindFunctionName(b.QueryByID(e.FunctionID))
@@ -293,6 +296,7 @@ func maybeDropAdditionallyForShardedIndex(
 	b BuildCtx,
 	toBeDroppedIndex *scpb.SecondaryIndex,
 	toBeDroppedIndexName string,
+	stmt tree.Statement,
 	dropBehavior tree.DropBehavior,
 ) {
 	if toBeDroppedIndex.Sharding == nil || !toBeDroppedIndex.Sharding.IsSharded {
@@ -350,6 +354,11 @@ func maybeDropAdditionallyForShardedIndex(
 			b.Drop(e)
 		}
 	})
+	tbl := b.QueryByID(toBeDroppedIndex.TableID).FilterTable().MustGetOneElement()
+	ns := b.QueryByID(toBeDroppedIndex.TableID).FilterNamespace().MustGetOneElement()
+	tn := tree.MakeTableNameFromPrefix(b.NamePrefix(tbl), tree.Name(ns.Name))
+	shardCol := shardColElms.FilterColumn().MustGetOneElement()
+	dropColumn(b, &tn, tbl, stmt, stmt, shardCol, shardColElms, dropBehavior)
 }
 
 // dropAdditionallyForExpressionIndex attempts to drop the additional