From d6a3f9d0d97f25a1aff0da20a4a5194ffa9b62be 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 | 16 ++++++++ .../testdata/logic_test/alter_primary_key | 34 +++++++++++++++++ .../logictest/testdata/logic_test/alter_table | 7 +++- .../alter_table_alter_primary_key.go | 38 ++++++++++++++++++- 4 files changed, 92 insertions(+), 3 deletions(-) diff --git a/pkg/sql/alter_primary_key.go b/pkg/sql/alter_primary_key.go index bdbb33ace04d..ca43f6719f5f 100644 --- a/pkg/sql/alter_primary_key.go +++ b/pkg/sql/alter_primary_key.go @@ -453,6 +453,22 @@ 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 pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, + "table %q has an index (%s) that is still referenced by %q", + tableDesc.GetName(), + idx.GetName(), + refDesc.GetName()) + } + } } // 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..6ac84c18c4d0 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: 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: 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 d42497e6ab47..17fc121d592c 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_table +++ b/pkg/sql/logictest/testdata/logic_test/alter_table @@ -3511,7 +3511,12 @@ 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 +skipif config local-legacy-schema-changer +statement error pgcode 55000 pq: 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); + +onlyif config local-legacy-schema-changer +statement error pgcode 2BP01 pq: cannot drop index \"t_108974_f_pkey\" because function \"f_108974\" depends on it 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 76f38e05c133..991e9ed2a2fc 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 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(pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, + "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(pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, + "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.