schemachanger: check dependents when dropping hash-sharded index

Release note (bug fix): Fixed a bug where DROP INDEX on a hash-sharded
index did not properly detect dependencies from functions and procedures
on the shard column. This bug would cause the DROP INDEX statement to
fail with an internal validation error. Now, the statement returns a
correct error message, and using DROP INDEX ... CASCADE works as
expected by dropping the dependent function/procedure.

Author: rafiss

pkg/sql/drop_index.go

@@ -198,7 +198,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
 					}
@@ -246,7 +246,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()
@@ -258,7 +258,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
@@ -267,7 +267,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() {
@@ -284,8 +284,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

@@ -527,4 +527,61 @@ CREATE INDEX roaches_value_idx ON roaches(value);
 statement error pq: rejected \(sql_safe_updates = true\): DROP INDEX
 DROP INDEX IF EXISTS roaches@roaches_value_idx;
 
+statement ok
+SET sql_safe_updates = false;
+
+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

@@ -170,6 +170,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),
@@ -179,7 +184,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

@@ -104,7 +104,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
 }
 
@@ -115,6 +115,7 @@ func dropSecondaryIndex(
 	indexName *tree.TableIndexName,
 	dropBehavior tree.DropBehavior,
 	sie *scpb.SecondaryIndex,
+	stmt tree.Statement,
 ) {
 	{
 		next := b.WithNewSourceElementID()
@@ -141,7 +142,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.
@@ -208,7 +211,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))
@@ -292,6 +295,7 @@ func maybeDropAdditionallyForShardedIndex(
 	b BuildCtx,
 	toBeDroppedIndex *scpb.SecondaryIndex,
 	toBeDroppedIndexName string,
+	stmt tree.Statement,
 	dropBehavior tree.DropBehavior,
 ) {
 	if toBeDroppedIndex.Sharding == nil || !toBeDroppedIndex.Sharding.IsSharded {
@@ -349,6 +353,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