diff --git a/pkg/sql/alter_primary_key.go b/pkg/sql/alter_primary_key.go index bdbb33ace04d..bbf9ff05caac 100644 --- a/pkg/sql/alter_primary_key.go +++ b/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. diff --git a/pkg/sql/logictest/testdata/logic_test/alter_primary_key b/pkg/sql/logictest/testdata/logic_test/alter_primary_key index e2e6434c7f9c..6bed5f2dcd44 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_primary_key +++ b/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 diff --git a/pkg/sql/logictest/testdata/logic_test/alter_table b/pkg/sql/logictest/testdata/logic_test/alter_table index 38184fb0251e..9a5f5596eb2f 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_table +++ b/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 diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go index 99bd6c13daa0..ca83a978b78d 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go +++ b/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.