Merge cockroachdb#124132

124132: sql: alter primary key hangs if index references exist r=fqazi a=fqazi

Previously, ALTER PRIMARY KEY could hang if an index reference existed from either a view or function using the index selection syntax. This was becasue neither the declarative schema changer or legacy schema changer support updating these references. To address this, this patch will prevent ALTER PRIMARY KEY if such references exist to any indexes that will be recreated.

Fixes: cockroachdb#123017
Release note (bug fix): ALTER PRIMARY KEY could hang if the table had any indexes which were referred to by views or functions using the force index syntax.

Co-authored-by: Faizan Qazi <[email protected]>

pkg/sql/alter_primary_key.go

@@ -68,10 +68,6 @@ func (p *planner) AlterPrimaryKey(
 		return err
 	}
 
-	if err := p.disallowDroppingPrimaryIndexReferencedInUDFOrView(ctx, tableDesc); err != nil {
-		return err
-	}
-
 	if alterPrimaryKeyLocalitySwap != nil {
 		if err := p.checkNoRegionChangeUnderway(
 			ctx,
@@ -453,6 +449,26 @@ func (p *planner) AlterPrimaryKey(
 		if idx.GetID() != newPrimaryIndexDesc.ID && shouldRewrite {
 			indexesToRewrite = append(indexesToRewrite, idx)
 		}
+		// If this index is referenced by any other objects, then we wil
+		// block the primary key swap, since we don't have a mechanism to
+		// fix these references yet.
+		for _, tableRef := range tableDesc.GetDependedOnBy() {
+			if tableRef.IndexID == idx.GetID() {
+				refDesc, err := p.Descriptors().ByIDWithLeased(p.txn).Get().Desc(ctx, tableRef.ID)
+				if err != nil {
+					return err
+				}
+				return unimplemented.NewWithIssuef(124131,
+					"table %q has an index (%s) that is still referenced by %q",
+					tableDesc.GetName(),
+					idx.GetName(),
+					refDesc.GetName())
+			}
+		}
+	}
+
+	if err := p.disallowDroppingPrimaryIndexReferencedInUDFOrView(ctx, tableDesc); err != nil {
+		return err
 	}
 
 	// TODO (rohany): this loop will be unused until #45510 is resolved.

pkg/sql/logictest/testdata/logic_test/alter_primary_key

@@ -1990,3 +1990,37 @@ statement error column col0_4 is of type tsvector and thus is not indexable
 ALTER TABLE tab_122871 ALTER PRIMARY KEY USING COLUMNS (col0_4);
 
 subtest end
+
+# In #123017 we noticed that ALTER PRIMARY KEY does not work correctly
+# if there are either view or function references to an index due to validation
+# errors in both the legacy and declarative schema changer. Long term we can
+# support this in the declarative schema changer.
+subtest alter_primary_key_block_func_view_ref
+
+statement ok
+CREATE TABLE t_view_func_ref_123017 (
+    a STRING NOT NULL,
+    b INT2,
+    UNIQUE (b ASC)
+);
+CREATE FUNCTION f_123017() RETURNS RECORD LANGUAGE SQL AS $$
+  SELECT NULL FROM t_view_func_ref_123017@t_view_func_ref_123017_b_key
+$$;
+CREATE VIEW t_view_123017 AS (SELECT b FROM t_view_func_ref_123017@t_view_func_ref_123017_b_key);
+
+statement error pq: unimplemented: table \"t_view_func_ref_123017\" has an index \(t_view_func_ref_123017_b_key\) that is still referenced by \"f_123017\"
+ALTER TABLE t_view_func_ref_123017 ALTER PRIMARY KEY USING COLUMNS (a);
+
+statement ok
+DROP FUNCTION f_123017;
+
+statement error pq: unimplemented: table \"t_view_func_ref_123017\" has an index \(t_view_func_ref_123017_b_key\) that is still referenced by \"t_view_123017\"
+ALTER TABLE t_view_func_ref_123017 ALTER PRIMARY KEY USING COLUMNS (a);
+
+statement ok
+DROP VIEW t_view_123017;
+
+statement ok
+ALTER TABLE t_view_func_ref_123017 ALTER PRIMARY KEY USING COLUMNS (a);
+
+subtest end

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -3511,7 +3511,7 @@ SET sql_safe_updates = false;
 statement error pgcode 2BP01 pq: cannot drop index "t_108974_f_i_j_idx" because function "f_108974" depends on it
 DROP INDEX t_108974_f@t_108974_f_i_j_idx;
 
-statement error pgcode 2BP01 pq: cannot drop index "t_108974_f_pkey" because function "f_108974" depends on it
+statement error pgcode 0A000 pq: unimplemented: table \"t_108974_f\" has an index \(t_108974_f_i_j_idx\) that is still referenced by \"f_108974\"
 ALTER TABLE t_108974_f ALTER PRIMARY KEY USING COLUMNS (j);
 
 statement error pgcode 2BP01 pq: cannot drop index "t_108974_v_pkey" because view "v_108974" depends on it

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

@@ -106,7 +106,7 @@ func alterPrimaryKey(b BuildCtx, tn *tree.TableName, tbl *scpb.Table, t alterPri
 	b.LogEventForExistingTarget(inflatedChain.finalSpec.primary)
 
 	// Recreate all secondary indexes.
-	recreateAllSecondaryIndexes(b, tbl, inflatedChain.finalSpec.primary, inflatedChain.inter2Spec.primary)
+	recreateAllSecondaryIndexes(b, t, tbl, inflatedChain.finalSpec.primary, inflatedChain.inter2Spec.primary)
 
 	// Drop the rowid column, if applicable.
 	rowidToDrop := getPrimaryIndexDefaultRowIDColumn(b, tbl.TableID, inflatedChain.oldSpec.primary.IndexID)
@@ -666,7 +666,10 @@ func checkIfConstraintNameAlreadyExists(b BuildCtx, tbl *scpb.Table, t alterPrim
 // columns remain the same in the face of a primary key change, the key suffix
 // columns or the stored columns may not.
 func recreateAllSecondaryIndexes(
-	b BuildCtx, tbl *scpb.Table, newPrimaryIndex, sourcePrimaryIndex *scpb.PrimaryIndex,
+	b BuildCtx,
+	t alterPrimaryKeySpec,
+	tbl *scpb.Table,
+	newPrimaryIndex, sourcePrimaryIndex *scpb.PrimaryIndex,
 ) {
 	publicTableElts := b.QueryByID(tbl.TableID).Filter(publicTargetFilter)
 	// Generate all possible key suffix columns.
@@ -685,6 +688,37 @@ func recreateAllSecondaryIndexes(
 	// Recreate each secondary index.
 	scpb.ForEachSecondaryIndex(publicTableElts, func(_ scpb.Status, _ scpb.TargetStatus, idx *scpb.SecondaryIndex) {
 		out := makeIndexSpec(b, idx.TableID, idx.IndexID)
+		// If this index is referenced by any other objects, then we wil
+		// block the primary key swap, since we don't have a mechanism to
+		// fix these references yet.
+		// TODO(fqazi): As a part of #124131 we should add logic to fix
+		// these references.
+		backrefs := b.BackReferences(idx.TableID)
+		functions := backrefs.FilterFunctionBody().Elements()
+		for _, function := range functions {
+			for _, tableRef := range function.UsesTables {
+				if tableRef.TableID == idx.TableID && tableRef.IndexID == idx.IndexID {
+					panic(unimplemented.NewWithIssuef(124131,
+						"table %q has an index (%s) that is still referenced by %q",
+						publicTableElts.FilterNamespace().MustGetOneElement().Name,
+						out.name.Name,
+						b.QueryByID(function.FunctionID).FilterFunctionName().MustGetOneElement().Name))
+				}
+			}
+		}
+		views := backrefs.FilterView().Elements()
+		for _, view := range views {
+			for _, f := range view.ForwardReferences {
+				if f.ToID == idx.TableID && f.IndexID == idx.IndexID {
+					panic(unimplemented.NewWithIssuef(124131,
+						"table %q has an index (%s) that is still referenced by %q",
+						publicTableElts.FilterNamespace().MustGetOneElement().Name,
+						out.name.Name,
+						b.QueryByID(view.ViewID).FilterNamespace().MustGetOneElement().Name))
+				}
+			}
+		}
+
 		var idxColIDs catalog.TableColSet
 		inColumns := make([]indexColumnSpec, 0, len(out.columns))
 		// Determine which columns end up in the new secondary index.