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 PRIMARY KEY rewrites secondary index if new PK subsets old PK #84303

Conversation

Xiang-Gu
Copy link
Contributor

@Xiang-Gu Xiang-Gu commented Jul 12, 2022

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.

Fixes: #84040

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.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@Xiang-Gu Xiang-Gu force-pushed the rewrite_secondary_index_when_new_pk_columns_subsets_old_pk_columns branch from 08534aa to 5ccd4ed Compare July 13, 2022 14:23
@Xiang-Gu Xiang-Gu marked this pull request as ready for review July 13, 2022 14:23
@Xiang-Gu Xiang-Gu requested a review from a team July 13, 2022 14:23
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @Xiang-Gu)


pkg/sql/logictest/testdata/logic_test/alter_primary_key line 1645 at r1 (raw file):

# were rewritten, and hence dropping a column from the old PK columns does not
# unexpectedly drop an existing secondary index.
subtest regression_#84040

Can you add a test that explicitly tests the hash sharded index case?

Code quote:

# 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

@Xiang-Gu Xiang-Gu force-pushed the rewrite_secondary_index_when_new_pk_columns_subsets_old_pk_columns branch 2 times, most recently from 6c92b44 to 190246b Compare July 14, 2022 19:16
@postamar
Copy link
Contributor

@Xiang-Gu Xiang-Gu changed the title sql/alter_primary_key: rewrite secondary index if new PK subsets old PK sql: ALTER PRIMARY KEY rewrites secondary index if new PK subsets old PK Jul 15, 2022
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.
@Xiang-Gu Xiang-Gu force-pushed the rewrite_secondary_index_when_new_pk_columns_subsets_old_pk_columns branch from 190246b to 53bf4d5 Compare July 15, 2022 22:10
@Xiang-Gu
Copy link
Contributor Author

bors r+

@craig craig bot merged commit a3d74c3 into cockroachdb:master Jul 15, 2022
@craig
Copy link
Contributor

craig bot commented Jul 15, 2022

Build succeeded:

@blathers-crl
Copy link

blathers-crl bot commented Jul 15, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 53bf4d5 to blathers/backport-release-21.2-84303: 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 21.2.x failed. See errors above.


error creating merge commit from 53bf4d5 to blathers/backport-release-22.1-84303: 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 22.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@postamar
Copy link
Contributor

@Xiang-Gu don't forget to backport this one manually as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: alter pk using subset of old pk key columns should always rewrite secondary indexes
4 participants