Adds session option to cache query plans #3246
Conversation
go/vt/vtgate/executor.go
Outdated
| @@ -280,6 +284,22 @@ func (e *Executor) handleSet(ctx context.Context, session *vtgatepb.Session, sql | |||
| default: | |||
| return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unexpected value for client_found_rows: %d", val) | |||
| } | |||
| case "no_cache_query_plan": | |||
There was a problem hiding this comment.
Although this was the option we discussed IRL, as a general rule I try to avoid having the "negative" case encoded in options, even when it is the default.
In other words, I think we should change this so the exposed option is set cache_query_plan with a default value of 1.
go/vt/vtgate/executor.go
Outdated
| return plan, nil | ||
| } | ||
|
|
||
| // getNoCacheQueryPlanFromSession extracts NoCacheQueryPlan from session |
There was a problem hiding this comment.
This is a mouthful and the "FromSession" is implicit in the function signature.
Also in light of the above comment, I'd change this to be func shouldCacheQueryPlan(session *vtgatepb.Session) bool
proto/query.proto
Outdated
| @@ -287,6 +287,10 @@ message ExecuteOptions { | |||
| } | |||
|
|
|||
| TransactionIsolation transaction_isolation = 9; | |||
|
|
|||
| // no_cache_query_plan specifies if the query plan shoud be cached by vitess. | |||
There was a problem hiding this comment.
Note that even if we switch the exposed set option, this needs to stay as-is so that the default value in the ExecuteOptions protobuf stays 0.
Not that I know of
Yes! |
rafael
force-pushed
the
add-session-option-for-query-plan-cache
branch
from
71b50e5 to
da58bc5
Compare
|
@demmer addressed most of the comments. I have one comment about: I agree with the idea and I also try to avoid negative options. However, in this case I would argue we should keep it like this to be consistent. If |
rafael
force-pushed
the
add-session-option-for-query-plan-cache
branch
from
da58bc5 to
77afafc
Compare
rafael
changed the title
|
Addressed negative option by renaming |
rafael
force-pushed
the
add-session-option-for-query-plan-cache
branch
from
77afafc to
3be2507
Compare
|
Change looks good. Can you rebase against the latest master first and make sure all the tests pass? I worry that the latest dedup improvements may affect some tests. |
rafael
force-pushed
the
add-session-option-for-query-plan-cache
branch
from
1ae1d62 to
7d5e2a7
Compare
rafael
force-pushed
the
add-session-option-for-query-plan-cache
branch
from
7d5e2a7 to
fdf7ec3
Compare
rafael
force-pushed
the
add-session-option-for-query-plan-cache
branch
from
fdf7ec3 to
d37ad9b
Compare
|
I had a bad rebase and I think @googlebot got confused. It should be good now :) |
Motivation
Recently we ran into some issues due to query plan cache in vtgate growing too fast and causing OOM errors in the machine. It turned out, that in this context we don't need to be caching query plans.
This commit adds a new option to ExecuteOptions. It allows clients to set a
session variable to not cache query plans.
If you are using mysql server plugin, you can set this variable with:
By default, all query plans are cached (existent behavior)
Open Questions
CLIENT_FOUND_ROWS, it would be way more convenient. However, I didn't find a clear path to support this :(Tests