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 panic adding unique, non-indexable column #26684

Merged
merged 1 commit into from
Jun 14, 2018
Merged

sql: Fix panic adding unique, non-indexable column #26684

merged 1 commit into from
Jun 14, 2018

Conversation

bobvawter
Copy link
Contributor

@bobvawter bobvawter commented Jun 13, 2018

This change updates the checkColumnsFor...Index functions to use
allNonDropColumns() to validate against any proposed mutations. Otherwise, a
ColumnMutation that adds a non-indexable column followed by an
IndexMutation creating an index on that column would would be incorrectly
accepted, leading to a panic.

Fixes #26483

Release note (sql change): Return an error to the user instead of panicing when
trying to add a column with a unique constraint when that column's type is not
indexable.

@bobvawter bobvawter requested review from maddyblue, vivekmenezes and a team June 13, 2018 14:08
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@maddyblue maddyblue left a comment

Choose a reason for hiding this comment

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

My concern from a high level is that this is an unsafe API for us to use. The change here makes it safe by going through a special path because we have special knowledge of how to do things in the right order.

Is there a solution which makes the invalid state impossible to produce using the public API?

@vivekmenezes
Copy link
Contributor

I'd add a test that adds the same column type and an index as two statements in the same transaction.

I wonder if this problem can be fixed by allowing the index to be added but then changing
func (desc *TableDescriptor) ValidateTable( to check for this problem by including the column mutations in the check.

@knz
Copy link
Contributor

knz commented Jun 14, 2018

@bobvawter as commented on the linked issue, this bug is not specific to arrays, it is general to any non-indexable type. That includes json, for example. I recommend updating the commit title and PR title accordingly, for clarity in the git history.

@bobvawter bobvawter changed the title sql: Fix panic adding unique, array column sql: Fix panic adding unique, non-indexable column Jun 14, 2018
This change updates the `checkColumnsFor...Index` functions to use
`allNonDropColumns()` to validate against any proposed mutations.  Otherwise, a
`ColumnMutation` that adds a non-indexable column followed by an
`IndexMutation` creating an index on that column would would be incorrectly
accepted, leading to a panic.

Fixes #26483

Release note (sql change): Return an error to the user instead of panicing when
trying to add a column with a unique constraint when that column's type is not
indexable.
@bobvawter bobvawter requested a review from a team June 14, 2018 14:05
@bobvawter
Copy link
Contributor Author

PTAL: I found a much simpler way to do this by just using allNonDropColumns(); updated the test to also check ADD COLUMN; CREATE INDEX in a transaction; revised the commit message.


Review status: :shipit: complete! 0 of 0 LGTMs obtained


Comments from Reviewable

@knz
Copy link
Contributor

knz commented Jun 14, 2018

:lgtm: very nice find.


Reviewed 2 of 2 files at r1.
Review status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


Comments from Reviewable

@bobvawter
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Jun 14, 2018
26684: sql: Fix panic adding unique, non-indexable column r=bobvawter a=bobvawter

This change updates the `checkColumnsFor...Index` functions to use
`allNonDropColumns()` to validate against any proposed mutations.  Otherwise, a
`ColumnMutation` that adds a non-indexable column followed by an
`IndexMutation` creating an index on that column would would be incorrectly
accepted, leading to a panic.

Fixes #26483

Release note (sql change): Return an error to the user instead of panicing when
trying to add a column with a unique constraint when that column's type is not
indexable.

26705: roachtest: use --sequential when generating store dumps r=benesch a=petermattis

This makes the mapping from cockroach node ID to roachprod node ID the
identify function.

Release note: None

Co-authored-by: Bob Vawter <[email protected]>
Co-authored-by: Peter Mattis <[email protected]>
@craig
Copy link
Contributor

craig bot commented Jun 14, 2018

Build succeeded

@craig craig bot merged commit 12b3ab7 into cockroachdb:master Jun 14, 2018
@bobvawter bobvawter deleted the fix-26483 branch June 14, 2018 16:25
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.

None yet

5 participants