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: allow schema changes on tables in state: TableDescriptor_ADD #15929

Merged
merged 1 commit into from
May 17, 2017

Conversation

vivekmenezes
Copy link
Contributor

A new table descriptor is placed in the non-public TableDescriptor_ADD state
when it refers to another table through an FK or interleaved reference,
and can only be made public once the other tables back reference is
visible across the cluster.

This change allows schema change operations on such non-public tables,
while preserving the non-public behavior for table leases.

fixes #13505

@vivekmenezes vivekmenezes requested a review from dt May 15, 2017 18:15
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dt
Copy link
Member

dt commented May 16, 2017

:lgtm: though I'm a little uneasy about the common mustGet.* helpers being willing to return non-public descriptors by default -- while the current check in data_source might cover current uses, another might sneak in if someone isn't expecting these semantics / doesn't carefully read the comment. How much uglier would it be for them to return desc, filterTableState(desc) and then let schema change callsites decide if they want proceed on errTableAdding ?


Review status: 0 of 3 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@vivekmenezes
Copy link
Contributor Author

I thought about what would be safest change to make here and felt that passing in a boolean allowAdding makes it explicit, so that's what I've done. Turns out dan just merged a change for show_fingerprints that should disallow the use of a table in the ADD state and that could have easily slipped through the cracks without the boolean flag. Thanks for your feedback.


Review status: 0 of 9 files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@vivekmenezes
Copy link
Contributor Author

Let me add a few more tests to this PR before merging it.


Review status: 0 of 9 files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

A new table descriptor is placed in the non-public TableDescriptor_ADD state
when it refers to another table through an FK or interleaved reference,
and can only be made public once the other tables back reference is
visible across the cluster.

This change allows schema change operations on such non-public tables,
while preserving the non-public behavior for table leases.

fixes cockroachdb#13505
@vivekmenezes
Copy link
Contributor Author

I've added a few more tests to this PR. Thanks


Review status: 0 of 9 files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@dt
Copy link
Member

dt commented May 17, 2017

:lgtm:


Review status: 0 of 9 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

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: index creation fails if issued just after create table in same txn
4 participants