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: fix logic to detect if primary index constraint name is in use #75155

Merged
merged 1 commit into from
Jan 27, 2022

Conversation

fqazi
Copy link
Collaborator

@fqazi fqazi commented Jan 19, 2022

Fixes: #75154

Previously, when dropping and adding a primary constraint inside
a transaction with the same name would fail since we would detect
a primary index with the same name was active. This was inadequate
because replacing an index within a transaction is supported, even
if the name is the same. To address this, this patch checks if
the index is marked for deletion via the disabled flag, in which
case the name is not currently in use because the index will be
dropped in the transaction.

Release note (bug fix): Dropping and creating a primary index constraint in the same transaction with the same name would incorrectly fail.

@fqazi fqazi requested review from ajwerner, postamar and a team January 19, 2022 16:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@fqazi fqazi changed the title sql: fix logic to detect if primary index constraint name is in use [DNM] sql: fix logic to detect if primary index constraint name is in use Jan 19, 2022
@fqazi
Copy link
Collaborator Author

fqazi commented Jan 19, 2022

Looking into some CI stuff this isn't quiet right.

@fqazi fqazi force-pushed the constraintErr branch 2 times, most recently from 29459ea to e608026 Compare January 25, 2022 17:15
@fqazi fqazi changed the title [DNM] sql: fix logic to detect if primary index constraint name is in use sql: fix logic to detect if primary index constraint name is in use Jan 25, 2022
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.

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @postamar)


pkg/sql/logictest/testdata/logic_test/alter_table, line 2120 at r1 (raw file):

create table const_tbl (a int, b int, constraint "bob" primary key (a, b));
alter table const_tbl drop constraint "bob";
alter table const_tbl add constraint "bob" primary key (a, b); -- this statement

Can you do this multiple times in the same transaction to exercise that case too?

Code quote:


alter table const_tbl add constraint "bob" primary key (a, b); -- this statement

@fqazi fqazi force-pushed the constraintErr branch 2 times, most recently from 161ad2e to 52713c6 Compare January 26, 2022 18:02
Copy link
Collaborator Author

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)


pkg/sql/logictest/testdata/logic_test/alter_table, line 2120 at r1 (raw file):

Previously, ajwerner wrote…

Can you do this multiple times in the same transaction to exercise that case too?

Added a test to cover that case.

@fqazi fqazi requested a review from ajwerner January 26, 2022 18:02
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 1 of 2 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, and @postamar)


pkg/sql/logictest/testdata/logic_test/alter_table, line 2188 at r2 (raw file):

COMMIT;

statement error relation \"const_tbl\" \(150\): unimplemented: primary key dropped without subsequent addition of new primary key in same transaction.*

replace the number here with \d+?

Fixes: cockroachdb#75154

Previously, when dropping and adding a primary constraint inside
a transaction with the same name would fail since we would detect
a primary index with the same name was active. This was inadequate
because replacing an index within a transaction is supported, even
if the name is the same. To address this, this patch checks if
the index is marked for deletion via the disabled flag, in which
case the name is can be safely used (the index with the original
name is dropped).

Release note (bug fix): Dropping and creating a primary index
constraint with the same name in a transaction would incorrectly fail.
@fqazi
Copy link
Collaborator Author

fqazi commented Jan 26, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 27, 2022

Build succeeded:

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: dropping and creating primary index constraint with the same name fails
3 participants