sql: turn on query cache #34454
sql: turn on query cache #34454
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball, @justinj, and @rytaft)
|
bors r+ |
Build failed |
|
bors r+ |
|
I fixed a race condition in the query cache code. The @RaduBerinde, I put a TODO in the code for you to see if there's a better fix. For now, I made a copy of the |
|
bors r- |
Canceled |
|
bors r+ |
Merge conflict (retrying...) |
Build failed |
andy-kimball
force-pushed
the
cache-on
branch
from
8907c7d
to
6ed8655
Compare
Release note (performance improvement): A query plan cache saves a portion of the planning time for frequent queries.
The PreparedMetadata was being put into the cache, but then the InferredTypes field was being set after that by execPrepare. The fix is to make a copy of PreparedMetadata before entering it into the cache. Release note: None
andy-kimball
force-pushed
the
cache-on
branch
from
6ed8655
to
317a77d
Compare
|
bors r+ |
34454: sql: turn on query cache r=andy-kimball a=RaduBerinde Release note (performance improvement): A query plan cache saves a portion of the planning time for frequent queries. Some benchmarks with KV: https://docs.google.com/spreadsheets/d/1n818OsKTWsatbpXo5ddOJ53mL4hY-JhNJEuIlytilLc/edit#gid=0 While there is some improvement, the implicit prepare case is still nowhere close to prepare-once; I plan to benchmark and do more work on the prepare path. I think it's a good idea to enable the cache now though, so we get more time to weed out any problems. Co-authored-by: Radu Berinde <radu@cockroachlabs.com> Co-authored-by: Andrew Kimball <andyk@cockroachlabs.com>
Build failed |
|
Thanks, LGTM! I will look at moving that InferredTypes code earlier, before we add to the cache (or otherwise remove it from PrepareMetadata). |
|
Last run was a flake in TestDockerCLI, trying again. bors r+ |
Build failed |
|
Ah.. looks like we have to invalidate a cached memo if SafeUpdates changes. I guess I didn't run into that because updates were not planned with opt (I assume they are now?) |
|
Alternatively we could move that check to execution - seems like that's where it belongs conceptually. |
Do not use cached memo if the SafeUpdates setting does not match. Release note: None
|
I added a check for it. |
|
bors r+ |
34454: sql: turn on query cache r=andy-kimball a=RaduBerinde Release note (performance improvement): A query plan cache saves a portion of the planning time for frequent queries. Some benchmarks with KV: https://docs.google.com/spreadsheets/d/1n818OsKTWsatbpXo5ddOJ53mL4hY-JhNJEuIlytilLc/edit#gid=0 While there is some improvement, the implicit prepare case is still nowhere close to prepare-once; I plan to benchmark and do more work on the prepare path. I think it's a good idea to enable the cache now though, so we get more time to weed out any problems. Co-authored-by: Radu Berinde <radu@cockroachlabs.com> Co-authored-by: Andrew Kimball <andyk@cockroachlabs.com>
Build succeeded |
|
Thank you, looks great! |
Release note (performance improvement): A query plan cache saves a
portion of the planning time for frequent queries.
Some benchmarks with KV: https://docs.google.com/spreadsheets/d/1n818OsKTWsatbpXo5ddOJ53mL4hY-JhNJEuIlytilLc/edit#gid=0
While there is some improvement, the implicit prepare case is still nowhere close to prepare-once; I plan to benchmark and do more work on the prepare path. I think it's a good idea to enable the cache now though, so we get more time to weed out any problems.