sql: alter primary key hangs if index references exist #124132
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @fqazi)
pkg/sql/logictest/testdata/logic_test/alter_table line 3514 at r1 (raw file):
DROP INDEX t_108974_f@t_108974_f_i_j_idx; skipif config local-legacy-schema-changer
nit: shouid we have to distinguish the error messages between the legacy vs declarative schema changer (i'm assuming that the order of error prop. is the diff between the two ? the dsc looks like it does it in disallowDroppingPrimaryIndexReferencedInUDFOrView)
pkg/sql/alter_primary_key.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, |
There was a problem hiding this comment.
should we use the unimplemented.NewWithIssue helper and point at the issue you filed?
| 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 |
There was a problem hiding this comment.
does this mean the legacy schema changer isn't actually affected? (since it disallows the alter PK for a different reason already?)
| for _, function := range functions { | ||
| for _, tableRef := range function.UsesTables { | ||
| if tableRef.TableID == idx.TableID && tableRef.IndexID == idx.IndexID { | ||
| panic(pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, |
There was a problem hiding this comment.
should we use the unimplemented.NewWithIssue helper and point at the issue you filed?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/alter_table line 3518 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
does this mean the legacy schema changer isn't actually affected? (since it disallows the alter PK for a different reason already?)
Only in the case of functions and only if there are PK references, not secondary indexes, but views are still broken.
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.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/alter_primary_key.go line 465 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
should we use the
unimplemented.NewWithIssuehelper and point at the issue you filed?
Done.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 700 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
should we use the
unimplemented.NewWithIssuehelper and point at the issue you filed?
Done.
pkg/sql/logictest/testdata/logic_test/alter_table line 3514 at r1 (raw file):
Previously, annrpom (annie pompa) wrote…
nit: shouid we have to distinguish the error messages between the legacy vs declarative schema changer (i'm assuming that the order of error prop. is the diff between the two ? the dsc looks like it does it in
disallowDroppingPrimaryIndexReferencedInUDFOrView)
Good point, I moved the check to later in the legacy schema changer. The declarative one can't be modified because we do it once for the comma syntax.
|
bors r+ |
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 575e6f5 to blathers/backport-release-23.2-124132: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
@yuzefovich Thanks for the reminder, let me get one open now. |
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.