From 06272b5e4a6d165927a15f6ecdd3636f9f624bda Mon Sep 17 00:00:00 2001 From: Faizan Qazi Date: Tue, 14 May 2024 15:44:14 +0000 Subject: [PATCH] sql: alter primary key hangs if index references exist 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: #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. --- pkg/sql/alter_primary_key.go | 24 ++++++++++-- .../testdata/logic_test/alter_primary_key | 34 +++++++++++++++++ .../logictest/testdata/logic_test/alter_table | 2 +- .../alter_table_alter_primary_key.go | 38 ++++++++++++++++++- 4 files changed, 91 insertions(+), 7 deletions(-) diff --git a/pkg/sql/alter_primary_key.go b/pkg/sql/alter_primary_key.go index bdbb33ace04d..d9174f8e7423 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 will + // 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 fd2821ae7689..7881fff097d5 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_primary_key +++ b/pkg/sql/logictest/testdata/logic_test/alter_primary_key @@ -1999,3 +1999,37 @@ statement error column col0_20 is of type jsonb and thus is not indexable ALTER TABLE tab_122871 ALTER PRIMARY KEY USING COLUMNS (col0_20); 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 f2af5b988c05..b641bb7566a2 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_table +++ b/pkg/sql/logictest/testdata/logic_test/alter_table @@ -3520,7 +3520,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 e24fb1411afe..4009f8f186ce 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) @@ -665,7 +665,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. @@ -684,6 +687,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 will + // 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.