Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: alter pk using subset of old pk key columns should always rewrite secondary indexes #84040

Closed
chengxiong-ruan opened this issue Jul 7, 2022 · 0 comments · Fixed by #84303
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@chengxiong-ruan
Copy link
Contributor

chengxiong-ruan commented Jul 7, 2022

Describe the problem
Currently when doing ALTER PRIMARY kEY, we have smart logic to determine whether to rewrite secondary index or not. Simply put, the logic is based on the rule of how we encode secondary index key/value pair and if the secondary index key/value pair has enough information to construct a primary key to fetch more data. There is one corner case, when we chose not to rewrite, secondary index still has references to columns from the old primary key in suffixKeyColumns that's not part of new primary key. This is problematic because user could drop the old primary key column since they think they're not needed any more, and that would cause the secondary index to be dropped silently (see this issue)

See the repro below for details about the specific issue. This happens in all versions.

To Reproduce

create table t (
  a int not null,
  b int not null,
  c int not null,
  primary key (a, b),
  unique index uniq_idx (c) -- this index is unique, and all its key columns are not nullable.
                            -- based on crdb's encoding theory, the index value contains columns "a" and "b"
                            -- in the index's suffix columns. the index key only contains column "c"
);

-- now alter primary key to a subset of old primary key columns

alter table t alter primary key using columns (a); -- new primary key col "a" is a subset of "a, b".
                                                   -- at this moment `uniq_idx`'s suffix columns still contain "a" and "b"
                                                   -- because it was not rewritten. This is because our logic think that
                                                   -- `uniq_idx` has enough information to infer a primary key. And because
                                                   -- it's an unique key with only not nullable columns, its index key does not
                                                   -- contain any primary key column, so we chose not to  rewrite this index.
-- now let's drop column `b`, because we think it's not needed...
-- note that `b` is not in primary key and `uniq_idx` when we does a SHOW CREATE TABLE.
alter table t drop column b;
-- boom....`uniq_idx` is dropped because we think that column "b" is referenced by it :(

Desired Behavior
We need to rewrite the secondary index in this case even we have enough information to infer a primary key.

@chengxiong-ruan chengxiong-ruan added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-schema-deprecated Use T-sql-foundations instead labels Jul 7, 2022
@postamar postamar assigned fqazi and Xiang-Gu and unassigned fqazi Jul 12, 2022
@craig craig bot closed this as completed in a3d74c3 Jul 15, 2022
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants