Skip to content

Commit

Permalink
sql/alter_primary_key: rewrite secondary index if new PK subsets old PK
Browse files Browse the repository at this point in the history
Previously, during a `ALTER PRIMARY KEY`, if the new PK columns is a
subset of the old PK columns, we won't rewrite existing unique,
secondary index.

This was inadequate because the user might think this column is not used
anywhere and will want to drop it, which will unexpectedly drop the
dependent unique index.

Release note (bug fix): This PR fixed a bug where, in a `ALTER PRIMARY
KEY`, if the new PK columns is a subset of the old PK columns, we will
not rewrite existing secondary index, and hence those secondary indexes
continue to have some of the old PK columns in their `suffixColumns`.
But the user might, reasonably, think those columns are not used anymore
and proceed to drop them. The bug then caused those dependent secondary
indexes to be dropped, unexpectedly for the user.
  • Loading branch information
Xiang-Gu committed Jul 14, 2022
1 parent 4566e92 commit 190246b
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 5 deletions.
5 changes: 5 additions & 0 deletions pkg/sql/alter_primary_key.go
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,11 @@ func (p *planner) AlterPrimaryKey(
return true, nil
}
}
if !idx.Primary() && catalog.MakeTableColSet(newPrimaryIndexDesc.KeyColumnIDs...).SubsetOf(
catalog.MakeTableColSet(tableDesc.PrimaryIndex.KeyColumnIDs...)) {
// Always rewrite a secondary index if the new PK columns is a (strict) subset of the old PK columns.
return true, nil
}
if idx.IsUnique() {
for i := 0; i < idx.NumKeyColumns(); i++ {
colID := idx.GetKeyColumnID(i)
Expand Down
79 changes: 74 additions & 5 deletions pkg/sql/logictest/testdata/logic_test/alter_primary_key
Original file line number Diff line number Diff line change
Expand Up @@ -1207,9 +1207,9 @@ t1_pkey id DESC
t1_pkey id2 N/A
t1_pkey name N/A
t1_pkey rowid N/A
t1_id_key id ASC
t1_id_id2_key id ASC
t1_id_id2_key id2 ASC
t1_id_key id ASC


statement ok
Expand All @@ -1222,9 +1222,9 @@ t1_pkey id DESC
t1_pkey id2 N/A
t1_pkey name N/A
t1_pkey rowid N/A
t1_id_key id ASC
t1_id_id2_key id ASC
t1_id_id2_key id2 ASC
t1_id_key id ASC

statement ok
alter table t1 alter primary key using columns(id desc);
Expand All @@ -1236,9 +1236,9 @@ t1_pkey id DESC
t1_pkey id2 N/A
t1_pkey name N/A
t1_pkey rowid N/A
t1_id_key id ASC
t1_id_id2_key id ASC
t1_id_id2_key id2 ASC
t1_id_key id ASC

statement ok
alter table t1 alter primary key using columns(id) USING HASH WITH (bucket_count=10)
Expand All @@ -1253,11 +1253,11 @@ t1_pkey name N/A
t1_pkey rowid N/A
t1_id_key1 id DESC
t1_id_key1 crdb_internal_id_shard_10 ASC
t1_id_key id ASC
t1_id_key crdb_internal_id_shard_10 ASC
t1_id_id2_key id ASC
t1_id_id2_key id2 ASC
t1_id_id2_key crdb_internal_id_shard_10 ASC
t1_id_key id ASC
t1_id_key crdb_internal_id_shard_10 ASC

statement ok
CREATE TABLE table_with_virtual_cols (
Expand Down Expand Up @@ -1637,3 +1637,72 @@ t_a_b_key a ASC false
t_a_b_key b ASC false
t_a_key a ASC false
t_a_key b ASC true

# The following regression test makes sure that when the new PK columns
# is a (strict) subset of the old PK columns, all existing secondary indexes
# were rewritten, and hence dropping a column from the old PK columns does not
# unexpectedly drop an existing secondary index.
subtest regression_#84040

statement ok
DROP TABLE IF EXISTS t

statement ok
CREATE TABLE t (
a INT NOT NULL,
b INT NOT NULL,
c INT NOT NULL,
PRIMARY KEY (a, b),
UNIQUE INDEX uniq_idx (c)
);

statement ok
ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (a)

statement ok
ALTER TABLE t DROP COLUMN b

query TTT
select index_name,column_name,direction from [show indexes from t];
----
t_pkey a ASC
t_pkey c N/A
uniq_idx c ASC
uniq_idx a ASC

# Alter primary key on a hash-sharded primary key to be non-hash-sharded.
# This had unexpectedly caused all unique indexes to be dropped silently on v21.2.
# See the support issue https://github.com/cockroachlabs/support/issues/1687
statement ok
DROP TABLE IF EXISTS t;

statement ok
CREATE TABLE t (
i INT PRIMARY KEY USING HASH WITH (bucket_count=7) DEFAULT unique_rowid(),
j INT NOT NULL UNIQUE
)

# Assert that the primary key is hash-sharded and the unique index is created.
query TTT
SELECT index_name,column_name,direction FROM [SHOW INDEXES FROM t]
----
t_pkey crdb_internal_i_shard_7 ASC
t_pkey i ASC
t_pkey j N/A
t_j_key j ASC
t_j_key crdb_internal_i_shard_7 ASC
t_j_key i ASC

# Alter the primary key to be no longer hash-sharded.
statement ok
ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (i)

# Now assert that the primary key has been modified to non-hash-sharded,
# and the unique index still exists.
query TTT
SELECT index_name,column_name,direction FROM [SHOW INDEXES FROM t]
----
t_pkey i ASC
t_pkey j N/A
t_j_key j ASC
t_j_key i ASC

0 comments on commit 190246b

Please sign in to comment.