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: Restrict renames of objects that views depend on #9988

Merged
merged 3 commits into from
Oct 19, 2016

Conversation

a-robinson
Copy link
Contributor

@a-robinson a-robinson commented Oct 14, 2016

This change is Reviewable

@a-robinson
Copy link
Contributor Author

Also fix creation of back-references to only include columns actually used by the view query.

@dt

statement ok
CREATE VIEW v AS SELECT name FROM users@{FORCE_INDEX=ufo}

statement error cannot rename index "ufo" because it is depended on by view "test.v"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the behavior other SQL engines provide? An alternative would be to propagate the rename through to the VIEW.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd certainly be preferable behavior, but we've basically punted on query rewriting until we can have a proper semantic encoding of the view's query rather than the current text-based one - #9045 (comment)

Unless you're suggesting that perhaps this particular case of rewriting is simple enough to handle?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I wasn't suggesting the rewriting was easy, just wondering what the long-term goal is here. This is fine in the short-term.

One thought about how to avoid the VIEW query rewriting is that we can extend our SQL syntax in order to allow referring to tables/columns/indexes by ID. Then we'd do the name resolution once when the VIEW is created. Easy to describe this, but it would likely be a whole bunch of work to actually accomplish.

@a-robinson
Copy link
Contributor Author

Friendly ping :)

Also, I've added a commit here to improve the test coverage for dropping indexes that a view depends on and the error messages for view-related drop errors. It's fairly independent logically, but I wanted to reuse some of the error-generating code that I added for renames to add detail to the error messages that I think improves them. Feel free to disagree on that though, perhaps the extra verbosity isn't useful?

@dt dt self-assigned this Oct 19, 2016
@dt
Copy link
Member

dt commented Oct 19, 2016

:lgtm: though we could drop TODOs in to do the query rewrite, or even use the unimplemented error message that includes the issue to help clue in users who might expect it to Just Work.


Reviewed 1 of 1 files at r1.
Review status: 1 of 13 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@a-robinson
Copy link
Contributor Author

a-robinson commented Oct 19, 2016

Good point, added a TODO pointing to #10083. I didn't update the error message because I'm not sure of a way to phrase it that isn't pretty unwieldy, but do you think I should?

@a-robinson a-robinson merged commit f3b5885 into cockroachdb:master Oct 19, 2016
This was referenced Oct 19, 2016
@a-robinson a-robinson deleted the vrename branch January 6, 2017 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants