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: Support for selecting from views #9355

Merged
merged 2 commits into from
Sep 15, 2016

Conversation

a-robinson
Copy link
Contributor

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

In the process, open the floodgates to grabbing leases on views.
I believe I've handled all the callers that get a table lease, but let
me know if you think I might have missed something!

Only look at the last commit in this PR - everything else is just a copy of #9212, which I'll rebase on top of once it gets in.

@dt @paperstreet


This change is Reviewable

@a-robinson
Copy link
Contributor Author

#2971

@a-robinson
Copy link
Contributor Author

Hm, I've locally rebased this on top of #9212, but in the process broke the test case that joins two views together. Investigating...

@a-robinson
Copy link
Contributor Author

So the issue got triggered by something in #9327. If I run the view logic tests against what's currently up in this PR, it works, but if I cherry-pick #9327 on top of it, the join in the logic test starts panicking. It even affects pre-existing tables/views that were created on disk before #9327. Taking a closer look at the contents of #9327 now.

@a-robinson
Copy link
Contributor Author

Looks like the change to AllocateIDs that ensures a primary key exists is the culprit, somehow. I'll rework it to make more sense for views.

@dt
Copy link
Member

dt commented Sep 14, 2016

huh, sorry about that.

maybe we should a) unify some view and virtual table handling (i.e. #9361) then b) opt non-persisted tables out of ensuring they have PK/Families?

@a-robinson
Copy link
Contributor Author

I haven't checked out virtual tables enough to know what unification would involve, but I can try taking a look if you'd like. I assume this is the same conversation as #9361?

@dt
Copy link
Member

dt commented Sep 14, 2016

yeah, I think it's fine to check IsView to opt views out of automatic PKs and Family allocations, plus the places in Validate which will need to check that, and then maybe make IsView true for virtual tables too (which is kinda the direction I was thinking on #9361).

@a-robinson
Copy link
Contributor Author

Alright, I've opted views out of PKs, indexes, and families, and shuffled the validation code around a little bit to accommodate. I'm happy to do more invasive surgery if you think it'd be better long term.

Copy link
Member

@dt dt left a comment

Choose a reason for hiding this comment

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

LGTM

I think I'd have a slight preference to keeping AllocateIDs() callable on any tabledesc and having it Do The Right Thing internally, but I don't feel that strongly.

@danhhz
Copy link
Contributor

danhhz commented Sep 15, 2016

I do think we need more invasive surgery, this is all getting a little hard to follow. But we certainly shouldn't shave that particular yak in this commit.

I haven't looked deeply at this commit, but would it be possible to move to a structure where AllocateIDs does whatever work is shared between physical tables, views, and information_shcema and once calls AllocatePhysicalTableIDs for regular tables? Or is the logic too intermingled for something like that?

(edit)
If not, then an LGTM from david is good enough for me.

In the process, open the floodgates to grabbing leases on views.
I believe I've handled all the callers that get a table lease, but let
me know if you think I might have missed something!
@a-robinson
Copy link
Contributor Author

How about this version? It merges a tiny bit more of the logic for views and virtual tables and makes it safe to call AllocateIDs and ValidateTable on all table descriptors. It's stricter about not allocating even an ID to the nameless primary index on virtual tables, so I had to tweak the hard-coded table descriptors for the information_schema.

@a-robinson
Copy link
Contributor Author

All the relevant changes around that are in the first commit, in case it's not clear

Copy link
Member

@dt dt left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@a-robinson a-robinson merged commit d2dcab1 into cockroachdb:develop Sep 15, 2016
@jseldess jseldess mentioned this pull request Oct 10, 2016
20 tasks
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