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

distsql: support local execution for tenants #47902

Closed
tbg opened this issue Apr 22, 2020 · 6 comments
Closed

distsql: support local execution for tenants #47902

tbg opened this issue Apr 22, 2020 · 6 comments
Assignees
Labels
A-multitenancy Related to multi-tenancy

Comments

@tbg
Copy link
Member

tbg commented Apr 22, 2020

Background

In phase 2, DistSQL won't work and needs to be disabled "properly". (At the time of writing, one may want to use DistSQL since it's set to auto, but it fails during planning and falls back to regular execution - this adds overhead).

This work should wait until #47903 is done.

Related to #47900

@tbg tbg added A-multitenancy Related to multi-tenancy A-multitenancy-blocker labels Apr 22, 2020
@tbg tbg changed the title distsql: don't try to use it on a phase 2 sql tenant server distsql: support local execution for tenants May 1, 2020
@tbg
Copy link
Member Author

tbg commented May 1, 2020

I had missed something I am now learning in the context of #48110 - there is no alternative to DistSQL, i.e. local execution still uses DistSQL, but without distributing the plan.

Unfortunately this means that we can't "just turn off" DistSQL. We need to make sure local execution still does work. Doing so properly is likely a bit of work; we don't want to end up in a place where it just works "heuristically" but the code is a mess.

cc @asubiotto feels like this is an important item for you (or someone) to look into once #48110 merges, as without it even SELECT 1 fails when there's no NodeID.

@tbg
Copy link
Member Author

tbg commented May 1, 2020

I think I managed to get this into good enough shape in #48110, though there's likely some cleanup to be had. As of that PR, when the nodeID is not available (=we're running a tenant) we force DistSQL to plan locally. And it seems that after some finagling, that's good enough to make sure local execution never asks for the nodeID.

@asubiotto
Copy link
Contributor

We need to make sure local execution still does work

What exactly do you mean by this? "local execution" (i.e. executing planNodes) is gone forever, not distributing the distsql plan is a supported equivalent. which is what happens when a user sets distsql=off or we execute something we don't thing is going to benefit from distribution (e.g. point lookups).

I would expect a lack of NodeID to mess some stuff up but nothing fundamental but I might be missing something. I'm more than happy to take a look at anything that's failing.

@tbg
Copy link
Member Author

tbg commented May 1, 2020

As I mentioned, it wasn't as bad as I thought. I have things back to passing.

@yuzefovich
Copy link
Member

yuzefovich commented Jun 15, 2020

Just noticed that this issue is left in 20.2 B milestone as "Done" but is not closed. Should it be closed?

@asubiotto
Copy link
Contributor

Yes, #49697 closed this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy
Projects
None yet
Development

No branches or pull requests

3 participants